Problem/Motivation

Toolbar tabs aren’t working properly, see the unexpected behavior below.
toolbar tabs not working

Observed behavior:
- Sometimes it works, but most of the time it doesn’t
- The expected tab is not activated consistently

Expected beheavior
- The clicked tab should be activated
- The other tabs should be deactivated
- The submenu (subtree) should appear

Steps to reproduce

1. Install a fresh Drupal 11.x site
2. Log in as admin
3. Click on the toolbar menu tabs — the clicked tab should become active, and the others should be deactivated, sometimes the clicked tab even open the sub menu.

Proposed resolution

Sometimes, for an unknown reason, Backbone does not trigger or update the activeTab model correctly.
To address this, I propose manually triggering the change event when the user clicks a tab.

We can trigger this event inside the onTabClick function in the ToolbarVisualView.js file.
Right after setting the model, similar to how it’s already triggered when the page loads:
this.model.trigger('change:activeTab');

Remaining tasks

  • Code review

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#22 3529674-nr-bot_11qx5u6z.txt910 bytesneeds-review-queue-bot
#16 rec-after-patch.gif131.82 KBangel_devoeted
#16 rec-before-patch.gif135.12 KBangel_devoeted
#11 Confrim Tests Success with Patch.png298.65 KBmdyoung3
#11 Confirm Tests Fail without patch.png399.2 KBmdyoung3
#9 tests-after-patch.png156.2 KBeric.vvf
#9 tests-before-patch.png201.93 KBeric.vvf
#6 Toolbar tab behavior.mkv155.99 KBroshanibhangale
#4 tabs-working.gif2.07 MBjoaopauloc.dev
tabs-is-not-working.gif3.09 MBjoaopauloc.dev

Issue fork drupal-3529674

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:

About issue forks

Comments

joaopauloc.dev created an issue. See original summary.

joaopauloc.dev’s picture

joaopauloc.dev commented
Title: Toolbar tabs when clicked is not working properly. » Toolbar tab behavior is inconsistent when clicked.

joaopauloc.dev opened merge request !12358

joaopauloc.dev’s picture

joaopauloc.dev commented
StatusFileSize
new tabs-working.gif2.07 MB

Toolbar working properly after applied the fix of the pull request.
toolbar tab fixed

joaopauloc.dev’s picture

joaopauloc.dev commented
Status: Active » Needs review
roshanibhangale’s picture

roshanibhangale commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Toolbar tab behavior.mkv155.99 KB

Hi

I have tested the above issue on Drupal 11.x.

Steps to reproduce

  1. 1. Install a fresh Drupal 11.x site
  2. 2. Log in as admin
  3. 3. Click on the toolbar menu tabs — the clicked tab should become active, and the others should be deactivated, sometimes the clicked tab even open the sub menu.

After successfully applied MR12358, the toolbar menu tabs are working as expected. The clicked tab is activated, The other tabs are deactivated and The submenu (subtree) is appearing properly.
Hence this is RTBC +1.

Attaching video for reference.

smustgrave’s picture

smustgrave commented
Status: Reviewed & tested by the community » Needs work

Seems like something that can expand a javascript test

eric.vvf made their first commit to this issue’s fork.

eric.vvf’s picture

eric.vvf commented
StatusFileSize
new tests-before-patch.png201.93 KB
new tests-after-patch.png156.2 KB

I was able to reproduce this on Drupal 11.x-dev. It’s not a super obvious one to hit.

  1. Fresh site, logged in (I used a clean/anonymous browser tab).
  2. By default the Administration tab is active.
  3. If I only toggle Administration <> User (and toggle back to Admin), everything looks fine and I can't trigger the problem.
  4. The bug shows up when I go User <> another tab that is not Administration, e.g. Shortcuts (make sure the shortcut module is enabled and the user has access toolbar + access shortcuts).
  5. After clicking User > Shortcuts, I end up with two active tabs at the same time (both aria-pressed="true").

Based on @smustgrave’s note, I wrote a FunctionalJavascript test that follows that exact sequence:

  1. Normalize to Administration active
  2. Click the User tab (works as expected)
  3. Click the Shortcuts tab (this is where it breaks pre-patch)

Results:
Fails without @joaopaulocdev patch (two elements match > test goes red).
results before patch

All tests passed after the patch:
results before patch

eric.vvf’s picture

eric.vvf commented
Status: Needs work » Needs review
mdyoung3’s picture

mdyoung3 commented
StatusFileSize
new Confirm Tests Fail without patch.png399.2 KB
new Confrim Tests Success with Patch.png298.65 KB

I ran an install of 11.x-dev from the fork for this on my local. I clicked through the tabs and was able to replicate the bug on my local.

Then I checked out of the branch with the patch, and I was able to confirm it fixed the issue.

Also checked the test, which ran successfully for me with the patch...

Confirm Tests Pass with Patch

And failed without the patch.

Confirm Tests Fail without Patch

I hope this is a good review. Let me know if more is needed as it's my first time contributing. Please and thank you.

ressa’s picture

ressa
he/him
commented

Thanks everyone for working on fixing this. I see high levels of dogged persistence by several contributors to replicate the bug, and will just mention that here is an even more demanding one to verify, with a lot more steps. But wouldn't it be great to fix it?

#3540339: Rebuild all caches, including APCu, when clicking "Clear all caches"

At least getting a verification whether there truly is a problem, would be awesome.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Looking at the current steps I don't see how shortcuts is needed. Note toolbar is being deprecated from core and Shortcut is up for discussion #3476880: [Policy] Move Shortcut module to contrib so may not want to mix their tests.

mdyoung3’s picture

mdyoung3 commented

Toolbar's deprecation is currently postponed: https://www.drupal.org/project/drupal/issues/3484850

Probably worth getting the fix in.

joaopauloc.dev’s picture

joaopauloc.dev commented
Status: Needs work » Needs review

Removed the dependency of shortcut module.

angel_devoeted’s picture

angel_devoeted commented
StatusFileSize
new rec-before-patch.gif135.12 KB
new rec-after-patch.gif131.82 KB

Hi

I tested this on a clean Drupal 11.x-dev using Drupal Forge.

Before applying MR !12358, I was able to reproduce the issue where multiple toolbar tabs could remain active at the same time.

After applying the patch, the behavior is now correct (only one tab stays active, and sub-menus open and close as expected).

Before / After screenshots

Before

Multiple toolbar tabs remain active

After

Only one toolbar tab stays active and submenus behave correctly

I also checked the test file

core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php

and confirmed that the Shortcut module dependency was removed and replaced with the toolbar_test module.

That keeps the test independent from Shortcut (based on its possible move out of core).

Everything works as expected on my end and matches the before/after screenshots I captured.

RTBC +1.

oily’s picture

oily commented

The test-only test outputs:

PHPUnit 11.5.42 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.14
Configuration: /builds/issue/drupal-3529674/core/phpunit.xml.dist
..F                                                                 3 / 3 (100%)
Time: 00:16.514, Memory: 8.00 MB
There was 1 failure:
1) Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegrationTest::testSwitchBetweenMultipleTabs
Failed asserting that false is true.
/builds/issue/drupal-3529674/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php:176
/builds/issue/drupal-3529674/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php:147
FAILURES!
Tests: 3, Assertions: 41, Failures: 1.
Exiting with EXIT_CODE=1

Test coverage seems fine.

oily’s picture

oily commented
Issue summary: View changes
Issue tags: -Needs issue summary update

Have applied the IS template and re-structured/ re-written the IS accordingly.

The test coverage seems good. The code needs reviewed.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative, +Bug Smash Initiative

I can confirm this from the screenshot in #16, at first I wasn't able to trigger it but with the user link it was definitely reproducible very easily.

The MR does address the problem in all combinations of user link, shortcuts, and main tab. I did rebase the MR as it was 400+ commits back.

MR is still green

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

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3529674-nr-bot_11qx5u6z.txt910 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.