The toolbar handler uses menu.link_tree service to create the menu, which does not add the toolbar specific classes. This causes display issues with the devel menu when using default margins and padding for uls versus the resets used in the toolbar menu templates and css. The handler should use the toolbar.menu_tree extension class which adds the necessary templates.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeNCM created an issue. See original summary.

mikelutz’s picture

willzyx’s picture

Status: Active » Needs review

@mikeNCM thanks for contributing! moving to NR for trigger the testbot

Status: Needs review » Needs work

The last submitted patch, 2: devel-use_toolbar_menu_tree_for_toolbar-2881521-2-d8.patch, failed testing.

mikelutz’s picture

So, my motivation for this change is an issue with the margin in the toolbar menu. My original patch needs a little bit more thought to work across the board. The main issue is that the ul that holds the devel menu needs to have zero top and bottom margin to look right. in bartik, ul.menu is set to 0 top and bottom margin, however in seven, it is given .25em top and bottom margin, and with a custom theme, chrome defaults to 1em top and bottom margin on a ul. Toolbar fixes this in its menus by using the toolbar menu tree, which gives the ul a class of .toolbar-menu instead of .menu. This class is given zero margin in core stable css. The patch in #2 switched devel to use this menu tree, and changed the class from .menu to .toolbar menu.

Good Toolbar:
Standard Toolbar

Devel tray in seven:
Devel menu with Seven

Devel tray in custom theme on chrome.
Devel menu with chrome margin

This caused a few additional problems though. First, in bartik, the ul gets a clearfix added, and this combined with the floated right 'configure' option in the devel toolbar means that the css in devel needs to be updated to float the new ul to the left (or right in rtl) or bartik will have the configre button clear the rest of the menu, and end up on a second line.

The second issue is the tests, which are looking for the wrong css class to run. They are easily updated.

So, the two options are to update the patch to fix the tests and the css, which I have done and included here. I think it's the 'right' thing to do to use the toolbar menu tree to create the toolbar menu. The only issue is that it does change the class of the ul, and could potentially break some css on someone's custom theme. I think this is unlikely, and given the fact that devel is primarily used for developers anyway, I don't believe any CSS changes will seriously negatively affect anybody.

The other solution is to explicitly add a margin:0; in the devel css for the .toolbar .toolbar-tray-horizontal .menu selector, which is already there. this makes it a one line change, doesn't change the css class, and may be a better option for those reasons. It's really a question for the maintainers as to how important maintaining backwards-compatible css is.

willzyx’s picture

Status: Needs work » Needs review
willzyx’s picture

FileSize
35.01 KB

@mikeNCM Looks good. I think that the change that you suggest is quite reasonable and probably we can commit it.. but the patch is incomplete and cause some visual regressions. For example (as you describe before) the result using bartik is something like

mikelutz’s picture

Bah, my fault, wrong file. It doesn't include the updates to the CSS file. I'll post the right one shortly.

mikelutz’s picture

  • willzyx committed b318390 on 8.x-1.x authored by mikeNCM
    Issue #2881521 by mikeNCM, willzyx: Toolbar Handler should use toolbar....
willzyx’s picture

Status: Needs review » Fixed

Committed and pushed to 8.x. Thanks and sorry for the delay!

Status: Fixed » Closed (fixed)

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