Skip to content

fix(web-components): #4653 read out radio group label when hidden - #4655

Merged
GCHQ-Developer-530 merged 1 commit into
mi6:developfrom
MFA-G:fix/4653-radio-group-hide-label-aria
Sep 1, 2026
Merged

fix(web-components): #4653 read out radio group label when hidden#4655
GCHQ-Developer-530 merged 1 commit into
mi6:developfrom
MFA-G:fix/4653-radio-group-hide-label-aria

Conversation

@MFA-G

@MFA-G MFA-G commented Aug 25, 2026

Copy link
Copy Markdown

Summary of the changes

When hideLabel is true, ic-radio-group does not render the <legend>, and nothing else supplies the group's accessible name — so the role="radiogroup" fieldset ends up with no accessible name and screen readers announce the group without its label. The prop's own docs say the opposite:

If true, the label will be hidden and the required label value will be applied as an aria-label.

This applies label as an aria-label on the fieldset when, and only when, the label is hidden:

<fieldset
  role="radiogroup"
  id={this.name}
  aria-required={`${required}`}
  aria-label={hideLabel ? label : undefined}
>

The visible-label path is untouched: the <legend> still names the group, and no aria-label is added, so nothing overrides it. ic-text-field (aria-label={label} on the input) and ic-checkbox-group (aria-labelledby on a screen-reader-only span) already do the equivalent — this brings the radio group in line.

Related issue

Fixes #4653

Testing

Two spec tests added to ic-radio-group.spec.ts:

  • with hide-label: no <legend> is rendered and the fieldset's aria-label is the label value;
  • without it: the <legend> is rendered and the fieldset has no aria-label (so the legend keeps naming the group).

The first fails on develop and passes with the change. Existing snapshots are unaffected, since none of them set hide-label.

npx stencil test --spec       →  67 suites, 1160 tests, 656 snapshots — all passing
npx eslint src/components/ic-radio-group  →  0 errors

Checklist

General

  • Changes to docs package checked and committed. — no docs change needed; the prop is already documented as behaving this way.
  • All acceptance criteria reviewed and met.

Testing

  • Relevant unit tests and visual regression tests added. — unit tests added; there is no visual change, so no VR test.
  • Visual testing against Figma component specification completed. — no rendered output changes.
  • Playground stories in React Storybook up to date, with any prop changes and additions addressed. — no prop changes.

Accessibility

  • A11y unit test added and yields no issues.
  • Correct roles used and ARIA attributes used correctly where required.
  • Logical heading structure is maintained.

Resize/zoom, system modes, content extremes

Not applicable — the change adds a single ARIA attribute and produces no visual or layout difference.

Note: I do not have NVDA/VoiceOver or the Accessibility Insights FastPass available on this machine, so the manual screen reader passes on the checklist have not been performed by me. The behaviour is covered by the unit tests above and is straightforward to confirm in Storybook with hideLabel toggled, following the reproduction steps in the issue.

@CLAassistant

CLAassistant commented Aug 25, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Welcome 👋

Welcome to the ic-ui-kit repo, thank you for submitting a pull request!

How to contribute

Please read our CONTRIBUTING.md, which explains our ways of working and guidelines for contributions.

Code of Conduct

We'd appreciate it if you could read and abide by our Code of Conduct, as we wish to foster an inclusive and respectful community.

Targeting your pull request

We use develop rather than main as the base for contributions - please make sure your PR is targeting develop.

Signing the CLA

We require all contributors to sign our Contributor License Agreement (CLA) before we can accept a contribution. If you are contributing on behalf of an organization please follow your organization's policies in signing CLAs.

Associated issue

Please make sure that your pull request has an issue open - this allows us to keep track of changes made and offer support where needed.

@GCHQ-Developer-299 GCHQ-Developer-299 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @MFA-G, thank you for your contribution, the code change looks good!

However, the commit message does not follow the structure required in our CONTRIBUTING.md. Please edit your commit message appropriately.

@MFA-G
MFA-G force-pushed the fix/4653-radio-group-hide-label-aria branch from 85b1246 to 79850c2 Compare August 25, 2026 14:29
@MFA-G

MFA-G commented Aug 25, 2026

Copy link
Copy Markdown
Author

Thanks @GCHQ-Developer-299 — amended in 79850c2.

The subject was lower case and the footer used refs #4653 rather than a closing keyword. Per CONTRIBUTING.md it now reads:

fix(web-components): #4653 Read out radio group label when it is hidden

<body>

fix #4653
  • type fix, scope web-components (single scope, one commit — only ic-radio-group.tsx and its tests changed);
  • issue reference #4653 at the start of the description, present tense;
  • longer body kept, and the issue link moved to the fix #4653 footer so the issue closes on merge.

npx commitlint passes against the repo config (config-conventional + config-lerna-scopes). Force-pushed with --force-with-lease; the diff is byte-for-byte unchanged from the version you reviewed.

I'll get the CLA signed as well.

@GCHQ-Developer-299

Copy link
Copy Markdown
Contributor

Thanks @GCHQ-Developer-299 — amended in 79850c2.

The subject was lower case and the footer used refs #4653 rather than a closing keyword. Per CONTRIBUTING.md it now reads:

fix(web-components): #4653 Read out radio group label when it is hidden

<body>

fix #4653
  • type fix, scope web-components (single scope, one commit — only ic-radio-group.tsx and its tests changed);
  • issue reference #4653 at the start of the description, present tense;
  • longer body kept, and the issue link moved to the fix #4653 footer so the issue closes on merge.

npx commitlint passes against the repo config (config-conventional + config-lerna-scopes). Force-pushed with --force-with-lease; the diff is byte-for-byte unchanged from the version you reviewed.

I'll get the CLA signed as well.

@MFA-G that's closer, but you have to remove the ticket number from the top line of the commit. 'fix #4658' in the commit body is the correct place to put it. If it helps to provide an example of a good commit message: here is one that has the correct structure

@MFA-G
MFA-G force-pushed the fix/4653-radio-group-hide-label-aria branch from 79850c2 to 229698c Compare August 27, 2026 01:18
@MFA-G

MFA-G commented Aug 27, 2026

Copy link
Copy Markdown
Author

Thanks @GCHQ-Developer-299 — removed the ticket number from the subject line in 229698c, following the structure of the example commit you linked.

The commit message is now:

fix(web-components): read out radio group label when it is hidden

When `hideLabel` is true the legend is not rendered, so the radiogroup
fieldset has no accessible name at all and screen readers announce the
group without its label. Apply `label` as an `aria-label` on the
fieldset in that case, matching the documented behaviour of the prop and
the way ic-text-field and ic-checkbox-group already behave.

fix #4653
  • subject: type(scope): description only, no ticket number, present tense, lower case;
  • issue reference kept in the footer as fix #4653 so the issue closes on merge.

npx commitlint passes against the repo config. The tree is byte-for-byte identical to the version you already approved code-wise — only the commit message changed.

@GCHQ-Developer-299

Copy link
Copy Markdown
Contributor

Hi @MFA-G please rebase your branch against develop to get the audit bump we've just merged in. After that your branch should be ready for us to review

@MFA-G

MFA-G commented Aug 27, 2026

Copy link
Copy Markdown
Author

Thanks @GCHQ-Developer-299 — branch is now up to date with develop (711437b), so the audit bump you merged is included.

I brought develop in with a merge commit rather than a rebase, so the review history above stays anchored to the reviewed commit (229698c). If you'd prefer a linear branch I'm happy to force-push a rebase instead — just say the word.

The only failing check was the Audit step of Static analysis tests, which is what the bump addresses; the functional change itself is unchanged (ic-radio-group.tsx +1 line, ic-radio-group.spec.ts +26). Ready for review.

@GCHQ-Developer-299

Copy link
Copy Markdown
Contributor

Hi @MFA-G as requested initially, please rebase against develop instead of merging.

@MFA-G
MFA-G force-pushed the fix/4653-radio-group-hide-label-aria branch from 711437b to e173922 Compare August 28, 2026 01:18
@MFA-G

MFA-G commented Aug 28, 2026

Copy link
Copy Markdown
Author

Done — rebased onto develop as requested (e173922). I dropped the merge commit, so the branch is now a single linear commit on top of the latest develop (including the audit bump) and the diff is unchanged from the reviewed 229698c. Sorry for the extra round trip.

@GCHQ-Developer-299

Copy link
Copy Markdown
Contributor

Thank you @MFA-G that's perfect. Unfortunately we just had to fix an audit issue so I need you to rebase against develop once more, then this should be ready to review!

When `hideLabel` is true the legend is not rendered, so the radiogroup
fieldset has no accessible name at all and screen readers announce the
group without its label. Apply `label` as an `aria-label` on the
fieldset in that case, matching the documented behaviour of the prop and
the way ic-text-field and ic-checkbox-group already behave.

fix mi6#4653
@MFA-G
MFA-G force-pushed the fix/4653-radio-group-hide-label-aria branch from e173922 to ed89804 Compare August 29, 2026 01:19
@MFA-G

MFA-G commented Aug 29, 2026

Copy link
Copy Markdown
Author

Rebased onto develop again (now at eeb8c2b, which includes the audit fix). The branch is 1 ahead / 0 behind with a single commit and the diff is unchanged. Ready for review whenever you are — thanks for your patience!

@GCHQ-Developer-299
GCHQ-Developer-299 requested a review from a team September 1, 2026 08:49
@GCHQ-Developer-530
GCHQ-Developer-530 merged commit bfe2af0 into mi6:develop Sep 1, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants