Prevent issues with conditionally revealed content when content id includes CSS syntax characters
#2370
Conversation
|
Needs to be merged after #2255 once the target branch has automagically updated. |
b4b8541 to
8d9ce98
Compare
|
First attempt at a changelog entry:
Or, an alternative version which tries less hard to abstract technical details and language (which may be clearer for users who are more familiar with JavaScript, but is arguably harder to understand for everyone else):
I don't know if it's also worth mentioning that using the same @EoinShaughnessy any thoughts on the above? |
|
@36degrees Changelog entry is taking shape nicely! I'd lean towards using the first, simpler version, to help any users who may be less familiar with JS. (Plus, the final PR will mention
Depends, I guess. This sounds like a borderline thing between writing Design System-specific instructions and telling people how to use HTML. Maybe, by saying multiple uses could display the wrong element, we've already given enough caution.
First paragraph is about JS looking anywhere on the page. Second and third paragraphs are about reuse of ID. Is that ok? Maybe combine the second and third paragraphs, and reverse their order? Something like in the suggested revision below:
|
|
@36degrees Thanks for the talk-through! Had a go at slight rewording, let me know what you think!
Few questions:
|
|
Alternative wording, based on questions from previous comment:
|
|
@EoinShaughnessy that looks pretty good, but I really trip over the heading when reading it. I think because you naturally read 'condition reveals JavaScript' with 'reveals' being a verb, rather than 'conditional reveals Javascript' as a compound noun? What about something like 'Make sure radios or checkboxes that conditionally reveal other questions still work as expected'? |
|
@36degrees Good point! Yeah, your suggestion removes that ambiguity. Want me to add something like the below to the Changelog?
|
783aae5 to
6d0c227
Compare
When checkboxes with conditional reveals are initialised, we check that the element targeted by aria-controls exists:
```
if (!controls || !$module.querySelector('#' + controls)) {
return
}
```
Because the string passed to `querySelector` is evaluated as a CSS selector, this can fail if the ID contains characters that have a special meaning in CSS, such as `.` or `[]` – the ID would need to be escaped for it to be evaluated correctly.
Add a failing test to cover this case.
The tests fail with:
```
Checkboxes with conditional reveals › when JavaScript is available › does not error when ID of revealed content contains special characters
DOMException: Failed to execute 'querySelector' on 'Element': '#conditional-user.profile[contact-prefs]-2' is not a valid selector.
```
```
Radios with conditional reveals › when JavaScript is available › does not error when ID of revealed content contains special characters
DOMException: Failed to execute 'querySelector' on 'Element': '#conditional-user.profile[contact-prefs]-2' is not a valid selector.
```
Because the string passed to `querySelector` is evaluated as a CSS selector, this can fail if the ID contains characters that have a special meaning in CSS, such as `.` or `[]` – the ID would need to be escaped for it to be evaluated correctly. Avoid this by using `document.getElementById` instead, so the ID is no longer evaluated as a selector. The downside is that we can't constrain the scope to the $module. However, we don't think this should cause any issues as the ID _should_ be unique within the document. This fixes the failing tests introduced in the previous commit.
6d0c227 to
2dc30f7
Compare
id includes CSS syntax characters
961f8b8 to
122ad88
Compare
122ad88 to
45b4b93
Compare
45b4b93 to
c0a81d1
Compare
Because the string passed to
querySelectoris evaluated as a CSS selector, this can fail (throwing a JavaScript error) if the ID contains characters that have a special meaning in CSS, such as.or[]– the ID would need to be escaped for it to be evaluated correctly.Avoid this by using
document.getElementByIdinstead, so the ID is no longer evaluated as a selector.The downside is that we can't constrain the scope to the $module. However, we don't think this should cause any issues as the ID should be unique within the document.
Details of breaking change
Closes #1808