Follow up for #1938044: Prefix all toolbar classes to prevent theme clashes

My theme overrides theme_menu_tree() with it's own html code. Navbar and Toolbar menus seem to expect that class ul.menu exists, but it doesn't. Navbar should expect only it's own classes and add more classes if required. e.g. use ul.navbar-submenu. Bartic theme also injects a clearfix to navbar via bartic_menu_tree() that is not expected/wished, too. To avoid these type of theming issues toolbar need to add it's own theming for the navbar menus. system.menu.css also injects an unwanted margin with ul.menu, see #1938742: Navbar should override margin in system.css (ul.menu li) that has been fixed in navbar and #1939642: Reset .menu margin from system.css in toolbar.base.css. Currently, toolbar relies on Bartik style.css to do this. may be obsolete than.

Example code in themes that cause such type of issues:

/**
 * Returns HTML for a wrapper for a menu sub-tree.
 */
function mytheme_menu_tree($variables) {
  return '<ul class="foo-menu">' . $variables['tree'] . '</ul>';
}
Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Issue priority Major because it breaks contrib themes
Unfrozen changes Unfrozen because it only changes CSS and markup to display the toolbar properly in contrib themes and the D7 backport has already been committed to navbar project.
Disruption Disruptive for core/contributed and custom modules/themes because it will require a BC break/deprecation/internal refactoring/widespread changes... if it does not get committed asap.

Screenshot that shows the bug:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

Issue summary: View changes

a

hass’s picture

Issue summary: View changes

a

tim.plunkett’s picture

Category: bug » task
Priority: Major » Normal

Those classes are added by system module. Toolbar should be able to rely on system module. If you'd like to decouple it further, that's fine.

hass’s picture

Category: task » bug
Priority: Normal » Major

Nope. Toolbar should not use this classes. This causes a lot of bugs and I already work on this with jesse.

tim.plunkett’s picture

Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs tests

Okay, let's see something then.

hass’s picture

Status: Postponed (maintainer needs more info) » Active
jessebeach’s picture

Status: Active » Postponed

If Toolbar could insert its own class into a render array with some sanity, then this would be a non-issue. We will be able to do this once the TWIG conversion/refactor of theme_menu_tree is finished. Hass, if you have time to work on that conversion, that would be the place to focus:

#1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template

jessebeach’s picture

Priority: Major » Normal

Downgrading to Normal.

jessebeach’s picture

Issue summary: View changes

A

jessebeach’s picture

The same .menu .clearfix sin is about to be repeated in this template patch for theme_menu_tree. Best to cut this off at the pass.

#1898478: menu.inc - Convert theme_ functions to Twig

Jeff Burnz’s picture

hass’s picture

hass’s picture

Issue tags: +Needs backport to D7
hass’s picture

Status: Postponed » Active

Twig file need to be named menu--admin.html.twig, but currently it cannot saved in core/toolbar/templates. It only works in core/themes/seven.

But submenus are still not rendered as menu_admin.

hass’s picture

With my work on #2119989: Add navbar_menu_tree() to prevent theme clashes I was able to complete this task here, too.

Status: Needs review » Needs work
hass’s picture

I found an easier solution. Patch attached.

Status: Needs review » Needs work
hass’s picture

hass’s picture

dawehner’s picture

+++ b/core/modules/toolbar/templates/toolbar-menu.html.twig
+++ b/core/modules/toolbar/templates/toolbar-menu.html.twig
@@ -0,0 +1,44 @@

@@ -0,0 +1,44 @@
+{#
+/**
+ * @file
+ * Default theme implementation to display a toolbar menu.
+ *
+ * Available variables:
+ * - menu_name: The machine name of the menu.
+ * - items: A nested list of menu items. Each menu item contains:
+ *   - attributes: HTML attributes for the menu item.
+ *   - below: The menu item child items.
+ *   - title: The menu link title.
+ *   - url: The menu link url, instance of \Drupal\Core\Url
+ *   - localized_options: Menu link localized options.
+ *
+ * @ingroup themeable
+ */
+#}
+{% import _self as menus %}
+
+{#
+  We call a macro which calls itself to render the full tree.
+  @see http://twig.sensiolabs.org/doc/tags/macro.html
+#}
+{{ menus.menu_links(items, attributes, 0) }}
+
+{% macro menu_links(items, attributes, menu_level) %}
+  {% import _self as menus %}
+  {% if items %}
+    {% if menu_level == 0 %}
+      <ul{{ attributes.addClass('toolbar-menu') }}>
+    {% else %}
+      <ul class="toolbar-menu">
+    {% endif %}
+      {% for item in items %}
+        <li{{ item.attributes }}>
+          {{ link(item.title, item.url) }}
+          {% if item.below %}
+            {{ menus.menu_links(item.below, attributes, menu_level + 1) }}
+          {% endif %}
+        </li>
+      {% endfor %}
+    </ul>
+  {% endif %}
+{% endmacro %}

It is sad that you have to override the entire template in order to do that. Did we considered to introduce some twig block regions here, see http://twig.sensiolabs.org/doc/templates.html#template-inheritance

hass’s picture

The twig block override you posted looks like a good idea, but I do not know if this has been considered already. Menu template looks like a good candidate. We should open a follow up case for his and optimize there. I hope I have not wasted my time too late because of beta bugfix rules.

+++ b/core/modules/toolbar/toolbar.module
@@ -42,6 +42,10 @@
+  $items['menu__admin'] = array(
+    'variables' => array('items' => array(), 'attributes' => array()),
+    'template' => 'toolbar-menu',
+  );
 

These theme overriding is also something I do not like... Yesterday I implemented such a module theme override the very first time and learned a lot about the lacks of documentation in hook_theme() and doc pages. d.o and Google yielded nothing, too. I was not aware that every module can override a theme function of any other module. Lesson learned the hard way.

If the menu template suggestions would be more flexible we may be able to get rid of these code. I thought about a region suggestion for menus. It's very often required for menus. The missing context of menus is a real and old problem.

Can you RTBC this patch, please?

hass’s picture

@dawehner: I tried twig blocks, but it's not working. See #2402181: Optimize toolbar-menu template with Twig template inheritance

hass’s picture

Issue summary: View changes
hass’s picture

Issue summary: View changes
hass’s picture

Are the maintainers of Toolbar able to review and RTBC the patch, please?

mgifford’s picture

I'm not a CSS guy, but implemented this on SimplyTest.me to see if it worked as expected. It seems to.

@dawehner - do you have concerns with committing this and opening up a follow-up issue?

Status: Needs review » Needs work
idebr’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
13.79 KB

This should fix the failing test.

idebr’s picture

Issue tags: +frontend, +CSS
star-szr’s picture

Issue tags: +Twig

Here are some thoughts, @Fabianx reminded me about this issue on IRC.

  1. +++ b/core/modules/toolbar/templates/toolbar-menu.html.twig
    @@ -0,0 +1,44 @@
    +      <ul{{ attributes.addClass('toolbar-menu') }}>
    

    This can be done in preprocess.

  2. +++ b/core/modules/toolbar/templates/toolbar-menu.html.twig
    @@ -0,0 +1,44 @@
    +      <ul class="toolbar-menu">
    

    This can't as is, but what if we rename attributes to top_attributes, and add a fourth parameter (optional) to hold the attributes for the > 0 level <ul>'s in menu.html.twig? Then preprocess could add that in.

    That would allow us to eliminate this special case template altogether. Not sure how to do it cleanly without an API change to the 'menu' theme hook though, ideally we would want that to hold defaults for 'top_attributes' and 'attributes'.

    Edit: Is this even needed though? Why is this class needed on every level?

  3. +++ b/core/modules/toolbar/toolbar.module
    @@ -42,6 +42,10 @@ function toolbar_theme($existing, $type, $theme, $path) {
    +  $items['menu__admin'] = array(
    +    'variables' => array('items' => array(), 'attributes' => array()),
    +    'template' => 'toolbar-menu',
    +  );
    

    If we go this route I think we should leave out the 'template' line so the template becomes menu__admin, and/or use a more specific theme suggestion. Also, I think normally when we add hook_theme() items containing underscores like this, we set the 'base hook' key. Could be wrong on that though :)

star-szr’s picture

Status: Needs review » Needs work

I wanted to do a markup comparison to confirm/deny #30.2, but the patch needed a reroll - attached.

My confusion stemmed from comparing the system menu.html.twig which doesn't have the 'menu' class on the > 0 level <ul>'s. This also means if we're adding a new template it needs to be done in Classy as well.

Adding a Twig block like in #2402181: Optimize toolbar-menu template with Twig template inheritance and #19 may be another possibility worth exploring further as well. But it does seem to me that if this class is required for JS then adding it via preprocess is pretty fair even in the banana world.

star-szr’s picture

Status: Needs work » Needs review
FileSize
11.7 KB

Missing attachment. Not totally sure about all the CSS conflicts, that's where all the conflicts were. Could use another set of eyes.

star-szr’s picture

FileSize
12.21 KB
1.04 KB

Typo alert! Also found another toolar to clean up, but that could be a separate issue too.

star-szr’s picture

Wow, that other "toolar" has been there over 2 years! Wonder what it looks like when that is fixed. #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop

hass’s picture

@cottser: this file is a 1:1 clone of menu.html.twig. This file also define menu class and is not using preprocess for adding a class. We need to remove all .menu classes from all levels. It worked 100% correct and was 100% final. There is no need to change anything in this patch or you need to change menu first. Is there any reason why we should complicate things by not doing the same like we do with menu?

star-szr’s picture

Status: Needs review » Needs work

At the very least #30.3 needs to be addressed and the changes need to be made in the Classy template. Most of the other concerns are around duplication and maintainability.

@hass on a higher level I'm not sure what you mean by 100% correct and 100% final. Getting a patch to RTBC is a process, just because the patch is green doesn't mean it's done and I think you've been around long enough to know this. Thanks for your work so far and I hope you'll continue to push this issue forward.

hass’s picture

We need to keep the template name. Menu template does not fit as it hardcodes .menu class and we cannot override it. You should give it a try and you may find out that menu has no template suggestion that can be used. You cannot create menu--admin.html.twig as it will never used. See the suggestions added. They are all useless.

hass’s picture

hass’s picture

Classy hardcodes the class again.

star-szr’s picture

Issue tags: +JavaScript

In a separate issue we should probably refactor the JS to use a data attribute or something instead. I think there's a general issue about that somewhere but I can't find it at the moment.

After further review and testing I don't think the approach taken by the patch is viable because it would also affect a menu block that you can create for the Administration menu with just a few clicks. It's just too fragile. I think we may need to look into some other options here.

hass’s picture

The main issue here are the suggestions. We need much better menu suggestions. The issue relies all on the missing context. Menu template has no context. So you have no idea that the menu is generated for

  • menu
  • menu__admin
  • menu__admin__block
  • menu__admin__sidebar1

If we had suggestions that works like this we could make this code a lot better and allow easier menu customizations. But I have no idea how this could be archived. Until than this approach is the best we can do for now. Nobody can move the toolbar into a block as this will not work and break the toolbar, too.

Feel free to take over and add these suggestions to menu templates. I hope you find a way.

star-szr’s picture

I am working on something to add a suggestion for the toolbar case. Adding a related issue for menu blocks.

star-szr’s picture

Moved the "toolar" clean-up in toolbar.module.css to this issue: #2448023: Fix or remove dead CSS in toolbar.module.css

Here's another approach still using a template override. Will keep playing to see what else is possible.

hass’s picture

Another problem here is that hook_preprocess_menu(&$variables) also has no context and hook_preprocess_menu__admin(&$variables) cannot defined in a module as I know.

In preprocess you are blind what menu the code is currently generated/altered. Menu theming is a mess!

hass’s picture

hass’s picture

#43: If that works it looks better than mine code for sure. :-)

LewisNyman’s picture

hass’s picture

hass’s picture

Adding one inconsitency case.

hass queued 43: 1944572-43.patch for re-testing.

hass’s picture

Status: Needs review » Reviewed & tested by the community

Tested patch and works as advertised.

star-szr’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing, +Needs screenshots

I don't think this is ready yet. It should have some manual testing and screenshots since it changes quite a lot.

hass’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs screenshots

This is what I have done. All css classes are properly set in code. Since NOTHING changes on UI there are no changes at all in GUI. All looks like before. This patch is only to allow themers to override theme functions and to prevent clashes if menu class is removed from theme menu.

We also seriously tested all this stuff in D7 and it works there, too.

webchick’s picture

Assigned: Unassigned » nod_
Status: Reviewed & tested by the community » Needs review

Wow, this is a much bigger change than one would think from looking at the issue title.

Going by our newfangeldy core governance structure, this probably approaches the threshold of "significant change" to a subsystem, and could therefore use sign-off from a Toolbar maintainer. There is currently only one of those listed, and it is nod_, so assigning to him for final sign-off.

hass’s picture

Without this patch there is no way to make sure toolbar is not completly broken in contrib themes. This a very serious bug for an extended time and cannot wait for 8.1.

Fabianx’s picture

nod_’s picture

Assigned: nod_ » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

Toolbar works and looks the same using any core theme.

I didn't realize tools for themeing menus were that poor, new template suggestions++. JS changes looks good, CSS changes too.

Bit of a shame that modules displaying a menu inside of a toolbar tray have to declare the arbitraty toolbar-menu class, but it doesn't change much from declaring the no less arbitrary menu class. It's a much lesser evil. I'm happy with this, was expecting a bigger change based on #54 :þ

Next best thing would be #2402181: Optimize toolbar-menu template with Twig template inheritance indeed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 43: 1944572-43.patch, failed testing.

webchick queued 43: 1944572-43.patch for re-testing.

nod_’s picture

Status: Needs work » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok great, thanks for that extra bit of review! CSS/JS largely goes over my wee head. :) Thanks too for sticking with this one, hass!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 72c59cd on 8.0.x
    Issue #1944572 by hass, Cottser, idebr: Remove "ul.menu" dependency to...

Status: Fixed » Closed (fixed)

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