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

Support initial aria-describedby on all form fields #1347

Merged
merged 1 commit into from
May 21, 2019
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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@

🆕 New features:

- Support aria-describedby on all form fields

All form fields now support an initial `aria-describedby` value, populated before the optional hint and error message IDs are appended.

Useful when fields are described by errors or hints on parent fieldsets.

([PR #1347](https://github.com/alphagov/govuk-frontend/pull/1347))

- Pull Request Title goes here

Description goes here (optional)
Expand Down
20 changes: 20 additions & 0 deletions src/components/checkboxes/checkboxes.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
params:
- name: describedBy
type: string
required: false
description: One or more element IDs to add to the input `aria-describedby` attribute without a fieldset, used to provide additional descriptive information for screenreader users.
- name: fieldset
type: object
required: false
Expand Down Expand Up @@ -241,6 +245,22 @@ examples:
hint:
colinrotherham marked this conversation as resolved.
Show resolved Hide resolved
text: Go on, you know you want to!

- name: with fieldset and error message
data:
name: colours
errorMessage:
text: Please accept the terms and conditions
fieldset:
legend:
text: What is your nationality?
items:
- value: british
text: British
- value: irish
text: Irish
- value: other
text: Citizen of another country

- name: with all fieldset attributes
data:
idPrefix: example
Expand Down
5 changes: 4 additions & 1 deletion src/components/checkboxes/template.njk
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@

{#- a record of other elements that we need to associate with the input using
aria-describedby – for example hints or error messages -#}
{% set describedBy = "" %}
{% set describedBy = params.describedBy if params.describedBy else "" %}
{% if params.fieldset.describedBy %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right in thinking there's no difference between passing { fieldset : { describedBy: 'foo' }} and { describedBy: 'foo' }? Wondering if we need to support both of those, or if we should just support passing it on the fieldset.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would also be consistent with the date input and radio components.

Copy link
Contributor Author

@colinrotherham colinrotherham May 17, 2019

Choose a reason for hiding this comment

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

@36degrees It's because checkboxes are special, unlike radios.

Checkboxes with single option (and hint) set 'aria-describedby' on input

http://govuk-frontend-review.herokuapp.com/components/checkboxes/with-single-option-(and-hint)-set-'aria-describedby'-on-input/preview

So the describedBy attribute can be without a fieldset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, in which case I think this makes sense, thanks for talking me through it.

I can't see a test that covers this functionality (setting aria-describedby on a single checkbox without a fieldset) – could we add one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! Thanks.

I've extended the existing single checkbox tests and made two new ones.

You can see the new bit pushed here: https://github.com/alphagov/govuk-frontend/compare/8e23ce86235c93a77d54b7ccccb0edad923ef7e5..09a9700814d433925b52a9f6d25848093cacdd61

{% set describedBy = params.fieldset.describedBy %}
{% endif %}

{% set isConditional = false %}
{% for item in params.items %}
Expand Down
121 changes: 117 additions & 4 deletions src/components/checkboxes/template.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,30 @@ describe('Checkboxes', () => {
expect($component.hasClass('app-checkboxes--custom-modifier')).toBeTruthy()
})

it('renders initial aria-describedby on fieldset', () => {
const describedById = 'some-id'

const $ = render('checkboxes', {
name: 'example-name',
fieldset: {
describedBy: describedById
},
items: [
{
value: '1',
text: 'Option 1'
},
{
value: '2',
text: 'Option 2'
}
]
})

const $fieldset = $('.govuk-fieldset')
expect($fieldset.attr('aria-describedby')).toMatch(describedById)
})

it('render attributes', () => {
const $ = render('checkboxes', {
name: 'example-name',
Expand Down Expand Up @@ -521,7 +545,7 @@ describe('Checkboxes', () => {
})

it('associates the fieldset as "described by" the error message', () => {
const $ = render('checkboxes', examples['with all fieldset attributes'])
const $ = render('checkboxes', examples['with fieldset and error message'])

const $fieldset = $('.govuk-fieldset')
const $errorMessage = $('.govuk-error-message')
Expand All @@ -534,6 +558,27 @@ describe('Checkboxes', () => {
.toMatch(errorMessageId)
})

it('associates the fieldset as "described by" the error message and parent fieldset', () => {
const describedById = 'some-id'
const params = examples['with fieldset and error message']

params.fieldset.describedBy = describedById

const $ = render('checkboxes', params)

const $fieldset = $('.govuk-fieldset')
const $errorMessage = $('.govuk-error-message')

const errorMessageId = new RegExp(
WORD_BOUNDARY + describedById + WHITESPACE + $errorMessage.attr('id') + WORD_BOUNDARY
)

expect($fieldset.attr('aria-describedby'))
.toMatch(errorMessageId)

delete params.fieldset.describedBy
})

it('does not associate each input as "described by" the error message', () => {
const $ = render('checkboxes', examples['with error message and hints on items'])

Expand Down Expand Up @@ -576,22 +621,62 @@ describe('Checkboxes', () => {

expect($fieldset.attr('aria-describedby')).toMatch(hintId)
})

it('associates the fieldset as "described by" the hint and parent fieldset', () => {
const describedById = 'some-id'
const params = examples['with all fieldset attributes']

params.fieldset.describedBy = describedById

const $ = render('checkboxes', params)
const $fieldset = $('.govuk-fieldset')
const $hint = $('.govuk-hint')

const hintId = new RegExp(
WORD_BOUNDARY + describedById + WHITESPACE + $hint.attr('id') + WORD_BOUNDARY
)

expect($fieldset.attr('aria-describedby')).toMatch(hintId)
delete params.fieldset.describedBy
})
})

describe('when they include both a hint and an error message', () => {
it('associates the fieldset as described by both the hint and the error message', () => {
const $ = render('checkboxes', examples['with all fieldset attributes'])

const $fieldset = $('.govuk-fieldset')
const $errorMessageId = $('.govuk-error-message').attr('id')
const $hintId = $('.govuk-hint').attr('id')
const errorMessageId = $('.govuk-error-message').attr('id')
colinrotherham marked this conversation as resolved.
Show resolved Hide resolved
const hintId = $('.govuk-hint').attr('id')

const combinedIds = new RegExp(
WORD_BOUNDARY + hintId + WHITESPACE + errorMessageId + WORD_BOUNDARY
)

expect($fieldset.attr('aria-describedby'))
.toMatch(combinedIds)
})

it('associates the fieldset as described by the hint, error message and parent fieldset', () => {
const describedById = 'some-id'
const params = examples['with all fieldset attributes']

params.fieldset.describedBy = describedById

const $ = render('checkboxes', params)

const $fieldset = $('.govuk-fieldset')
const errorMessageId = $('.govuk-error-message').attr('id')
const hintId = $('.govuk-hint').attr('id')

const combinedIds = new RegExp(
WORD_BOUNDARY + $hintId + WHITESPACE + $errorMessageId + WORD_BOUNDARY
WORD_BOUNDARY + describedById + WHITESPACE + hintId + WHITESPACE + errorMessageId + WORD_BOUNDARY
)

expect($fieldset.attr('aria-describedby'))
.toMatch(combinedIds)

delete params.fieldset.describedBy
})
})

Expand Down Expand Up @@ -664,6 +749,21 @@ describe('Checkboxes', () => {
const $input = $('input')
expect($input.attr('aria-describedby')).toMatch('t-and-c-error')
})

it('adds aria-describedby to input if there is an error and parent fieldset', () => {
const describedById = 'some-id'
const params = examples["with single option set 'aria-describedby' on input"]

params.describedBy = describedById

const $ = render('checkboxes', params)
const $input = $('input')

expect($input.attr('aria-describedby'))
.toMatch(`${describedById} t-and-c-error`)

delete params.describedBy
})
})

describe('single checkbox (with hint) without a fieldset', () => {
Expand All @@ -672,5 +772,18 @@ describe('Checkboxes', () => {
const $input = $('input')
expect($input.attr('aria-describedby')).toMatch('t-and-c-with-hint-error t-and-c-with-hint-1-item-hint')
})

it('adds aria-describedby to input if there is an error, hint and parent fieldset', () => {
const describedById = 'some-id'
const params = examples["with single option (and hint) set 'aria-describedby' on input"]

params.describedBy = describedById

const $ = render('checkboxes', params)
const $input = $('input')

expect($input.attr('aria-describedby'))
.toMatch(`${describedById} t-and-c-with-hint-error t-and-c-with-hint-1-item-hint`)
})
})
})
20 changes: 19 additions & 1 deletion src/components/date-input/date-input.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,25 @@ examples:
-
name: year
classes: govuk-input--width-4
- name: with errors
- name: with errors only
data:
id: dob-errors
fieldset:
legend:
text: What is your date of birth?
errorMessage:
text: Error message goes here
items:
-
name: day
classes: govuk-input--width-2 govuk-input--error
-
name: month
classes: govuk-input--width-2 govuk-input--error
-
name: year
classes: govuk-input--width-4 govuk-input--error
- name: with errors and hint
data:
id: dob-errors
fieldset:
Expand Down
2 changes: 1 addition & 1 deletion src/components/date-input/template.njk
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

{#- a record of other elements that we need to associate with the input using
aria-describedby – for example hints or error messages -#}
{% set describedBy = "" %}
{% set describedBy = params.fieldset.describedBy if params.fieldset.describedBy else "" %}

{% if params.items %}
{% set dateInputItems = params.items %}
Expand Down
Loading