bootstrap

bootstrap copied to clipboard

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

Will fix #35565

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

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 😃

@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

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

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

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

WIll also probably fix https://github.com/twbs/bootstrap/issues/31099

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

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) 😃

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

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

Regards

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

Does it fit the currently supported browsers for Bootstrap?

@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)

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)

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

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)

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?

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

Merry Christmas to all of you!

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

Regards

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)

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

@GeoSot @XhmikosR Is this PR good to go?

Ping?

Sorry for the late reply, I've been pretty busy with real life.

I'll try to have a look in the next days.

Hello @GeoSot & @XhmikosR do you want to consider this? Should I try updating it?

Hello, we had discussed a bit. I can recall that the main thoughts were that :

• we have multiple places we are using the querySelector (so would be inconsistent in some cases)
• that in case we support this, we have to be ready for new problems in case of selectors like #2foo .bar or #2foo #2bar

So I really don't have a valid answer right now, except that I am already working on the background to make the decision and the change easier in future #36027

I think we get hit by this, and a fix would be welcome.

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

Hello, we had discussed a bit. I can recall that the main thoughts were that :

* we have multiple places we are using the `querySelector` (so would be inconsistent in some cases)

* that in case we support this, we have to be ready for new problems in case of selectors like `#2foo .bar` or `#2foo #2bar`

So I really don't have a valid answer right now, except that I am already working on the background to make the decision and the change easier in future #36027

Hello @GeoSot ,

Ok, I had another proposal, to use the following:

if (selector !== null && window.CSS && window.CSS.escape) {
  selector = selector.replaceAll(/#([^\s#]+)/g, function(full,id){return "#" + CSS.escape(id)})
}

A few examples of results:

"#section-1.1 .bar".replaceAll(/#([^\s#]+)/g, function(_full,id){return "#" + CSS.escape(id)})
"#section-1\\.1 .bar" 

"#foo .bar".replaceAll(/#([^\s#]+)/g, function(_full,id){return "#" + CSS.escape(id)})
=> "#foo .bar"

It sounds to work well... but as you mentioned, for some weird reason, it would not work for IDs starting with numbers.

So, for some weird reason, CSS.escape('2f') => "32 f"

So, in your example:

"#2foo .bar".replaceAll(/#([^\s#]+)/g, function(_full,id){return "#" + CSS.escape(id)})
=> "#\\32 foo .bar" => BAD

From my testing, this is only an issue with identifiers starting with numbers. I don't have a clear understanding of why, since the specification is not talking about this: https://drafts.csswg.org/cssom/#the-css.escape()-method

What I thus propose is to basically ignore IDs starting with a number and thus to use:

if (selector !== null && window.CSS && window.CSS.escape) {
  selector = selector.replaceAll(/#([^0-9][^\s#]+)/g, function(full,id){return "#" + CSS.escape(id)})
}

Which gives some more correct results than the current implementation:

"#2foo .bar > #this-is-my-solution-1.0".replaceAll(/#([^0-9][^\s#]+)/g, function(_full,id){return "#" + CSS.escape(id)})
=> "#2foo .bar > #this-is-my-solution-1\\.0" 

The only downside is that it does not work completely well with IDs starting with numbers, but for everything else, it works.

What do you think?

@GeoSot huh... according to https://mothereff.in/css-escapes and my tests, the CSS.escape is actually working!

So, I am gonna submit a new patch

@GeoSot @XhmikosR Ok, I am now happy with this new version, it now works by escaping all IDs, whatever their position in the selector

Works with complex selectors such as ".foo 2bar", added a UTest to ensure it works well: https://github.com/twbs/bootstrap/pull/35566/files#diff-232a393b53d525c6749638629be8a4288ec1929b51c8144fe92a41c25c885bc3R132

We need to double check if our currently supported browsers have this feature.

Apart from that:

1. as long as the change covers all cases
2. everything is covered by tests

we could try landing it.

@XhmikosR I documented what is supported: https://caniuse.com/mdn-api_css_escape

Since a feature tested, older systems will behave as they do now