Problem/Motivation

hook_menu_tree() is used to generate the menu of navbar.

That means a custom hook menu tree definition in a theme may remove the default .menu class from all menu trees including the navbar. In case of navbar the toolbar stops working in this case.

This example clashes already with bartik_menu_tree() that adds .menu .clearfix to navbar menu tree. For the reason that the .clearfix class is added by bartik some things in navbar required wonky CSS overrides to get the clearfix issues pushed out. This wonky overrides hurts us in uncountable many cases and we will always need to add more workarounds if we do not solve the menu_tree root cause.

Proposed resolution

Add a navbar_menu_tree() theming function and add our own menu class to navbar menus e.g. '<ul class="navbar-menu">' . $variables['tree'] . '</ul>';. This way we stop injecting .menu and .clearfix class and get all a lot leaner and more reliable CSS code. As we no longer rely on standard hook_menu_tree the navbar no longer clashes with theme CSS and this will start working with all contrib themes. If a theme maintainer like to override he could do, too as there is a navbar specific theme function and not only one global that affects all menus.

Remaining tasks

Review and commit

User interface changes

None.

API changes

None.

Original report by hass

See attached screenshot that shows one of the typical bugs.

2013-10-25_000505.png

CommentFileSizeAuthor
#21 Issue-2119989-by-hass-Add-navbar_menu_tree-to-prevent-theme_clashes-D7.patch13.29 KBhass
#19 Issue-2119989-by-hass-Add-navbar_menu_tree-to-prevent-theme-clashes.patch18.61 KBhass
2013-10-25_000505.png6.33 KBhass
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

hass CreditAttribution: hass commented
hass’s picture

hass CreditAttribution: hass commented
Status: Active » Closed (duplicate)

My custom theme is injecting again my custom class and not .menu.

<ul class="ym-menu">
  <li class="first leaf"><a href="/drupal7/en/node/add">Add content</a></li>
  <li class="last leaf"><a href="/drupal7/en/admin/content">Find content</a></li>
</ul>

This is the known menu_tree theming issue again. We really need to fix this!!! Suxxx

hass’s picture

hass CreditAttribution: hass commented

It's a bit different here. I have no chance to override this class here as there is no hook_menu_tree__shortcuts. No idea how to solve correctly.

hass’s picture

hass CreditAttribution: hass commented
Status: Closed (duplicate) » Active

That is Drupal theme_tree hell... shortcut_set_1 is a name that can be configured by users. That's really a joke.

/**
 * @FIXME Remove once fixed in navbar
 * Return HTML for D7 navbar module with class 'menu' or navbar fails to render.
 */
function mytheme_menu_tree__shortcut_set_1($variables) {
  return '<ul class="menu">' . $variables['tree'] . '</ul>';
}
jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented
Issue tags: +Needs issue summary update

I'm sorry, but I don't really understand any of the comments and how they relate to the image in the issue summary.

@Hass: Could you make this any easier to understand, by updating the issue summary to our standard format?

Also, what is the "known menu_tree theming issue" to which you refer, is there a link to that issue?

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented

Are you implying that this is something that would be resolved by #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template?

hass’s picture

hass CreditAttribution: hass commented

#1944572: Remove "ul.menu" dependency to prevent theme clashes

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

#1898464: toolbar.module - Convert theme_ functions to Twig is close to being complete. It needs some guiding so that we can put this .menu issue to rest once and for all.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

hass, could you include a small CSS file in your theme for Navbar as a library and add the library if the Navbar module is enabled? I hate to make this kind of soft dependency in your theme, but it seems as if you've got some advanced theme layer manipulation in your code and I'd like to see find resolution to this family of issues with menu trees.

hass’s picture

hass CreditAttribution: hass commented

The root cause of this issue is .menu class again. We need to remove this from the module and toolbar module, too. It's really the only option we have to make this rocket stable. Otherwise navbar will fail in every theme that override menu_tree what it already an issue in core, too.

It looks like I'm just not a good candidate for writing the patch for this... :-(

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

This issue is really bothering me, too, and I have yet to come up with a way around it.

There must be a way to avoid that theme function altogether in Navbar and just use a custom list theme function.

hass’s picture

hass CreditAttribution: hass commented
Title: Gap between shortcuts first level and second level » Remove all .menu classes to prevent theme clashes
Priority: Normal » Critical
Issue summary: View changes

This is one if not the highest prio case that need to be fixed.

alibama’s picture

alibama CreditAttribution: alibama commented

I'm using openframework from stanford that also seems to have some bootstrap in it - I'm seeing similar failure, reverted to admin menu. Really like navbar, look forward to seeing a functional release

meecect’s picture

meecect CreditAttribution: meecect commented

My issue was closed as a duplicate of this ( https://www.drupal.org/node/2238133 ). I have a css snippet that one could use in your theme to solve the issue for at least me, using bootstrap and the navbar shortcuts menu. Basically, I just added this:

.drupal-navbar .navbar-tray-shortcuts ul.nav:before, .drupal-navbar .navbar-tray-shortcuts ul.nav:after {
content: "";
display:  none;
}

It gets the job done. If you know anything about the history of Drupal, waiting around for a cleaner and more official fix may take years.

hass’s picture

hass CreditAttribution: hass commented
Issue summary: View changes
hass’s picture

hass CreditAttribution: hass commented
Issue summary: View changes
hass’s picture

hass CreditAttribution: hass commented
Issue tags: -Needs issue summary update
hass’s picture

hass CreditAttribution: hass commented
Title: Remove all .menu classes to prevent theme clashes » Add navbar_menu_tree() to prevent theme clashes
hass’s picture

hass CreditAttribution: hass commented
Status: Active » Needs review
FileSize
Issue-2119989-by-hass-Add-navbar_menu_tree-to-prevent-theme-clashes.patch18.61 KB

Here is the patch that works perfectly fine and solves all the many many issues and I think it fixes a lot of other open issues.

We can cleanup the CSS files from useless overrides in a follow up case. It would over-complicate this case otherwise I think.

hass’s picture

hass CreditAttribution: hass commented
Issue summary: View changes
hass’s picture

hass CreditAttribution: hass commented
FileSize
Issue-2119989-by-hass-Add-navbar_menu_tree-to-prevent-theme_clashes-D7.patch13.29 KB

When I ported this code to D8 I found a theme override solution and ported these back to D7 now. It requires a lot less code and is easier to review. Let's go with this one.

hass’s picture

hass CreditAttribution: hass commented
Issue summary: View changes
eshta’s picture

eshta CreditAttribution: eshta commented

@hass - this approach is great. Much cleaner. I'm going to test it out locally on a few things before I RTBC it. I encourage any other folks following this issue to give it a spin and add some feedback.

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Needs review » Reviewed & tested by the community
hass’s picture

hass CreditAttribution: hass commented

Will you commit this? :-)

eshta’s picture

eshta CreditAttribution: eshta commented

Yup... Just about to :-)

Was running through the list of items to get a sense of everything going in.

  • eshta committed ac4687d on 7.x-1.x authored by hass 
    Issue #2119989 by hass: Add navbar_menu_tree() to prevent theme clashes
eshta’s picture

eshta CreditAttribution: eshta commented
Status: Reviewed & tested by the community » Fixed
hass’s picture

hass CreditAttribution: hass commented

Great. Let's try to get #2227293: Remove navbar from print view done before a new release so we may be done for a very long time.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea commented

Followup bug #2404801: Shortcut table used, but Shortcut module may be disabled.

hass’s picture

hass CreditAttribution: hass commented

Follow up bug #2403649: Menu items in RTL not properly aligned

hass’s picture

hass CreditAttribution: hass commented
Parent issue: » #1944572: Remove "ul.menu" dependency to prevent theme clashes

Please also help getting the D8 patch in core.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.