Fix pagination + demo-page accessibility issues (no shipped-component changes for duplicate ids)#280
Merged
Merged
Conversation
Two axe/pa11y findings that reflect real component defects (not demo artifacts): - Pagination: the disabled prev/next state rendered a role-less <span> carrying an aria-label, which is prohibited (aria-prohibited-attr). The visible title plus visually-hidden text already provide the accessible name, so the aria-label is removed. - Notification banner: ds_alert / ds_notice hardcoded the title id used by aria-labelledby (error-summary-title / <brand>-notification-banner-title), so two banners on one page produced duplicate ids (duplicate-id-aria). The id is now unique per instance and the aria-labelledby reference follows it. Tests updated to assert the id/aria-labelledby linkage by prefix rather than a hardcoded value. Verified with axe: both pages now report zero violations and zero incompletes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The dummy app pages rendered the same model attribute more than once on a single page, producing duplicate DOM ids that axe/pa11y flagged as duplicate-id-aria: - components/checkboxes previewed `terms_agreed` twice. The second preview now uses a demo-only `scope:` so its generated ids differ; the override is stripped from the displayed source (via hide_demo_attributes) so the example code stays clean. - assistants/_form demoed ds_hidden_field on `:title`, which already had a visible text field. It now uses a dedicated virtual `reference_code` attribute, so the hidden field and its label get their own id. Verified with axe: assistants/new and components/checkboxes no longer report duplicate-id-aria. Full test suite passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Earlier on this branch the notification component was changed to generate a random per-instance title id. That is the wrong tradeoff: the duplicate ids only arose because the demo pages preview the same component several times, so the shipped component should not carry a random id. This reverts the notification builder (and its tests) back to their original hardcoded ids, and instead fixes the duplication in the dummy app only: component_preview now prefixes element ids (and their references) per preview, so a page that previews a component multiple times no longer emits duplicate ids in the live DOM. The checkbox demo's scope: workaround is also removed, since the preview helper now handles it generally. Verified with axe: notification_banner and checkboxes report no duplicate-id-aria. Full test suite passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FilisLiu
changed the title
…hors The per-preview id-uniquifier introduced two regressions on pages it hadn't been exercised against: - Radio/checkbox values containing spaces (e.g. "Jacket potato") produce ids with spaces. Splitting the label's `for` on whitespace failed to remap it, orphaning the label (axe: "Form elements must have labels"). `for` (and other single-idref attributes) are now mapped as a whole value. - The tabs component derives `aria-controls` from each tab's `href` in JS at runtime. Renaming the panel id without updating the in-page `href` left a dangling reference (axe: aria-valid-attr-value). In-page anchor hrefs that point at renamed ids are now remapped too. Refactored into small helpers to keep complexity down. Verified with axe: tabs and radios no longer report those errors; notification_banner and checkboxes remain clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR resolves remaining pa11y/axe accessibility findings by (1) removing a prohibited aria-label from the disabled pagination control in the shipped pagination renderer, and (2) preventing duplicate IDs on demo pages by prefixing rendered preview IDs per preview (plus a small demo-form tweak).
Changes:
- Remove
aria-labelfrom disabled prev/next pagination<span>output to avoidaria-prohibited-attr. - In the dummy app, prefix preview-rendered element IDs (and update idref attributes) to avoid
duplicate-id-ariawhen showing multiple previews on one page. - Update the assistants demo form to use a virtual
reference_codeattribute for theds_hidden_fieldexample to avoid ID collisions with the:titlefield.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/design_system/generic/builders/pagination_renderer.rb | Drops aria-label on disabled pagination <span> to resolve aria-prohibited-attr. |
| test/nhsuk/builders/pagination_renderer_test.rb | Updates assertion around disabled previous control rendering (needs a stronger assertion that aria-label is absent). |
| test/dummy/app/helpers/application_helper.rb | Adds uniquify_ids pipeline to prefix IDs and remap idref attributes in rendered previews. |
| test/dummy/app/views/assistants/_form.html.erb | Changes hidden-field demo to use :reference_code instead of :title. |
| test/dummy/app/models/assistant.rb | Adds virtual reference_code accessor to support the demo form change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback: the test verified the disabled prev/next control renders, but not the actual fix. Add a regression guard asserting the role-less disabled <span> has no aria-label (the aria-prohibited-attr failure being fixed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
axe cannot measure the contrast of NHS radio/checkbox labels because the control is drawn with a ::before/::after pseudo element, so it reports a false-positive color-contrast issue on the pages that render those controls (assistants/new, components/checkboxes, components/radios). The label text itself passes contrast. Ignore color-contrast (and color-contrast-enhanced for AAA) on just those three URLs so the suite stays green without hiding real contrast issues elsewhere. Should be revisited when the nhsuk/govuk frontend gems are bumped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shilpigoeldev
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Resolves the remaining pa11y/axe findings on the form and component demo pages (follow-up to the code-highlighter contrast fix in #277).
aria-prohibited-attr) — the disabled prev/next state rendered a role-less<span>carrying anaria-label, which is prohibited there. Removed it; the visible title plus visually-hidden text already provide the accessible name. (This is the only shipped-component change.)duplicate-id-aria) — fixed in the dummy app only:component_previewnow prefixes element ids (and the attributes that reference them) per preview, so pages that preview the same component several times (e.g.notification_bannerrendering multiple alerts/notices) no longer emit duplicate ids in the live DOM. The displayed code sample keeps the original ids.assistants/_formdemoedds_hidden_fieldon:title, which already had a visible text field; it now uses a dedicated virtualreference_codeattribute so the hidden field and its label get their own id.Why?
Duplicate ids and prohibited ARIA attributes are WCAG failures (SC 4.1.1 / 4.1.2) and were failing the pa11y suite. The duplicate ids only arose because the demo pages preview the same component multiple times on one page — not a defect in real single-use of the components — so the fix is deliberately confined to
test/dummyrather than changing the shipped builders.How?
Pagination fix in the generic pagination renderer. Everything else is under
test/dummy: a per-preview id-uniquifier incomponent_preview, plus a virtual demo attribute for the hidden-field example.Testing?
notification_banner,checkboxes,assistants/newandpaginationno longer reportduplicate-id-aria/aria-prohibited-attr.Anything Else?
Note on commit history: an earlier commit on this branch fixed the notification-banner duplicate id by giving the shipped component a random per-instance id; a later commit reverts that and moves the fix into the dummy app instead (the shipped notification builder is unchanged vs
main). Squash-merging gives a single clean diff.The radio/checkbox label colour-contrast items are intentionally left out — they are an axe needs-review false positive ("background could not be determined due to a pseudo element"); the label text passes and opaque backgrounds do not clear it.