Properly escape IDs in getSelector() to handle weird IDs

[pierresouchay]

IDs need to be escaped before document.querySelector() is called

Will fix #35565

Closes #34412
closes #36939
Fixes #34381
Fixes #31099

[GeoSot]

Nice job @pierresouchay . I 've done some testing it and seems it brings another 'feature', enabling use ids starting with number.

Please add some tests to support your MR 😃

[pierresouchay]

@GeoSot can you approve the worklow, so I can test it does not break anything?
I'll add the test, for now I just tested in blind mode

[GeoSot]

You can try npm run js-debug and npm run js-lint before pushing, too 😉

[pierresouchay]

@GeoSot Thanks, I'll try to add the test this evening. Do you have suggestion on what to test? I think I'll simply change a collapse test to target 0/my/id instead of test2 id...

[pierresouchay]

@GeoSot all done, I applied linter, added test with problematic ID and rebased

[pierresouchay]

WIll also probably fix #31099

Update: it does fix #31099 (added unit test to prove it)

[GeoSot]

You probably need to add a test on index.spec.js => getSelector and to handle the same thing in the getElement function (plus proper test) 😃

[pierresouchay]

You probably need to add a test on index.spec.js => getSelector and to handle the same thing in the getElement function (plus proper test) 😃

@GeoSot Done in last commit

[pierresouchay]

@GeoSot or @XhmikosR can you launch the tests, I think the PR is ready.

Regards

[pierresouchay]

@GeoSot Current support is not bad https://caniuse.com/mdn-api_css_escape

Does it fit the currently supported browsers for Bootstrap?

[pierresouchay]

@GeoSot I could do another minor change (if you want me to):

  if (selector !== null && selector.startsWith('#')) {
    // document.querySelector needs escaping to handle IDs (html5+) containing for instance /
    selector = '#' + CSS.escape(selector.slice(1))
  }

to be changed to:

  if (selector !== null && selector.startsWith('#') && window.CSS && window.CSS.escape) {
    // document.querySelector needs escaping to handle IDs (html5+) containing for instance /
    selector = '#' + CSS.escape(selector.slice(1))
  }

=> so It would work as it does now for very old browsers not supporting CSS.escape (so, would still break on non HTML4 IDs, but would work for HTML4 IDs)

[pierresouchay]

Hello @XhmikosR,

I added the tests (as seen in the labels of the PR), do you need more?

(BTW: I need someone to approve the tests in order to run)

[GeoSot]

Discussing this with @XhmikosR , there are about 7 usages of querySelector inside the code base.

Indeed, your proposal is valid, but it doesn't cover all possibilities.

I can see two possible solutions here.

1. Keep it as is now (the simply and ugly)
2. Make a more consistent approach, where all query selections have to pass through the specific check, and at least cover one single selector id

Have in mind that there might be cases like .randomClass #4fooID

[pierresouchay]

IMHO, this change fixes bugs, not all of them, ans is straightforward (while we might argue on my comment about detecting the CSS.escape() function)
The escaping of other places with non-leading # is an issue I agree, but it probably far more complex and requires more refactoring. Unfortunately, this would requires some clever parsing because the # is not supported as it is with CSS escape as it requires more context to know if we are talking about ids of "regular" selectors (I mean html 4 compliant ones).
Still I think the / is more and more an issue as it makes sense to have IDs compliant with hash based-routing/history in single web pages (that's why I found the issue in the first place)

[GeoSot]

Personally, I am ok with this check
(selector && selector.startsWith('#') && window.CSS && window.CSS.escape)

@XhmikosR , please tell us you opinion. shall we go with the MVP or not?

[pierresouchay]

@GeoSot & @XhmikosR Done, updated the test to detected CSS.escape() in latest patch.

Merry Christmas to all of you!

[pierresouchay]

@XhmikosR & @GeoSot hello,
Hope you are all doing well. Did you decide something about this PR?

Regards

[GeoSot]

I 'll approve it as it solves the 3 issues, keeping in mind that it is not an end up solution (the other 7 occurrences have to be changed in the future)

[pierresouchay]

Hello @XhmikosR & @GeoSot

What should I do to get this MR being merged (I know, I have to solve conflicts first)?

Do you want to rewrite all places where a selector is used?

Regards

[pierresouchay]

@GeoSot Thank you, we are close \o/

[GeoSot]

No, even more than close. We are done ;)

Thank you a lot for your patience and your time

[larseggert]

I still see error like this with 5.2.3:

Uncaught DOMException: Element.querySelector: '#section-1.1' is not a valid selector

[XhmikosR]

@pierresouchay you should make a separate issue with all the info there.

[larseggert]

@XhmikosR adding this to a test illustrates the bug:

diff --git a/js/tests/unit/scrollspy.spec.js b/js/tests/unit/scrollspy.spec.js
index c7951e6..704b52a 100644
--- a/js/tests/unit/scrollspy.spec.js
+++ b/js/tests/unit/scrollspy.spec.js
@@ -341,11 +341,13 @@ describe('ScrollSpy', () => {
           '  <ul class="nav">',
           '    <li class="nav-item"><a class="nav-link" id="a-1" href="#div-1">div 1</a></li>',
           '    <li class="nav-item"><a class="nav-link" id="a-2" href="#div-2">div 2</a></li>',
+          '    <li class="nav-item"><a class="nav-link" id="a-2.1" href="#div-2.1">div 2</a></li>',
           '  </ul>',
           '</nav>',
           '<div class="content" style="overflow: auto; height: 50px">',
           '  <div id="div-1" style="height: 100px; padding: 0; margin: 0">div 1</div>',
           '  <div id="div-2" style="height: 200px; padding: 0; margin: 0">div 2</div>',
+          '  <div id="div-2.1" style="height: 200px; padding: 0; margin: 0">div 2.1</div>',
           '</div>'
         ].join('')

Error:

Chrome Headless 109.0.5414.87 (Mac OS 10.15.7) ScrollSpy constructor should add the active class to the correct element FAILED
	SyntaxError: Failed to execute 'querySelector' on 'Element': '#div-2.1' is not a valid selector.
	error properties: Object({ code: 12, INDEX_SIZE_ERR: 1, DOMSTRING_SIZE_ERR: 2, HIERARCHY_REQUEST_ERR: 3, WRONG_DOCUMENT_ERR: 4, INVALID_CHARACTER_ERR: 5, NO_DATA_ALLOWED_ERR: 6, NO_MODIFICATION_ALLOWED_ERR: 7, NOT_FOUND_ERR: 8, NOT_SUPPORTED_ERR: 9, INUSE_ATTRIBUTE_ERR: 10, INVALID_STATE_ERR: 11, SYNTAX_ERR: 12, INVALID_MODIFICATION_ERR: 13, NAMESPACE_ERR: 14, INVALID_ACCESS_ERR: 15, VALIDATION_ERR: 16, TYPE_MISMATCH_ERR: 17, SECURITY_ERR: 18, NETWORK_ERR: 19, ABORT_ERR: 20, URL_MISMATCH_ERR: 21, QUOTA_EXCEEDED_ERR: 22, TIMEOUT_ERR: 23, INVALID_NODE_TYPE_ERR: 24, DATA_CLONE_ERR: 25 })
	    at Object.findOne (js/src/dom/selector-engine.js:41:44 <- js/tests/unit/scrollspy.spec.js:9260:46)
	    at ScrollSpy._initializeTargetsAndObservables (js/src/scrollspy.js:211:48 <- js/tests/unit/scrollspy.spec.js:15411:76)
	    at ScrollSpy.refresh (js/src/scrollspy.js:91:10 <- js/tests/unit/scrollspy.spec.js:15227:12)
	    at new ScrollSpy (js/src/scrollspy.js:73:10 <- js/tests/unit/scrollspy.spec.js:15207:12)
	    at js/tests/unit/scrollspy.spec.js:355:27 <- js/tests/unit/scrollspy.spec.js:15743:29
	    at new Promise (<anonymous>)
	    at UserContext.<anonymous> (js/tests/unit/scrollspy.spec.js:338:14 <- js/tests/unit/scrollspy.spec.js:15740:16)
	    at <Jasmine>

[julien-deramond]

@larseggert Could you please create a separate issue with this information and the problem you have? Otherwise, it is very difficult for us to track since this PR is already merged.

[larseggert]

See #37858