Problem/Motivation
The toggle button is missing from all other toolbar tabs other than "Manage". #3257504: Empty toolbar tray displays a stray orientation toggle introduced a change to prevent the toolbar orientation toggle from appearing in otherwise-empty toolbar tabs. The fix for that issue caused this side-effect bug. Checking for .has(.toolbar-menu) was not an ideal approach as there were scenarios where this JavaScript is running before all the relevant .toolbar-menu classes are present.
Core 9.3.21 using the Umami demo

Core 9.4.0 using the Umami demo

When there is a toolbar tab added by a module, like "Workbench," only the first tab gets the icon. So the "Workbench" tab has the icon and the "Manage" tab has a button but no icon due to missing value attribute. The user tab has no toggle button at all.
Steps to reproduce
1. Install the shortcut module (or anything that adds a new tab to the toolbar)
2. Go to that tab and see the icon for horizontal/vertical toggle is missing
Proposed resolution
Solve the problem of #3257504: Empty toolbar tray displays a stray orientation toggle in a different way: if a tab is empty, then hide the entire tab with display: none;
Remaining tasks
Review.
User interface changes
Introduced terminology
NA
API changes
NA
Data model changes
NA
Release notes snippet
NA
| Comment | File | Size | Author |
|---|---|---|---|
| #37 | toolbar_html.png | 79.8 KB | dcam |
| #36 | toolbar.png | 24.36 KB | dcam |
Issue fork drupal-3305152
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
jonraedeke commentedComment #3
redamakhchan commentedSame direction this cause a js error for users without manage tab :
Uncaught TypeError: $orientationToggleButton[0] is undefined updateTrayOrientation ../core/modules/toolbar/js/views/ToolbarVisualView.js?v=9.4.5:139This JS code assume toggle button are always there, but for the last update it is only available on Manage Tab.
Comment #5
broonThis issue was introduced by committing the patch in #3257504: Empty toolbar tray displays a stray orientation toggle.
If you remove the
.has('.toolbar-menu')part from Core again, you'll have the buttons back (but also a stray button if there is no menu at all).Comment #6
broonAlso, this happens for users who may see the toolbar but are not admins as they will see the "Manage" tab although it has no items. Thus, the menu is not really existant.
See #2135445: Toolbar displays Manage tab even if the user is not permitted to see it
Comment #7
broonHere's a patch that reverts the introduction of the "has-menu" check. This is not a final solution but works fine in conjunction with the patch in https://www.drupal.org/project/drupal/issues/2135445#comment-13003616 to show the admin toolbar for users without admin permissions.
Comment #8
sinn commentedFix from the #7 for Drupal 10
Comment #10
bnjmnmComment #12
bnjmnmIgnore the
escape-admin-pathbranch, that was accidental.The
3305152-toggle-icon-formerge request, however is ready for review.This fixes the underlying issue without undoing the benefits of #3257504: Empty toolbar tray displays a stray orientation toggle. Now, if the manage tab has an empty tray, the tab itself is not visible either.
Note that checking for
.has(.toolbar-menu)was not an ideal approach as there were scenarios where this JavaScript is running before all the relevant.toolbar-menuclasses are present.Comment #13
smustgrave commentedFailures in MR seem legit to the issue.
Comment #14
bnjmnmTest updated to account for empty trays not being displayed anymore.
Comment #15
smustgrave commentedRebase seems good.
Comment #16
smustgrave commentedSorry wrong ticket. Will take a look today.
Comment #17
smustgrave commentedVerified this issue on a fresh install of 10.1
Verified MR solved the issue.
Comment #19
chi commentedThis might not be relevant, but when looking at screenshot #17 don't you think that "Edit shortcuts" link is not in place?
Comment #20
Tbower89 commentedTried to apply the patch from Comment #7, but the issue still existed. Noticing that it was applying only on the first element, added a loop so that it would apply to all the trays on the toolbar to fix the issue. A hard cache clear on the browser is required to see this work. This was done for Drupal Core 9.5.8.
Comment #22
Ratan Priya commentedRe-rolled #20 for 11.x-dev
Comment #25
agentrickardNote on review of #20, if your Shortcut toolbar has no items in it, you will get a fatal JS error:
That logic needs an
if !== undefinedwrapper.Comment #26
le72Noted the issue on Drupal 10.2
Comment #27
nterbogt commentedThis patch caused significant issues for us for non admin users. I'm yet to look into what is different in the HTML structure between admin and non admin users that might be affecting it.
It could be some local change also, but flagging it here so others can be aware and test.
Update: Further investigation and it appears the patch is incompatible with the Siteimprove toolbar item implementation. Showing / hiding that item from users... shows and hides the bug.
Comment #29
smustgrave commentedRebased for main pipeline is now green.
Comment #30
smustgrave commentedThis came up on a client project and the solution by @bnjmnm worked great btw.
Comment #32
dcam commentedI'm having a bad time reviewing this issue. It's been one thing after another causing issues, not all of which are relevant. The latest thing is that the new "inert" class isn't being applied to toolbar tabs. The edited JS is being loaded on the page. There's no errors in the console. I'm viewing the toolbar as a user with no admin permissions, so the tab is empty. But there's no class. What's worse, the stray orientation toggle that was previously fixed by #3257504: Empty toolbar tray displays a stray orientation toggle is back.
Does anyone have any thoughts about this?
I'm updating the problem/motivation section of the issue summary to make it clear that this was caused by the fix for the other issue and that this is related to empty toolbar tabs. It isn't just about the orientation buttons. I had to figure that out by reading through comments and the previous issue. Please update it if I got any of it wrong.
Comment #33
smustgrave commentedThe empty toolbar bug from before had test coverage but not catching this?
Comment #34
dcam commentedThere's test coverage for the new inert class too, but that isn't working on my local either. Something is really off about the whole thing. I assume it's something about my environment, but I can't explain it. Is it tester error? Or is there some path that causes this to happen? I don't know.
Comment #35
smustgrave commentedYea I'm not entirely too sure either..
Comment #36
dcam commentedOk! I figured it out!
MenuVisualView.jshides the tab if it's empty. I had Twig debug turned on, which was outputting comments in the HTML node being checked for emptiness. So the "empty" tab wasn't actually empty, so the JS didn't add the inert class. Once I turned off Twig debugging the admin menu tab disappeared.That said, there may still be problems. I happened to turn on the "Use shortcuts" permission for the user under test. There's nothing in the tab, but it doesn't get the inert class. Here's a screenshot:

So you can see that the admin tab is hidden there, but the empty Shortcuts is visible. And you can see the stray orientation toggle. Are you able to verify this result?
Comment #37
dcam commentedCould it be because the JS checks the emptiness of a menu's HTML node, but an empty Shortcut menu's wrapper isn't rendered?

If it's true, then maybe this solution doesn't work for any tab that doesn't conform to specific HTML.
Comment #38
smustgrave commentedShould this be back to NW?
Comment #39
dcam commentedI don't think it will get attention if not. I was looking for someone to verify my results from #36 and #37.