Fix conditional reveals failing if the ID of the revealed element contains special characters

[36degrees]

If the conditionally revealed element has an ID that contains characters that have a special meaning in CSS, such as [], the call to querySelector fails because as it's being evaluated as a selector the ID needs to be escaped:

Uncaught DOMException: Failed to execute 'querySelector' on 'Element': '#conditional-person-1[how-contacted]-2' is not a valid selector.
    at Radios.<anonymous> (http://localhost:3000/public/all.js:1978:31)
    at NodeList.forEach (<anonymous>)
    at nodeListForEach (http://localhost:3000/public/all.js:14:18)
    at Radios.init (http://localhost:3000/public/all.js:1973:3)
    at http://localhost:3000/public/all.js:2384:24
    at NodeList.forEach (<anonymous>)
    at nodeListForEach (http://localhost:3000/public/all.js:14:18)
    at Object.initAll (http://localhost:3000/public/all.js:2383:3)
    at http://localhost:3000/components/radios/with-conditional-items-and-square-brackets/preview:98:32

In some cases the selector may be valid but still incorrect, like #conditional-rejectionReasons[candidate-actions] which would select any element with ID conditional-rejectionReasons, also having the attribute candidate-actions.

We can avoid all of this by using document.getElementById instead, avoiding the ID being evaluated as a selector. The downside is that we can't constrain the scope to the $module. However, I don't think this should cause any issues (especially as the ID should be unique) and this avoids alternatives like using CSS.escape to escape the ID, which we'd then need to polyfill.

Fixes #1808

[36degrees]

Raised as a draft as I'm worried that my Friday afternoon brain is missing a reason why changing the scope from $module to the document would be an issue…

[36degrees]

If we're happy with this approach it'd probably make sense to apply the same change to the character count, which should fix #1809:

govuk-frontend/src/govuk/components/character-count/character-count.js

Line 9 in 4a9d392

this.$countMessage = $module.querySelector('[id=' + this.$textarea.id + '-info]')

[hannalaakso]

Raised as a draft as I'm worried that my Friday afternoon brain is missing a reason why changing the scope from $module to the document would be an issue…

Thanks for digging into this! I admit that I need to dig into the original issue a bit more but these are my thoughts on swapping $module.querySelector for document.getElementById:

I think we need to be wary that some of the existing behaviour would change, even if we're talking about edge cases:

• Currently, if there are two separate radios components on the page and you give two items in each of the components the same ID, both conditional reveals still work (our Nunjucks macro docs for items.id don't actually say IDs need to be unique although it's obviously heavily implied).
• A third party plugin etc. could happen to be using the same ID in the page markup as our form element and our script would then grab third party element if it encountered it first.
• There could be duplicated IDs on a page because the app is complex and markup for a page is constructed from multiple sources 🤷

If we decide to make this change, we should probably have less trust in any elements that are queried with document.getElementById and do more checking in our script to make sure that it's the element is what we think it is. Our component wouldn't work correctly but at least we could catch a script error possibly resulting from trying to manipulate a DOM element that's the wrong one.

Also, querySelector is more flexible than getElementById. This sentence (from O'Reilly) explains it

The querySelector() method permits a parameter in the form of a CSS selector syntax. Basically, you can pass this method a CSS3 selector (e.g., #score>tbody>tr>td:nth-of-type(2)), which it will use to select a single element in the DOM.

but with the current implementation you wouldn't be able to use score>tbody>tr>td:nth-of-type(2) as the ID since it contains special characters so the script silently fails. Is there a CSS selector that could have previously worked with querySelector that now won't if we use getElementById? The answer is probably no.

If we make this change, we should also consider changing the code comment about scope to say that wherever IDs are concerned they are queried from the whole document (I don't think we have any other docs about scope but I might be having brain freeze).

[36degrees]

Let's play it safe and make this change in 4.0.