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

toolbar toggle in core 9.3

Core 9.4.0 using the Umami demo

toolbar toggle in core 9.4

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

Issue fork drupal-3305152

Command icon 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

jonraedeke created an issue. See original summary.

jonraedeke’s picture

Issue summary: View changes
redamakhchan’s picture

Same 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:139

This JS code assume toggle button are always there, but for the last update it is only available on Manage Tab.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

broon’s picture

This 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).

broon’s picture

Also, 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

broon’s picture

StatusFileSize
new1.48 KB

Here'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.

sinn’s picture

Status: Active » Needs review
StatusFileSize
new606 bytes

Fix from the #7 for Drupal 10

Status: Needs review » Needs work

The last submitted patch, 8: drupal-toggle_icon-3305152-8-D10.patch, failed testing. View results

bnjmnm’s picture

Version: 9.5.x-dev » 10.1.x-dev

bnjmnm’s picture

Status: Needs work » Needs review

Ignore the escape-admin-path branch, that was accidental.

The 3305152-toggle-icon-for merge 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-menu classes are present.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Failures in MR seem legit to the issue.

bnjmnm’s picture

Status: Needs work » Needs review

Test updated to account for empty trays not being displayed anymore.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Rebase seems good.

smustgrave’s picture

Status: Reviewed & tested by the community » Needs review

Sorry wrong ticket. Will take a look today.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new34.33 KB

Verified this issue on a fresh install of 10.1
Verified MR solved the issue.

1

lauriii made their first commit to this issue’s fork.

chi’s picture

This might not be relevant, but when looking at screenshot #17 don't you think that "Edit shortcuts" link is not in place?

Tbower89’s picture

Tried 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.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Ratan Priya’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new2.01 KB
new2.01 KB

Re-rolled #20 for 11.x-dev

Status: Needs review » Needs work

The last submitted patch, 22: 3305152-22.patch, failed testing. View results

agentrickard’s picture

Note on review of #20, if your Shortcut toolbar has no items in it, you will get a fatal JS error:

$orientationToggleButton[0] is not defined

That logic needs an if !== undefined wrapper.

le72’s picture

Noted the issue on Drupal 10.2

nterbogt’s picture

StatusFileSize
new31.28 KB

This 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.

smustgrave’s picture

Version: 11.x-dev » main
Issue summary: View changes
Status: Needs work » Needs review

Rebased for main pipeline is now green.

smustgrave’s picture

This came up on a client project and the solution by @bnjmnm worked great btw.

dcam made their first commit to this issue’s fork.

dcam’s picture

Issue summary: View changes

I'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.

smustgrave’s picture

The empty toolbar bug from before had test coverage but not catching this?

dcam’s picture

The empty toolbar bug from before had test coverage but not catching this?

There'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.

smustgrave’s picture

Yea I'm not entirely too sure either..

dcam’s picture

StatusFileSize
new24.36 KB

Ok! I figured it out! MenuVisualView.js hides 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:
A screenshot showing the hidden admin menu tab and a Shortcuts that that is visible when it shouldn't be.

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?

dcam’s picture

StatusFileSize
new79.8 KB

Could it be because the JS checks the emptiness of a menu's HTML node, but an empty Shortcut menu's wrapper isn't rendered?
A screenshot of HTML from a toolbar with a hidden admin menu and visible shortcuts menu.
If it's true, then maybe this solution doesn't work for any tab that doesn't conform to specific HTML.

smustgrave’s picture

Should this be back to NW?

dcam’s picture

Status: Needs review » Needs work

I don't think it will get attention if not. I was looking for someone to verify my results from #36 and #37.

oily made their first commit to this issue’s fork.

oily changed the visibility of the branch escape-admin-path to hidden.