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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
163 changes: 162 additions & 1 deletion app/views/examples/conditional-reveals/index.njk
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}

Expand Down Expand Up @@ -170,6 +171,166 @@
]
}) }}

{% endblock %}
<hr class="govuk-section-break govuk-section-break--l">

{% call govukFieldset({
legend: {
text: "Which colours do you like?",
classes: "govuk-fieldset__legend--l"
}
})%}

<h2 class="govuk-heading-m">Primary colours</h2>

{{ govukCheckboxes({
idPrefix: "colour-primary",
name: "colour",
items: [
{
value: "red",
text: "Red"
},
{
value: "yellow",
text: "Yellow",
conditional: {
html: '<p class="govuk-body">Orange is much nicer than yellow!</p>'
}
},
{
value: "blue",
text: "Blue"
}
]
}) }}

<h2 class="govuk-heading-m">Secondary colours</h2>

{{ govukCheckboxes({
idPrefix: "colour-secondary",
name: "colour",
items: [
{
value: "green",
text: "Green"
},
{
value: "purple",
text: "Purple"
},
{
value: "orange",
text: "Orange",
conditional: {
html: '<p class="govuk-body">I like orange too!</p>'
}
}
]
}) }}

<h2 class="govuk-heading-m">Other colours</h2>

{{ 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 %}

<hr class="govuk-section-break govuk-section-break--l">

{% call govukFieldset({
legend: {
text: "Which colour is your favourite?",
classes: "govuk-fieldset__legend--l"
}
})%}

<h2 class="govuk-heading-m">Primary colours</h2>

{{ govukRadios({
idPrefix: "fave-primary",
name: "fave",
items: [
{
value: "red",
text: "Red"
},
{
value: "yellow",
text: "Yellow",
conditional: {
html: '<p class="govuk-body">Orange is much nicer than yellow!</p>'
}
},
{
value: "blue",
text: "Blue"
}
]
}) }}

<h2 class="govuk-heading-m">Secondary colours</h2>

{{ govukRadios({
idPrefix: "fave-secondary",
name: "fave",
items: [
{
value: "green",
text: "Green"
},
{
value: "purple",
text: "Purple"
},
{
value: "orange",
text: "Orange",
conditional: {
html: '<p class="govuk-body">Orange is my favourite colour!</p>'
}
}
]
}) }}

<h2 class="govuk-heading-m">Other colours</h2>

{{ 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 %}

</div>
</div>
{% endblock %}
12 changes: 5 additions & 7 deletions src/govuk/components/checkboxes/checkboxes.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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))
lfdebrux marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -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))
}

/**
Expand Down
48 changes: 48 additions & 0 deletions src/govuk/components/checkboxes/checkboxes.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})
})
})
2 changes: 1 addition & 1 deletion src/govuk/components/radios/radios.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'))
Copy link
Member

Choose a reason for hiding this comment

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

🙌 🙌 🙌


if ($target && $target.classList.contains('govuk-radios__conditional')) {
var inputIsChecked = $input.checked
Expand Down
26 changes: 26 additions & 0 deletions src/govuk/components/radios/radios.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})
})
})
9 changes: 1 addition & 8 deletions src/govuk/components/radios/template.njk
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
Expand Down Expand Up @@ -45,7 +38,7 @@
{% endif %}
<div class="govuk-radios {%- if params.classes %} {{ params.classes }}{% endif %}{%- if isConditional %} govuk-radios--conditional{% endif -%}"
{%- for attribute, value in params.attributes %} {{ attribute }}="{{ value }}"{% endfor %}
{%- if isConditional %} data-module="govuk-radios"{% 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 -#}
Expand Down