Users can now conditionally reveal content on pages with multiple grouped radios #1497

NickColley · 2019-07-16T11:10:53Z

This pull request is a continuation of the work that @colinrotherham and @frankieroberto have proposed: #1297

It is similar to Frankie's original suggestion but keeps the scope as small as possible to avoid running JavaScript unnecessarily.

Fixes #1079
Closes #1297

NickColley · 2019-07-16T11:12:25Z

src/govuk/components/radios/radios.js

  nodeListForEach($allInputs, function ($input) {
-    this.setAttributes($input)
+    // In radios, only radios with the same name will affect each other.


Can we have a test for this? 🤔

Because all this will do is sync the conditional reveals with the checked state of their inputs, we'd have to do something really weird, like having another set of radios with a conditional reveal that's 'out of sync' with its radios, and asserting that it doesn't sync them back up again.

It's such a synthetic scenario I'm not sure it makes much sense as a test.

NickColley · 2019-07-16T11:41:23Z

src/govuk/components/radios/radios.js

@@ -40,18 +39,27 @@ Radios.prototype.setAttributes = function ($input) {
  var inputIsChecked = $input.checked
  $input.setAttribute('aria-expanded', inputIsChecked)

-  var $content = this.$module.querySelector('#' + $input.getAttribute('aria-controls'))
+  var $content = document.querySelector('#' + $input.getAttribute('aria-controls'))


Just realised I've confused myself.

This line is the one that's important that it isnt global, not the others.

#1156

Going to have to figure this out...

I think, we can't assume that 'aria-controls' on a radio means that the user has only conditional reveals.

In this case that we fixed, they put their own aria-controls on the radio for something unrelated.

I think we'll need to do something at the initialisation stage to avoid this...

We either need to have a way of querying the $module in which the given $input exists, or query the whole document, as this.$module will only search within the scope of the module of the clicked radio.

In terms of checking for aria-controls on something unrelated, yesterday you suggested checking for govuk-radios__conditional in $content.classList, which sounds like a sane defence?

If we do that, then scoping this to the document seems OK to me as we'll always just be syncing the --hidden class with the checked state of the input?

Yeah I forgot about the idea of looking for that class, that'll be the ticket thanks Ollie.

36degrees · 2019-07-16T13:18:34Z

src/govuk/components/radios/radios.js

+  //
+  // We also only want radios which have aria-controls, as they support conditional reveals.
+  var $scope = ($clickedInput.form || document)
+  var $allInputs = $scope.querySelectorAll('input[type="radio"][aria-controls]')


This is probably something of an edge-case but this won't query any inputs that are outside of the form but associated with it using form="form-id" as they won't be children of the form.

I'd suggest instead checking the form attribute of both within the forEach, i.e. if ($input.form === $clicked.form)

src/govuk/components/radios/radios.js

NickColley · 2019-07-16T13:44:47Z

Blocking this on the release notes being merged, so I can add a CHANGELOG entry...

NickColley · 2019-07-17T14:18:51Z

I wasn't up to date with my branch on my work computer and force pushed over some of my changes, so I'll sort this out when I get home...

When multiple groups of radios with the same `name` appear on a page, selecting any radio button may cause radio buttons in other groups to become deselected. However, because we only previously triggered setAttributes on radio buttons within the same $module, the conditional reveals associated with the now-unchecked radios would still be visible. By triggering setAttributes on all radios in the document, we can avoid this. This should be safe as all we're doing is syncing the visibility of the conditional reveal with its associated input.

This stops this loop for running unnecessarily, for example for clicks in an open conditional reveal.

We did not need the radios check as the querySelector already had `[type="radio"]`. We can also remove the aria-controls check by movin it into the querySelector as `[aria-controls]`.

We do not need to run this code for other inputs as their state will not have changed.

By doing this alongside checking the name we only run setAttributes on the radio inputs that could change.

Since sometimes users set aria-controls unrelated to conditional reveals we need to be extra careful here. See #1156 for more context.

NickColley · 2019-07-17T14:33:37Z

@36degrees suggested that I use the GitHub info log in this pull request to target the commit before I forced pushed which worked fine...

hannalaakso

LGTM 👍

hannalaakso · 2019-07-23T08:46:32Z

src/govuk/components/radios/radios.js

@@ -37,21 +36,35 @@ Radios.prototype.init = function () {
 }

 Radios.prototype.setAttributes = function ($input) {
-  var inputIsChecked = $input.checked
-  $input.setAttribute('aria-expanded', inputIsChecked)
+  var $content = document.querySelector('#' + $input.getAttribute('aria-controls'))


Non-blocking: It could be worth leaving a comment here to say that we use document as the scope to cater for the edge case when other radios with conditionals are part of the same group. It might not be immediately clear that there could be other radios that are in fact related even though they're outside $module.

I originally put a comment in but @36degrees thought that if we were doing this from scratch that we probably wouldnt have commented it. The case you're talking about is described in the test suite, so maybe that's enough? What do you think?

I think if someone changed this the tests would fail so I'm going to go with what Ollie and I discussed originally so can get this merged 🐎

edwardhorsford · 2019-07-23T09:37:52Z

@NickColley #1079 is about being able to group related radios together - is that made possible from this PR?

NickColley · 2019-07-23T09:40:22Z

@edwardhorsford this allows you to do the conditional reveals when you have groups of radios with the same name, so I think so! :)

This'll go out very soon in 3.0.0.

If you need a different use case feel free to open an new issue for us to consider.

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1497 July 16, 2019 11:11 Inactive

NickColley commented Jul 16, 2019

View reviewed changes

NickColley force-pushed the radio-trigger-setattributes branch from dae265e to 037c1f6 Compare July 16, 2019 11:18

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1497 July 16, 2019 11:18 Inactive

NickColley commented Jul 16, 2019

View reviewed changes

36degrees reviewed Jul 16, 2019

View reviewed changes

src/govuk/components/radios/radios.js Outdated Show resolved Hide resolved

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1497 July 16, 2019 13:23 Inactive

NickColley force-pushed the radio-trigger-setattributes branch from 65e843f to 42ab630 Compare July 16, 2019 13:40

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1497 July 16, 2019 13:40 Inactive

NickColley marked this pull request as ready for review July 16, 2019 13:42

NickColley added the blocked label Jul 16, 2019

NickColley removed the blocked label Jul 17, 2019

NickColley force-pushed the radio-trigger-setattributes branch from 42ab630 to 6eb6c86 Compare July 17, 2019 14:08

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1497 July 17, 2019 14:08 Inactive

NickColley added this to the v3.0.0 milestone Jul 17, 2019

NickColley changed the title ~~Trigger setAttributes on radio inputs with shared form owner~~ Support multiple grouped radios with conditional reveals Jul 17, 2019

NickColley force-pushed the radio-trigger-setattributes branch from 6eb6c86 to e7f5257 Compare July 17, 2019 14:13

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1497 July 17, 2019 14:13 Inactive

NickColley changed the title ~~Support multiple grouped radios with conditional reveals~~ Users can now conditionally reveal content on pages with multiple grouped radios Jul 17, 2019

NickColley force-pushed the radio-trigger-setattributes branch from e7f5257 to eded390 Compare July 17, 2019 14:16

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1497 July 17, 2019 14:16 Inactive

NickColley force-pushed the radio-trigger-setattributes branch from eded390 to 42ab630 Compare July 17, 2019 14:29

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1497 July 17, 2019 14:30 Inactive

NickColley and others added 3 commits July 17, 2019 15:31

Add example to demo multiple radio groups

b4a0232

Only handle clicks on radio inputs

7858436

This stops this loop for running unnecessarily, for example for clicks in an open conditional reveal.

NickColley and others added 5 commits July 17, 2019 15:31

Use querySelector string to filter out inputs

129cd38

We did not need the radios check as the querySelector already had `[type="radio"]`. We can also remove the aria-controls check by movin it into the querySelector as `[aria-controls]`.

Only set attributes on inputs with same name

d296af8

We do not need to run this code for other inputs as their state will not have changed.

Only query radios that are within the same form

60bc3f8

By doing this alongside checking the name we only run setAttributes on the radio inputs that could change.

Check for radio conditionals before setting attrs

9eb6465

Since sometimes users set aria-controls unrelated to conditional reveals we need to be extra careful here. See #1156 for more context.

Update CHANGELOG entry

8665fa5

NickColley force-pushed the radio-trigger-setattributes branch from 42ab630 to 8665fa5 Compare July 17, 2019 14:32

govuk-design-system-ci deployed to govuk-frontend-review-pr-1497 July 17, 2019 14:32 View deployment

hannalaakso approved these changes Jul 23, 2019

View reviewed changes

NickColley merged commit 0bce5fc into master Jul 23, 2019

NickColley deleted the radio-trigger-setattributes branch July 23, 2019 09:33

snyk-bot mentioned this pull request Feb 25, 2020

[Snyk] Upgrade govuk-frontend from 3.0.0 to 3.5.0 ministryofjustice/offender-categorisation#400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Users can now conditionally reveal content on pages with multiple grouped radios #1497

Users can now conditionally reveal content on pages with multiple grouped radios #1497

NickColley commented Jul 16, 2019 •

edited

Loading

NickColley Jul 16, 2019

36degrees Jul 16, 2019

NickColley Jul 16, 2019

NickColley Jul 16, 2019

36degrees Jul 16, 2019

NickColley Jul 16, 2019

36degrees Jul 16, 2019

NickColley commented Jul 16, 2019

NickColley commented Jul 17, 2019

NickColley commented Jul 17, 2019

hannalaakso left a comment

hannalaakso Jul 23, 2019

NickColley Jul 23, 2019

NickColley Jul 23, 2019

edwardhorsford commented Jul 23, 2019

NickColley commented Jul 23, 2019

Users can now conditionally reveal content on pages with multiple grouped radios #1497

Users can now conditionally reveal content on pages with multiple grouped radios #1497

Conversation

NickColley commented Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickColley commented Jul 16, 2019

NickColley commented Jul 17, 2019

NickColley commented Jul 17, 2019

hannalaakso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardhorsford commented Jul 23, 2019

NickColley commented Jul 23, 2019

NickColley commented Jul 16, 2019 •

edited

Loading