Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent conditionally revealed questions getting out of sync when multiple sets of radios and checkboxes contain inputs with the same name #2255

Merged
merged 6 commits into from
Oct 14, 2021

Conversation

36degrees
Copy link
Contributor

@36degrees 36degrees commented Jun 18, 2021

We have an open issue to provide examples of grouping radios and checkboxes under headings.

Whilst we don’t provide any guidance on this at the minute, we’ve previously suggested that users can achieve this by making multiple calls to the govukCheckboxes macro and grouping them together inside a single call to govukFieldset. However, this means that each ‘set’ of checkboxes is inside its own module, which can cause issues with things like conditional reveals.

Add an example of this to the review app, so that we can test that everything works as expected, and fix a few issues identified that can leave conditional reveals 'out of sync' with their controls in this context.

See individual commits for details.

Details of breaking change

Although technically this is a breaking change, it's the 'same' breaking change as in #2370. So I suggest we just include this in the list of 'Fixes'.

  • which users are affected: anyone using conditional reveals
  • the change that’s been made: when looking for the revealed content associated with a checkbox or radio, the JavaScript will now look for it across the entire page rather than only looking within that set of radios or checkboxes
  • changes users will have to make: ensure that all IDs on a page are unique, and that checkboxes / radios with conditionally revealed content are using the right ID for their content
  • impact of users not making those changes: if a page has multiple elements with the same ID (which is technically invalid HTML) radios or checkboxes may now cause the wrong element on the page to appear / disappear when selecting a checkbox or radio

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2255 June 18, 2021 13:51 Inactive
@36degrees
Copy link
Contributor Author

Should we include this in 3.13.0 or hold this and merge after the release has gone out?

We have 'frozen' the release, but arguably these are things that could have been fixed as part of #2151. Equally, these are existing bugs that are unlikely to come up in most contexts.

@lfdebrux
Copy link
Member

Should we include this in 3.13.0 or hold this and merge after the release has gone out?

I vote for holding!

@lfdebrux
Copy link
Member

Hmm, testing the new examples with VoiceOver in Safari I found that the h2s for the groups weren't announced?

var $target = document.querySelector('#' + $input.getAttribute('aria-controls'))
var $target = document.getElementById($input.getAttribute('aria-controls'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌 🙌 🙌

@36degrees 36degrees added this to the [NEXT] milestone Jun 22, 2021
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2255 June 22, 2021 16:53 Inactive
@lfdebrux
Copy link
Member

I think it might be good to have tests for this behaviour to catch possible future regressions; something like 32f116d for example?

@36degrees 36degrees self-assigned this Jul 8, 2021
@36degrees 36degrees modified the milestones: [NEXT], v4.0.0 Jul 12, 2021
@36degrees
Copy link
Contributor Author

I've realised that the (necessary) change from this.$module.querySelector to document.getElementById in bf0041a is the same change we've previously flagged as a breaking change, so we should treat this as breaking as well.

Attached to the 4.0 milestone.

@lfdebrux lfdebrux force-pushed the radio-checkboxes-tweaks branch from 32f116d to 7c3dd77 Compare July 30, 2021 11:38
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2255 July 30, 2021 11:38 Inactive
@lfdebrux lfdebrux force-pushed the radio-checkboxes-tweaks branch from 7c3dd77 to 6ad421d Compare July 30, 2021 11:47
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2255 July 30, 2021 11:48 Inactive
@lfdebrux
Copy link
Member

@36degrees I've added more tests in 6ad421d, I think covering everything this PR fixes. I have run these tests before the fixes and they fail as expected, so I think they should make good regression tests. Feel free to leave comments/suggestions on them!

Copy link
Contributor

@vanitabarrett vanitabarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 Think it just needs a changelog entry?

@36degrees 36degrees changed the title Fix bugs with radios and checkboxes where multiple modules have the same name Fix bugs with radios and checkboxes where multiple modules contain inputs with the same name Sep 27, 2021
@36degrees 36degrees changed the title Fix bugs with radios and checkboxes where multiple modules contain inputs with the same name Fix conditionally revealed questions getting out of sync when multiple sets of radios and checkboxes contain inputs with the same name Sep 27, 2021
@36degrees 36degrees force-pushed the radio-checkboxes-tweaks branch from 6ad421d to b4b8541 Compare September 27, 2021 14:41
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2255 September 27, 2021 14:42 Inactive
36degrees and others added 6 commits October 5, 2021 15:14
We have an open issue to provide examples of grouping radios and checkboxes under headings (#1079).

Whilst we don’t provide any guidance on this at the minute, we’ve previously suggested that users can achieve this by making multiple calls to the `govukCheckboxes` macro and grouping them together inside a single call to `govukFieldset`. However, this means that each ‘set’ of checkboxes is inside its own module, which can cause issues with things like conditional reveals.

Add an example of this to the review app, so that we can test that everything works as expected.
Rather than calling `this.syncAllConditionalReveals`, which only syncs conditional reveals on checkboxes within the same module, sync every conditional reveal with the same name and form owner during the the existing loop.

When syncing each conditional reveal, we also need to look for the targeted element across the entire document, as we might be syncing a conditional reveal outside of the current module.

We can also simplify the lookup by using document.getElementById rather than using querySelector which requires us to manually prepend the hash to get an ID selector.
This matches a change previously made to the checkboxes component in 7e746b4.

If you use two separate calls to the macro with the same name, but one of them does not contain any radios with conditional reaveals, then checking a radio in that list would not hide a conditional reveal in the other list, as the module has not been initialised so no `eventListener` has been set up.

Always initialise the javascript for every set of radios which solves this issue.
Use document.getElementById rather than document.querySelector which requires us to manually prepend the hash to get an ID selector.
Adds tests for JavaScript behaviours we want to work across separate
groups of radios or checkboxes with the same `name`, specifically:

- conditional reveals
- 'none of the above' checkbox
@36degrees 36degrees force-pushed the radio-checkboxes-tweaks branch from b4b8541 to 8d9ce98 Compare October 5, 2021 14:17
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2255 October 5, 2021 14:17 Inactive
@36degrees
Copy link
Contributor Author

For some reason the GitHub Actions have not been triggered on the latest commit. Closing and re-opening to try and force them to run…

@36degrees 36degrees closed this Oct 12, 2021
@36degrees 36degrees reopened this Oct 12, 2021
@36degrees 36degrees merged commit b4d0e1a into main Oct 14, 2021
@36degrees 36degrees deleted the radio-checkboxes-tweaks branch October 14, 2021 10:40
@36degrees 36degrees changed the title Fix conditionally revealed questions getting out of sync when multiple sets of radios and checkboxes contain inputs with the same name Prevent conditionally revealed questions getting out of sync when multiple sets of radios and checkboxes contain inputs with the same name Oct 14, 2021
@vanitabarrett vanitabarrett mentioned this pull request Dec 15, 2021
andymantell added a commit to surevine/govuk-react-jsx that referenced this pull request Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants