Javascript error when Toolbar is vertical and admin menu has unrouted links [#3137154]

Problem/Motivation

If you add a menu link to the Administration menu, and that menu link has an unrouted URI, when the toolbar is oriented vertically, there is a Javascript error which prevents the buttons for child menus from appearing.

Steps to reproduce:

Add menu link to administration menu at /admin/structure/menu/manage/admin/add
Set menu link title to anything.
Set link to any URL that is not routed in Drupal: (use https://www.google.com, for example)
Set parent link to --Administration
Save
If toolbar is horizontal, click the arrow to change it to vertical
Observe that buttons for child menus do not appear.
Observe that error is logged in browser console.

Uncaught Error: Syntax error, unrecognized expression: #toolbar-link-https://www-google-com
    at Function.se.error (jquery.min.js?v=3.4.1:2)
    at se.tokenize (jquery.min.js?v=3.4.1:2)
    at se.select (jquery.min.js?v=3.4.1:2)
    at Function.se [as find] (jquery.min.js?v=3.4.1:2)
    at k.fn.init.find (jquery.min.js?v=3.4.1:2)
    at MenuVisualView.js?v=8.8.5:19
    at Array.forEach (<anonymous>)
    at n.render (MenuVisualView.js?v=8.8.5:18)
    at _ (backbone.js:371)
    at m (backbone.js:356)

Proposed resolution

Issue appears to be in \Drupal\toolbar\Controller\ToolbarController::preRenderGetRenderedSubtrees(), where in this line:

$id = str_replace(['.', '<', '>'], ['-', '', ''], $url->isRouted() ? $url->getRouteName() : $url->getUri());

$url->getUri() contain unsafe characters for DOM IDs.

In toolbar_menu_navigation_links(), unrouted URIs are hashed like so:

$id = substr(Crypt::hashBase64($url->getUri()), 0, 16);

It seems to make sense to use that in the Controller pre-render method as well.

$id = str_replace(['.', '<', '>'], ['-', '', ''], $url->isRouted() ? $url->getRouteName() : substr(Crypt::hashBase64($url->getUri()), 0, 16));

Remaining tasks

Needs reviews and tests

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

(Major and critical issues should have a snippet that can be pulled into the release notes when a release is created that includes the fix)

Comment	File	Size	Author
#26	3137154-toolbar-unrouted-link-6.patch	1.04 KB	esod
#21	broken child links.png	42.09 KB	omkar.podey
#21	breaks.png	75.79 KB	omkar.podey
#21	doesnot break.png	73.43 KB	omkar.podey
#8	BeforePatch_Externalink.png	315.35 KB	priyanka.sahni
#8	BeforePatch_Internallink.png	343.42 KB	priyanka.sahni
#8	AfterPatch_Internallink.png	349.9 KB	priyanka.sahni
#8	AfterPatch_Externallink.png	391.38 KB	priyanka.sahni
#5	interdiff_4-5.txt	920 bytes	godotislate
#5	3137154-toolbar-unrouted-link-5.patch	3.03 KB	godotislate
#4	3137154-toolbar-unrouted-link-test-only-4.patch	1.99 KB	godotislate
#3	3137154-toolbar-unrouted-link-test-only-3.patch	1.8 KB	godotislate
#2	3137154-toolbar-unrouted-link-2.patch	1.03 KB	godotislate

Issue fork drupal-3137154

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3137154-javascript-error-when changes, plain diff MR !4910
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

15 May 2020 at 22:14

godotislate created an issue. See original summary.

Comment #2

godotislate

he/him

commented 15 May 2020 at 22:17

Issue tags:

+Needs tests

Status	File	Size
new	3137154-toolbar-unrouted-link-2.patch	1.03 KB

Here's a patch.

Comment #3

godotislate

he/him

commented 16 May 2020 at 04:27

Status	File	Size
new	3137154-toolbar-unrouted-link-test-only-3.patch	1.8 KB

Failing test-only patch.

Comment #4

godotislate

he/him

commented 16 May 2020 at 05:55

Status	File	Size
new	3137154-toolbar-unrouted-link-test-only-4.patch	1.99 KB

Test-only patch try 2.

Comment #5

godotislate

he/him

commented 16 May 2020 at 10:30

Status:

Active

» Needs review

Status	File	Size
new	3137154-toolbar-unrouted-link-5.patch	3.03 KB
new	interdiff_4-5.txt	920 bytes

Fix with test.

Comment #6

godotislate

he/him

commented 16 May 2020 at 15:01

Issue tags:

-Needs tests

Comment #7

priyanka.sahni commented 22 May 2020 at 05:27

Assigned:

Unassigned

» priyanka.sahni

Comment #8

priyanka.sahni commented 22 May 2020 at 10:02

Status:

Needs review

» Reviewed & tested by the community

Status	File	Size
new	AfterPatch_Externallink.png	391.38 KB
new	AfterPatch_Internallink.png	349.9 KB
new	BeforePatch_Internallink.png	343.42 KB
new	BeforePatch_Externalink.png	315.35 KB

I have tested by applying the patch#5.It was working fine and looks good to me.

Steps to test:

Add menu link to administration menu at /admin/structure/menu/manage/admin/add
Set menu link title to anything.
Set link to any URL that is not routed in Drupal: (use https://www.google.com, for example)
Set parent link to
Save
If toolbar is horizontal , click the arrow to change it to vertical
Observe that buttons for child menus gets appear.

Before_Patch_ExternalLink

Before_Patch_InternalLink

After_Patch_ExternalLink

After_Patch_InternalLink"

Comment #9

priyanka.sahni commented 22 May 2020 at 10:02

Assigned:

priyanka.sahni

» Unassigned

Comment #10

nod_

French

Lille

commented 29 May 2020 at 14:23

Status:

Reviewed & tested by the community

» Needs work

+++ b/core/modules/toolbar/src/Controller/ToolbarController.php
@@ -121,7 +122,7 @@ public static function preRenderGetRenderedSubtrees(array $data) {
+      $id = str_replace(['.', '<', '>'], ['-', '', ''], $url->isRouted() ? $url->getRouteName() : substr(Crypt::hashBase64($url->getUri()), 0, 16));

~~Using base64 and substring will not work, with this code http://www.google.com and http://www.google.com/somethingelse will get the same ID.~~

~~Try to look at Html::getId() instead.~~

Ok nevermind, spoke too soon

Comment #11

nod_

French

Lille

commented 29 May 2020 at 14:36

Status:

Needs work

» Reviewed & tested by the community

Patch is ok, it fixes what goes into drupalSettings to match what is in the DOM.

But the test should probably check that the generated ID matches with the ID in drupalSettings since that was what the issue was.

Comment #12

nod_

French

Lille

commented 29 May 2020 at 14:44

Status:

Reviewed & tested by the community

» Needs work

sorry for the emotional roller coaster.

Tests needs to be updated to check this more directly. We also need a comment that links what we're doing here to the toolbar_menu_navigation_links() since that is where it comes from. In case this gets updated it needs to be updated in 2 places otherwise it'll break again.

Comment #13

godotislate

he/him

commented 29 May 2020 at 17:48

Issue summary:

View changes

Comment #14

29 May 2020 at 17:48

Version:

9.1.x-dev

» 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Comment #15

29 May 2020 at 17:48

Version:

9.2.x-dev

» 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #16

29 May 2020 at 17:48

Version:

9.3.x-dev

» 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #17

29 May 2020 at 17:48

Version:

9.4.x-dev

» 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #18

29 May 2020 at 17:48

Version:

9.5.x-dev

» 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now 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.

Comment #19

29 May 2020 at 17:48

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.

Comment #20

smustgrave commented 18 July 2023 at 00:27

Status:

Needs work

» Postponed (maintainer needs more info)

Tried replicating following the steps in the IS on 10.2 but wasn't able to trigger the error.
Just to confirm
Created a link in the Admin menu
Title is My test
Link is https://www.google.com/
Toolbar is vertical
Click save
Go horizontal no issue

Create another link
Toolbar is horizontal
Click save
Go vertical no issue.

Comment #21

omkar.podey commented 7 August 2023 at 07:18

Issue summary:

View changes

Status	File	Size
new	doesnot break.png	73.43 KB
new	breaks.png	75.79 KB
new	broken child links.png	42.09 KB

What I think is that the steps are incorrect maybe. As @smustgrave i got a similar result initially with the structure being

But after i changed the menu links parent to --Administration for my test menu link(random link title).

It did break the child links in the vertical toolbar.

I think this was the intended bug to be solved, right?

Comment #22

omkar.podey commented 7 August 2023 at 07:25

Status:

Postponed (maintainer needs more info)

» Needs work

Comment #23

omkar.podey commented 7 August 2023 at 07:26

Issue summary:

View changes

Comment #24

29 September 2023 at 21:59

godotislate opened merge request !4910

Comment #25

godotislate

he/him

commented 29 September 2023 at 22:03

@omkar.podey: Yes, that's the issue. Thank you for the correction in the steps. It's been a couple years, and I'm off that project, but we saw the issue surface when we put a link to our pattern library in the admin menu for convenience, and QA caught that the menu broke in the vertical configuration.

Created an MR against 11.x-dev after tweaking the original patch to address feedback from #12. Not sure how to write a better test. The Javascript for the toolbar is more complicated than I can write a test for. It doesn't look like the original test captures the issue correctly anymore either. I've left the PR at draft to make the fix available against 11.x-dev for anyone who needs it. I'm willing to revisit the tests if anyone can provide some guidance on how to write them.

Comment #26

esod commented 9 July 2024 at 16:57

Status	File	Size
new	3137154-toolbar-unrouted-link-6.patch	1.04 KB

Reroll for Drupal 10.2.7. I don't have time to look at the test right now.

Comment #27

9 July 2024 at 16:57

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.