From 9fc2275da1bc14a76236afce8a312812e53986d6 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Fri, 18 Jun 2021 14:40:00 +0100 Subject: [PATCH 1/6] Add examples of multiple sets of radios/checkboxes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have an open issue to provide examples of grouping radios and checkboxes under headings (https://github.com/alphagov/govuk-frontend/issues/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. --- .../examples/conditional-reveals/index.njk | 163 +++++++++++++++++- 1 file changed, 162 insertions(+), 1 deletion(-) diff --git a/app/views/examples/conditional-reveals/index.njk b/app/views/examples/conditional-reveals/index.njk index 63d40e27d7..3b2c90b8df 100644 --- a/app/views/examples/conditional-reveals/index.njk +++ b/app/views/examples/conditional-reveals/index.njk @@ -2,6 +2,7 @@ {% from "back-link/macro.njk" import govukBackLink %} {% from "checkboxes/macro.njk" import govukCheckboxes %} +{% from "fieldset/macro.njk" import govukFieldset %} {% from "radios/macro.njk" import govukRadios %} {% from "input/macro.njk" import govukInput %} @@ -170,6 +171,166 @@ ] }) }} - {% endblock %} +
+ + {% call govukFieldset({ + legend: { + text: "Which colours do you like?", + classes: "govuk-fieldset__legend--l" + } + })%} + +

Primary colours

+ + {{ govukCheckboxes({ + idPrefix: "colour-primary", + name: "colour", + items: [ + { + value: "red", + text: "Red" + }, + { + value: "yellow", + text: "Yellow", + conditional: { + html: '

Orange is much nicer than yellow!

' + } + }, + { + value: "blue", + text: "Blue" + } + ] + }) }} + +

Secondary colours

+ + {{ govukCheckboxes({ + idPrefix: "colour-secondary", + name: "colour", + items: [ + { + value: "green", + text: "Green" + }, + { + value: "purple", + text: "Purple" + }, + { + value: "orange", + text: "Orange", + conditional: { + html: '

I like orange too!

' + } + } + ] + }) }} + +

Other colours

+ + {{ govukCheckboxes({ + idPrefix: "colour-other", + name: "colour", + items: [ + { + value: "imaginary", + text: "An imaginary colour" + }, + { + divider: "or" + }, + { + value: "none", + text: "None of the above", + behaviour: "exclusive" + } + ] + }) }} + + {% endcall %} + +
+ + {% call govukFieldset({ + legend: { + text: "Which colour is your favourite?", + classes: "govuk-fieldset__legend--l" + } + })%} + +

Primary colours

+ + {{ govukRadios({ + idPrefix: "fave-primary", + name: "fave", + items: [ + { + value: "red", + text: "Red" + }, + { + value: "yellow", + text: "Yellow", + conditional: { + html: '

Orange is much nicer than yellow!

' + } + }, + { + value: "blue", + text: "Blue" + } + ] + }) }} + +

Secondary colours

+ + {{ govukRadios({ + idPrefix: "fave-secondary", + name: "fave", + items: [ + { + value: "green", + text: "Green" + }, + { + value: "purple", + text: "Purple" + }, + { + value: "orange", + text: "Orange", + conditional: { + html: '

Orange is my favourite colour!

' + } + } + ] + }) }} + +

Other colours

+ + {{ govukRadios({ + idPrefix: "fave-other", + name: "fave", + items: [ + { + value: "imaginary", + text: "An imaginary colour" + }, + { + divider: "or" + }, + { + value: "none", + text: "None of the above", + behaviour: "exclusive" + } + ] + }) }} + + {% endcall %} + +{% endblock %} From 2076c6a6497611835358cdab35912b871d72bb75 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Fri, 18 Jun 2021 14:43:07 +0100 Subject: [PATCH 2/6] Sync conditional reveals across modules 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. --- src/govuk/components/checkboxes/checkboxes.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/govuk/components/checkboxes/checkboxes.js b/src/govuk/components/checkboxes/checkboxes.js index ebe086334d..bd19bfe2d1 100644 --- a/src/govuk/components/checkboxes/checkboxes.js +++ b/src/govuk/components/checkboxes/checkboxes.js @@ -76,7 +76,7 @@ Checkboxes.prototype.syncAllConditionalReveals = function () { * @param {HTMLInputElement} $input Checkbox input */ Checkboxes.prototype.syncConditionalRevealWithInputState = function ($input) { - var $target = this.$module.querySelector('#' + $input.getAttribute('aria-controls')) + var $target = document.getElementById($input.getAttribute('aria-controls')) if ($target && $target.classList.contains('govuk-checkboxes__conditional')) { var inputIsChecked = $input.checked @@ -99,10 +99,9 @@ Checkboxes.prototype.unCheckAllInputsExcept = function ($input) { var hasSameFormOwner = ($input.form === $inputWithSameName.form) if (hasSameFormOwner && $inputWithSameName !== $input) { $inputWithSameName.checked = false + this.syncConditionalRevealWithInputState($inputWithSameName) } - }) - - this.syncAllConditionalReveals() + }.bind(this)) } /** @@ -121,10 +120,9 @@ Checkboxes.prototype.unCheckExclusiveInputs = function ($input) { var hasSameFormOwner = ($input.form === $exclusiveInput.form) if (hasSameFormOwner) { $exclusiveInput.checked = false + this.syncConditionalRevealWithInputState($exclusiveInput) } - }) - - this.syncAllConditionalReveals() + }.bind(this)) } /** From e83afab865705e68bd1516099efe7a9b286beab3 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Fri, 18 Jun 2021 14:46:33 +0100 Subject: [PATCH 3/6] Always initialise modules on radios This matches a change previously made to the checkboxes component in 7e746b4670e10349a3e678f4bbd6e7d85e8b771d. 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. --- src/govuk/components/radios/template.njk | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/govuk/components/radios/template.njk b/src/govuk/components/radios/template.njk index 280ad806a5..c87f306f96 100644 --- a/src/govuk/components/radios/template.njk +++ b/src/govuk/components/radios/template.njk @@ -11,13 +11,6 @@ aria-describedby – for example hints or error messages -#} {% set describedBy = params.fieldset.describedBy if params.fieldset.describedBy else "" %} -{% set isConditional = false %} -{% for item in params.items %} - {% if item.conditional.html %} - {% set isConditional = true %} - {% endif %} -{% endfor %} - {#- Capture the HTML so we can optionally nest it in a fieldset -#} {% set innerHtml %} {% if params.hint %} @@ -45,7 +38,7 @@ {% endif %}
+ data-module="govuk-radios"> {% for item in params.items %} {% if item %} {#- If the user explicitly sets an id, use this instead of the regular idPrefix -#} From 8f05114b3b774ed22f24feefc317ccb6c7d20c6b Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Fri, 18 Jun 2021 14:48:49 +0100 Subject: [PATCH 4/6] Simplify lookup in radio conditional reveal sync Use document.getElementById rather than document.querySelector which requires us to manually prepend the hash to get an ID selector. --- src/govuk/components/radios/radios.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/govuk/components/radios/radios.js b/src/govuk/components/radios/radios.js index 1e891661da..5c4424439a 100644 --- a/src/govuk/components/radios/radios.js +++ b/src/govuk/components/radios/radios.js @@ -77,7 +77,7 @@ Radios.prototype.syncAllConditionalReveals = function () { * @param {HTMLInputElement} $input Radio input */ Radios.prototype.syncConditionalRevealWithInputState = function ($input) { - var $target = document.querySelector('#' + $input.getAttribute('aria-controls')) + var $target = document.getElementById($input.getAttribute('aria-controls')) if ($target && $target.classList.contains('govuk-radios__conditional')) { var inputIsChecked = $input.checked From 7e8f0e2acc80595d3a6b7e6f18f8906cf8940ed2 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 22 Jun 2021 17:51:34 +0100 Subject: [PATCH 5/6] Add tests for multiple sets of radios/checkboxes 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 --- .../components/checkboxes/checkboxes.test.js | 48 +++++++++++++++++++ src/govuk/components/radios/radios.test.js | 26 ++++++++++ 2 files changed, 74 insertions(+) diff --git a/src/govuk/components/checkboxes/checkboxes.test.js b/src/govuk/components/checkboxes/checkboxes.test.js index 3d5f9d2c8a..ce73487424 100644 --- a/src/govuk/components/checkboxes/checkboxes.test.js +++ b/src/govuk/components/checkboxes/checkboxes.test.js @@ -190,3 +190,51 @@ describe('Checkboxes with a None checkbox and conditional reveals', () => { }) }) }) + +describe('Checkboxes with multiple groups and a None checkbox and conditional reveals', () => { + describe('when JavaScript is available', () => { + it('none checkbox unchecks other checkboxes in other groups', async () => { + await page.goto(`${baseUrl}/examples/conditional-reveals`) + + // Check some checkboxes in the first and second groups + await page.click('#colour-primary-3') + await page.click('#colour-secondary-2') + + // Check the None checkbox in the third group + await page.click('#colour-other-3') + + // Expect the checkboxes in the first and second groups to be unchecked + const firstCheckboxIsUnchecked = await waitForVisibleSelector('[id="colour-primary-3"]:not(:checked)') + expect(firstCheckboxIsUnchecked).toBeTruthy() + + const secondCheckboxIsUnchecked = await waitForVisibleSelector('[id="colour-secondary-2"]:not(:checked)') + expect(secondCheckboxIsUnchecked).toBeTruthy() + }) + + it('hides conditional reveals in other groups', async () => { + await page.goto(`${baseUrl}/examples/conditional-reveals`) + + // Check the second checkbox in the first group, which reveals additional content + await page.click('#colour-primary-2') + + const conditionalContentId = await page.evaluate( + 'document.getElementById("colour-primary-2").getAttribute("aria-controls")' + ) + + // Assert that conditional reveal is visible + const isConditionalContentVisible = await waitForVisibleSelector(`[id="${conditionalContentId}"]`) + expect(isConditionalContentVisible).toBeTruthy() + + // Check the None checkbox + await page.click('#colour-other-3') + + // Assert that the second checkbox in the first group is unchecked + const otherCheckboxIsUnchecked = await waitForVisibleSelector('[id="colour-primary-2"]:not(:checked)') + expect(otherCheckboxIsUnchecked).toBeTruthy() + + // Expect conditional content to have been hidden + const isConditionalContentHidden = await waitForHiddenSelector(`[id="${conditionalContentId}"]`) + expect(isConditionalContentHidden).toBeTruthy() + }) + }) +}) diff --git a/src/govuk/components/radios/radios.test.js b/src/govuk/components/radios/radios.test.js index 9cc327d054..2aafc0a635 100644 --- a/src/govuk/components/radios/radios.test.js +++ b/src/govuk/components/radios/radios.test.js @@ -145,3 +145,29 @@ describe('Radios with conditional reveals', () => { }) }) }) + +describe('Radios with multiple groups and conditional reveals', () => { + describe('when JavaScript is available', () => { + it('hides conditional reveals in other groups', async () => { + await page.goto(`${baseUrl}/examples/conditional-reveals`) + + // Choose the second radio in the first group, which reveals additional content + await page.click('#fave-primary-2') + + const conditionalContentId = await page.evaluate( + 'document.getElementById("fave-primary-2").getAttribute("aria-controls")' + ) + + // Assert conditional reveal is visible + const isConditionalContentVisible = await waitForVisibleSelector(`[id="${conditionalContentId}"]`) + expect(isConditionalContentVisible).toBeTruthy() + + // Choose a different radio with the same name, but in a different group + await page.click('#fave-other-3') + + // Expect conditional content to have been hidden + const isConditionalContentHidden = await waitForHiddenSelector(`[id="${conditionalContentId}"]`) + expect(isConditionalContentHidden).toBeTruthy() + }) + }) +}) From 8d9ce9865297f5e5ef7e5200751f01273e5d7bae Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 5 Oct 2021 15:16:58 +0100 Subject: [PATCH 6/6] Document in CHANGELOG --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23ea826c6d..387aed0071 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ You must remove this setting. Otherwise, you would have to conditionally add ove This was added in [pull request 1963: Remove deprecated `$govuk-border-width-form-element-error` setting](https://github.com/alphagov/govuk-frontend/pull/1963). +## Fixes + +- [#2255: Fix conditionally revealed questions getting out of sync when multiple sets of radios and checkboxes contain inputs with the same name](https://github.com/alphagov/govuk-frontend/pull/2255) ## 3.14.0 (Feature release)