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>';
}
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:
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff.txt | 5.19 KB | star-szr |
#43 | 1944572-43.patch | 14.67 KB | star-szr |
#33 | interdiff.txt | 1.04 KB | star-szr |
#33 | 1944572-33.patch | 12.21 KB | star-szr |
#32 | 1944572-31.patch | 11.7 KB | star-szr |
Comments
Comment #0.0
hass CreditAttribution: hass commenteda
Comment #0.1
hass CreditAttribution: hass commenteda
Comment #1
tim.plunkettThose 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.
Comment #2
hass CreditAttribution: hass commentedNope. Toolbar should not use this classes. This causes a lot of bugs and I already work on this with jesse.
Comment #3
tim.plunkettOkay, let's see something then.
Comment #4
hass CreditAttribution: hass commentedJessebach is the maintainer of toolbar as i know and has no questions. See #1939642: Reset .menu margin from system.css in toolbar.base.css. Currently, toolbar relies on Bartik style.css to do this..
Comment #5
jessebeach CreditAttribution: jessebeach commentedIf 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
Comment #6
jessebeach CreditAttribution: jessebeach commentedDowngrading to Normal.
Comment #6.0
jessebeach CreditAttribution: jessebeach commentedA
Comment #7
jessebeach CreditAttribution: jessebeach commentedThe same
.menu .clearfix
sin is about to be repeated in this template patch fortheme_menu_tree
. Best to cut this off at the pass.#1898478: menu.inc - Convert theme_ functions to Twig
Comment #8
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis should help with #2149783: Toolbar CSS styles are too weak; common theme styles unintentionally distort it
Comment #9
hass CreditAttribution: hass commentedWe are getting closer...
Comment #10
hass CreditAttribution: hass commentedComment #11
hass CreditAttribution: hass commentedTwig file need to be named
menu--admin.html.twig
, but currently it cannot saved incore/toolbar/templates
. It only works incore/themes/seven
.But submenus are still not rendered as menu_admin.
Comment #12
hass CreditAttribution: hass commentedWith my work on #2119989: Add navbar_menu_tree() to prevent theme clashes I was able to complete this task here, too.
Comment #14
hass CreditAttribution: hass commentedI found an easier solution. Patch attached.
Comment #16
hass CreditAttribution: hass commentedComment #17
hass CreditAttribution: hass commentedD8 patch
Comment #19
dawehnerIt 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
Comment #20
hass CreditAttribution: hass commentedThe 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.
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?
Comment #21
hass CreditAttribution: hass commented@dawehner: I tried twig blocks, but it's not working. See #2402181: Optimize toolbar-menu template with Twig template inheritance
Comment #22
hass CreditAttribution: hass commentedComment #23
hass CreditAttribution: hass commentedComment #24
hass CreditAttribution: hass commentedAre the maintainers of Toolbar able to review and RTBC the patch, please?
Comment #25
mgiffordI'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?
Comment #28
idebr CreditAttribution: idebr commentedThis should fix the failing test.
Comment #29
idebr CreditAttribution: idebr commentedComment #30
star-szrHere are some thoughts, @Fabianx reminded me about this issue on IRC.
This can be done in preprocess.
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?
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 :)
Comment #31
star-szrI 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.
Comment #32
star-szrMissing attachment. Not totally sure about all the CSS conflicts, that's where all the conflicts were. Could use another set of eyes.
Comment #33
star-szrTypo alert! Also found another toolar to clean up, but that could be a separate issue too.
Comment #34
star-szrWow, 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
Comment #35
hass CreditAttribution: hass commented@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?
Comment #36
star-szrAt 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.
Comment #37
hass CreditAttribution: hass commentedWe 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.
Comment #38
hass CreditAttribution: hass commentedNow I see. Code has been changed a few days ago. http://cgit.drupalcode.org/drupal/commit/core/modules/system/templates/m...
Comment #39
hass CreditAttribution: hass commentedClassy hardcodes the class again.
Comment #40
star-szrIn 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.
Comment #41
hass CreditAttribution: hass commentedThe 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
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.
Comment #42
star-szrI am working on something to add a suggestion for the toolbar case. Adding a related issue for menu blocks.
Comment #43
star-szrMoved 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.
Comment #44
hass CreditAttribution: hass commentedAnother problem here is that
hook_preprocess_menu(&$variables)
also has no context andhook_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!
Comment #45
hass CreditAttribution: hass commentedComment #46
hass CreditAttribution: hass commented#43: If that works it looks better than mine code for sure. :-)
Comment #47
LewisNyman CreditAttribution: LewisNyman commentedComment #48
hass CreditAttribution: hass commentedComment #49
hass CreditAttribution: hass commentedAdding one inconsitency case.
Comment #51
hass CreditAttribution: hass commentedTested patch and works as advertised.
Comment #52
star-szrI don't think this is ready yet. It should have some manual testing and screenshots since it changes quite a lot.
Comment #53
hass CreditAttribution: hass commentedThis 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.
Comment #54
webchickWow, 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.
Comment #55
hass CreditAttribution: hass commentedWithout 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.
Comment #56
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedComment #57
nod_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 arbitrarymenu
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.
Comment #60
nod_Comment #61
webchickOk 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!