Problem/Motivation
On paths with specific chars in the query string, Drupal.behaviors.activeLinks causes JS errors:
Steps to reproduce
- Install Drupal,
- Log in as admin,
- (optional) Disable JS/CSS aggregation
- Visit for example: /?destination=/foo%3Fbar%3D%27
> We get errors in the console:
Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Document': '[data-drupal-link-system-path="node"]:not([hreflang])[data-drupal-link-query='{"destination":"/foo?bar='"}'],[data-drupal-link-system-path="<front>"]:not([hreflang])[data-drupal-link-query='{"destination":"/foo?bar='"}'],[data-drupal-link-system-path="node"][hreflang="en"][data-drupal-link-query='{"destination":"/foo?bar='"}'],[data-drupal-link-system-path="<front>"][hreflang="en"][data-drupal-link-query='{"destination":"/foo?bar='"}']' is not a valid selector.
at Object.attach (http://127.0.0.1:8888/core/misc/active-link.js?v=11.0-dev:54:35)
at http://127.0.0.1:8888/core/misc/drupal.js?v=11.0-dev:166:24
at Array.forEach (<anonymous>)
at Drupal.attachBehaviors (http://127.0.0.1:8888/core/misc/drupal.js?v=11.0-dev:162:34)
at http://127.0.0.1:8888/core/modules/big_pipe/js/big_pipe.js?v=11.0-dev:153:10
at http://127.0.0.1:8888/core/modules/big_pipe/js/big_pipe.js?v=11.0-dev:184:3
Here is the isolated code from the case above:
const selector = `[data-drupal-link-system-path="node"]:not([hreflang])[data-drupal-link-query='{"destination":"/foo?bar='"}']`;
document.querySelectorAll(selector);
Proposed resolution
It seems Drupal.behaviors.activeLinks builds selectors from the current query but doesn't escape ' chars properly before passing them to querySelectorAll.
Either escape those chars or do the query strings check later, after querySelector?
Remaining tasks
?
User interface changes
None
Introduced terminology
None
API changes
None
Data model changes
None
Release notes snippet
None
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | Activelinks_after.png | 94.93 KB | ahsannazir |
| #4 | Activelins_before.png | 404.82 KB | ahsannazir |
Issue fork drupal-3464340
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:
- 3464340-js-errors
changes, plain diff MR !8963
Comments
Comment #3
herved commentedComment #4
ahsannazir commentedI have tested the issue, the errors in the console are fixed. Attaching Before and After
Before:

After:

Comment #5
smustgrave commentedNot sure the JS test is failing as expected. Test-only feature showed green.
Comment #6
herved commented#5, it is, but I force pushed then kind of screwed things up trying to manually run things on gitlab I think...
How can we relaunch it? Thanks
Comment #7
herved commentedMoving back to needs review... The test fails correctly...
It doesn't look like this "Test-only" pipeline even starts/works
Comment #8
herved commentedOh I just realized now which job you meant, I focused on the wrong one.
https://git.drupalcode.org/issue/drupal-3464340/-/jobs/2320491
This should fail so something is odd indeed, back to needs work.
It says the test was skipped, is this pipeline running FunctionalJavaScript tests?
Comment #9
herved commentedThe "Test-only changes" pipeline was broken, should be fixed now with #3467080: Test-only job cannot be run due to wrong dependency.
I rebased the branch, it should work as expected now but I don't have the rights to run it it seems.
Comment #10
smustgrave commentedMay have to be ran locally as the pipeline is showing as skipped
Comment #11
smustgrave commentedRan locally and believe this is the correct error to receive so would say test coverage is there.
Manually testing the issue does appear resolved
Believe this one is good to go.
Comment #12
longwaveShould we be using
CSS.escapeinstead of a custom regex? https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_staticComment #13
herved commented#12, indeed we could.
According to my tests,
CSS.escapedoesn't seem strictly needed in this case, becauseJSON.stringifydoes seem to escape already a lot, but I may not have tested all possibilities.It does feel cleaner though and seems to handle really everything we throw at it so I'd go for it.
I also did it for the path, as it won't hurt but let me know.
Comment #14
longwaveComment #15
herved commentedComment #16
smustgrave commentedNever knew about CSS.escape but glad I do now!
Comment #17
nod_question in MR
Comment #18
herved commentedComment #19
nod_we can add back the queryString variable, shoving everything in the string is not very nice to read
Comment #20
herved commentedwhoops, forgot to move back to review, thanks
Comment #21
smustgrave commentedFeedback appears to be addressed.
Comment #25
nod_Committed and pushed 91cd1ab5e49 to 11.x and 6f437449b19 to 10.4.x. Thanks!