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

CommentFileSizeAuthor
#4 Activelinks_after.png94.93 KBahsannazir
#4 Activelins_before.png404.82 KBahsannazir

Issue fork drupal-3464340

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

herved created an issue. See original summary.

herved’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: -JavaScript +JavaScript
ahsannazir’s picture

Issue tags: -JavaScript +JavaScript
StatusFileSize
new404.82 KB
new94.93 KB

I have tested the issue, the errors in the console are fixed. Attaching Before and After

Before:

After:

smustgrave’s picture

Status: Needs review » Needs work

Not sure the JS test is failing as expected. Test-only feature showed green.

herved’s picture

#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

herved’s picture

Status: Needs work » Needs review

Moving back to needs review... The test fails correctly...
It doesn't look like this "Test-only" pipeline even starts/works

herved’s picture

Status: Needs review » Needs work

Oh 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?

herved’s picture

Status: Needs work » Needs review

The "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.

smustgrave’s picture

May have to be ran locally as the pipeline is showing as skipped

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative
1) Drupal\Tests\system\FunctionalJavascript\ActiveLinkTest::testQueryStringQuotes
Error: Failed to execute 'querySelectorAll' on 'Document': '[data-drupal-link-system-path="user/2"]:not([hreflang])[data-drupal-link-query='{"check_logged_in":"1","foo":"'"}'],[data-drupal-link-system-path="user/2"][hreflang="en"][data-drupal-link-query='{"check_logged_in":"1","foo":"'"}']' is not a valid selector.

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

longwave’s picture

Status: Reviewed & tested by the community » Needs review

Should we be using CSS.escape instead of a custom regex? https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_static

herved’s picture

#12, indeed we could.
According to my tests, CSS.escape doesn't seem strictly needed in this case, because JSON.stringify does 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.

longwave’s picture

Status: Needs review » Needs work
herved’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Never knew about CSS.escape but glad I do now!

nod_’s picture

Status: Reviewed & tested by the community » Needs work

question in MR

herved’s picture

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Needs work

we can add back the queryString variable, shoving everything in the string is not very nice to read

herved’s picture

Status: Needs work » Needs review

whoops, forgot to move back to review, thanks

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed.

  • nod_ committed 6f437449 on 10.4.x
    Issue #3464340 by herved, ahsannazir, smustgrave, longwave, nod_: JS...

  • nod_ committed 91cd1ab5 on 11.x
    Issue #3464340 by herved, ahsannazir, smustgrave, longwave, nod_: JS...
nod_’s picture

Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 91cd1ab5e49 to 11.x and 6f437449b19 to 10.4.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.