Cannot expand toolbar items referencing "<nolink>" or with options "_no_path"

This issue is referring to functionality that is added by the Admin Toolbar contrib module. Moving this issue from core to admin_toolbar issue queue.

A duplicate issue was posted later here. I'm updating this issue summary with some of the information posted there in order to improve/simplify the steps to reproduce and to add a proposed solution.

Adding screenshot to illustrate the problem

This is a problem even without admin_toolbar, so moving back to core. See attached screenshot.

Component got messed up, apologies.

Updating issue summary with image from @tedfordgif

IMHO, I'm seeing two part of this issue needs to be handled.

1. The styling for <nolink> menu item
2. The expand button for <nolink> menu item that has children menu items (either link or nolink)

What is the approach for initItems? Should we target span by a tag element or another class?

Currently the expand button may be shown by adding the target element on ititItems at core/modules/toolbar/js/toolbar.menu.js, line 104.

      // Initialize items and their links.
      $menu.find('li > a').wrap('<div class="toolbar-box">');

      // Initialize items and their links.
      $menu.find('li > a, li > span').wrap('<div class="toolbar-box">');

Screenshot of sidebar after change.
Not expanded

Expanded

Added a patch to display workable change without proper styling.

We discussed this issue at #3324991: Drupal Usability Meeting 2022-12-09. That issue will have a link to a recording of the meeting.

For the record, the attendees at today's usability meeting were @Antoniya, @benjifisher, @g-brodiei, @rkoller, and @simohell.

We agree that this issue is a usability bug, and I am adding the issue tag for that. The usability team usually does not have an opinion on how to implement an issue.

I notice that there is a patch attached to Comment #14, so the status should not be Active. The comment points out that the styling still needs work, so I am setting the status to NW, not NR.

A bugfix usually needs to include automated tests, in order to make sure that future code changes do not cause regressions. I am adding the issue tag for that, too. The Toolbar module already has two Nightwatch tests, and we can probably add a few lines to one of those.

Thank you @benjifisher.

Updating the patch for proper styling. This includes PostCSS + CSS files on the Claro admin theme as it overrides the original CSS provided by core's toolbar module.

As for testing, I skimmed through both of the nightwatch test files, can't find any test that's related to assert newly created menu items; On the other hand, I discovered a functional test for "testExternalLink" on core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php, probably something to work on for "testNoLink"?

Adding gif file to display styling after patch on Claro admin theme.

The CSS for span tag was appended with Class selector span[class*="toolbar-icon-menu-link-content:"]to avoid effects on other unrelated span tags underbutton tag.

Verified the patch #16 manually on Drupal 10.0.7. The patch was applied cleanly and worked as expected.

Attached is a screenshot after applying the patch.

Applying Patch:

After Applying Patch:

Setting status to needs review, still needs tests to be written

#16 failed to apply

Issue summary should be updated with the definitive proposed solution.

As mentioned tests are still needed.

Updating the patch #16 as it failed to apply. the patch solves the issue. Updating the issue summery with proposed resolution.

Rerolled the patch from #21 to apply to 10.2.7.

The patch seems to fix the issue.

Thanks everyone for the help!

Hello,

The latest patch #23 seems to break CSS aggregation for me.

It is missing a "-" character at this place :

Rerolled patch #23 with the previous bug fixed

I've created a MR that applies the patch in #26 (double checked: issue still exists, patch still applies, patch fixes the issue).

Can anybody please confirm that this needs test coverage? I'm asking, because I cannot find any existing tests that verify if the expand functionality works for normal links, so it seems a bit odd to write a test only for <nolink> items. Thanks!

Hello, i have tested this and have few observations as below.
I added a menu item with and set to expanded and added a sub item below it - After applying the MR the expand collapse functionality worked fine.
The patch 26 fails to apply so styling could not be checked.

Next, I also checked by adding menu item with a normal link, and set to expanded and added a sub item below it and checked, the menu item is not correctly shown.

Added screenshots for reference.

Definitely think we would still need test coverage. Can almost say if we had test coverage before this bug may not have popped up.

I have created a new Merge Request based on the 11.x branch to resolve this issue, as the previous forks were outdated or had conflicts.
Changes made:
* Fixed js/toolbar.menu.js to open items submenu (javascript now added open/close feature a links and )
* Fixed /css/toolbar.module.css and /css/toolbar.theme.css para mejora visual
* Added new /FunctionalJavascript to test problem

I've verified the changes locally and they are ready for review.

MR for main appears to be failing.

"I have updated the Merge Request to target the main branch and performed a rebase to resolve previous syntax errors and out-of-date dependencies.
The CI is now passing (the previous failure in testLanguageOfPartsPlugin was a random fail unrelated to my changes, as confirmed by the successful re-run).
Summary of changes:

Fixed the Toolbar bug in core/modules/toolbar.
Added a FunctionalJavascript test to cover the fix.

Ready for review.

Left some small comments around the test, thanks.