Fix Scrollspy to work with all valid HTML5 element ids #34412
Conversation
Add function to specifically retrieve target element ID, instead of a generic CSS selector. Especially useful for Scrollspy, nav-tabs, list-groups, offcanvas, modals, etc. where the target is guaranteed to be only a single element (as opposed to multiple elements sharing the same CSS class).
| @@ -8,7 +8,7 @@ | |||
| import { | |||
| defineJQueryPlugin, | |||
| getElement, | |||
| getSelectorFromElement, | |||
| getTargetIdFromElement, | |||
There was a problem hiding this comment.
Thank you @chrisyeh96 for you contribution 😃
Instead of introducing another function to just make the job, you could probably make a minor refactor to
getElementFromSelector to check if string starts with # and use the getElementById.
After this you can use it inside getSelectorFromElement as they use the exactly same code 😉
There was a problem hiding this comment.
Unfortunately, I don't think that your suggestion will work with edge cases. For example #someID > div is a valid CSS Selector that starts with a # but cannot be passed directly to document.getElementById().
I wrote the new function getTargetIdFromElement() specifically to be used in cases such as Scrollspy where the Bootstrap documentation explicitly says that the href or data-bs-target attributes of the element have the format #someValidHTML5Id.
The more general getElementFromSelector() should still be allowed for generic CSS selectors such as the example above (#someID > div).
There was a problem hiding this comment.
Unfortunately, I don't think that your suggestion will work with edge cases. For example #someID > div is a valid CSS Selector that starts with a # but cannot be passed directly to document.getElementById().
Maybe this could do the job. Right?
reg = new RegExp(/^#[\w\-]+$/gm)
reg.test(givenId)There was a problem hiding this comment.
That doesn't work either. As I mentioned in my original issue #34381, any string of Unicode characters without whitespace is considered a valid HTML5 ID. In particular, numbers, emoji, and non-Latin characters are all valid. These are not matched by your suggested regex.
My pitch for a separate getTargetIdFromElement() method
In my mind, there are 2 distinct cases that Bootstrap should support:
Case 1: Arbitrary CSS Selectors
The Bootstrap docs for components such as Collapse and Offcanvas specifically mention supporting arbitrary CSS selectors. These should use the existing getSelectorFromElement() / getElementFromSelector() methods, which call document.querySelector() behind the scenes. By definition of CSS selectors (see official spec below), these components would NOT support IDs that start with digits.
Case 2: ID-specific Selectors
The Bootstrap docs for components such as Scrollspy and Carousel explicitly mention requiring matching an HTML5 ID (see official spec below). These should use the new getTargetIdFromElement() method which only matches HTML5 IDs instead of arbitrary CSS selectors. Thus, these components would be able to support IDs that start with digits.
Official Specs for HTML5 IDs and CSS Selectors
HTML5 ID: The official spec states
When specified on HTML elements, the id attribute value must be unique amongst all the IDs in the element's tree and must contain at least one character. The value must not contain any ASCII whitespace. [...] There are no other restrictions on what form an ID can take; in particular, IDs can consist of just digits, start with a digit, start with an underscore, consist of just punctuation, etc.
The method document.getElementById() supports all valid HTML5 IDs.
CSS Selectors: The official spec states
An ID selector consists of a “number sign” (U+0023, #) immediately followed by the ID value, which must be a CSS identifier
where a CSS identifier is defined (official spec) as follows:
In CSS, identifiers (including element names, classes, and IDs in selectors) can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 and higher, plus the hyphen (-) and the underscore (_); they cannot start with a digit, two hyphens, or a hyphen followed by a digit.
The method document.querySelector() only supports valid CSS selectors. For example, document.querySelector("#3rd") will raise an exception.
| // a valid HTML5 id is any string without whitespace | ||
| const match = selector.match(/#(\S+)/) | ||
| const id = match ? match[1] : null | ||
| return document.getElementById(id) ? id : null |
There was a problem hiding this comment.
Can't this be tweaked a bit further? Like, there's no need to even call getElementById if id is null.
There was a problem hiding this comment.
True, but getElementById() already performs a check against null anyway.
As-is
getElementById() always performs a single check on whether id is null.
Your proposed tweak
If id is null, then we perform a single check on whether it's null. This is probably slightly faster than the function call to getElementById().
If id is not null, then we still perform a check ourselves, but then getElementById() also performs a check. In other words, there are two branching statements.
In other words, your proposal may speed up the case when id is null, but it's slower when id is not null. I'm happy with either option. Feel free to change it as you see fit.
Fixes #34381 by using
document.getElementById()instead ofdocument.querySelector()when testing the validity of an element ID.Whereas
document.querySelector()only allows valid CSS selectors and requires ID-based selectors to start with a letter,document.getElementById()handles all valid HTML5 element IDs.