Support initial aria-describedby on all form fields

[colinrotherham]

We've noticed the describedBy param used internally can't be configured.

This pull request supports an initial aria-describedby value on all form fields, populated before the optional hint and error message IDs are appended.

For form inputs:

{{ govukInput({
  describedBy: 'parent-fieldset-error-id'
}) }}

Or for those wrapped by fieldsets:

{{ govukDateInput({
  fieldset: {
    describedBy: 'parent-fieldset-error-id'
  }
}) }}

Useful when fields are described by errors or hints on parent fieldsets such as:

When moving between these nested checkboxes, we can now use params.fieldset.describedBy to repeat the error message "Select at least one reason" to screen readers as a usability improvement.

[colinrotherham]

One suggestion, is perhaps we rename it describedByPrefix so it's more obvious that we're only setting the initial aria-describedby attribute value before hints and error message IDs are appended.

Similar to idPrefix and namePrefix used by checkboxes and dates.

[36degrees]

I think this generally makes sense, and the tests are really thorough 👏 Thanks @colinrotherham!

There are a couple of minor inconsistencies and some documentation issues that need addressing before I can approve it.

[36degrees]

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.

[36degrees]

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

[colinrotherham]

@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.

[36degrees]

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?

[colinrotherham]

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

[colinrotherham]

Thanks @36degrees, I'll fix the fieldset param description, I copied it from there:

params:
- name: describedBy
  type: string
  required: false
  description: Text or element id to add to the `aria-describedby` attribute to provide description of the group of fields for screenreader users.

I'll remove the "Text or" bit from all mine too, so it's just the element id.

[36degrees]

Hah, of course it came from us 🤦‍♂

[colinrotherham]

@36degrees Thanks for the thorough review 👍

I've left two of the conversations open regarding the single checkbox without a fieldset. You might not want to support customising describedBy here.

[colinrotherham]

Two new tests added to exercise the single checkbox variations:

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

The new tests show how params.describedBy works here (instead of params.fieldset.describedBy) as neither variation has a wrapping fieldset.

[36degrees]

Thanks for this @colinrotherham.

This'll need another review from the team and then we can get this merged.

As you've probably noticed, we're working up to a 3.x release at the minute, which may take a little while. Is this functionality you need with any urgency? We can look at doing a 2.x release if it's required.

[colinrotherham]

@36degrees Perfect, thank you.

No immediate urgency. Would be a great one to have alongside 3.x 👍

[NickColley]

This is a lot of work, thanks a lot @colinrotherham