fix(a11y): improve facet selector accessibility#1536
fix(a11y): improve facet selector accessibility#1536Dayifour wants to merge 1 commit intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis pull request enhances the accessibility and semantic structure of the FacetSelector component. Category controls have been converted from plain buttons to radio controls with appropriate ARIA attributes ( Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/components/Compare/FacetSelector.vue (1)
31-64:⚠️ Potential issue | 🟠 Major
role="radiogroup"is missing required keyboard navigation, anddisabledbreaks focus discoverability.Two issues that together undermine the radio group semantics:
1 — Missing arrow-key handling.
User interactions for radiogroups must replicate the user interaction of a user entering into a group of same-named HTML radio buttons. Keyboard events for tabs, space, and arrow keys must be captured. Click events on both the radio elements and their associated labels must also be captured. Additionally, focus must be managed. The component only has@clickhandlers; there are nokeydownhandlers for arrow-key navigation between the All/None radios.2 — HTML
disabledonrole="radio"removes options from keyboard reach.
Asradiois an interactive control, it must be focusable and keyboard accessible. If the role is applied to a non-focusable element, use thetabindexattribute to change this. Using the HTMLdisabledattribute removes the button entirely from the focus order. There can be instances where elements need to be exposed as disabled, but are still available for users to find when navigating via the Tab key. Doing so can improve their discoverability as they will not be removed from the focus order of the web page, asaria-disableddoes not change the focusability of such elements.For
role="radio"buttons, replace the HTMLdisabledattribute witharia-disabledand guard the click handler programmatically:🔧 Proposed fix for both the All and None buttons
<ButtonBase role="radio" :aria-label="$t('compare.facets.select_category', { category: getCategoryLabel(category) })" :aria-checked="isCategoryAllSelected(category)" - :disabled="isCategoryAllSelected(category)" + :aria-disabled="isCategoryAllSelected(category)" `@click`="!isCategoryAllSelected(category) && selectCategory(category)" size="small" > {{ $t('compare.facets.all') }} </ButtonBase> <span class="text-2xs text-fg-muted/40">/</span> <ButtonBase role="radio" :aria-label="$t('compare.facets.deselect_category', { category: getCategoryLabel(category) })" :aria-checked="isCategoryNoneSelected(category)" - :disabled="isCategoryNoneSelected(category)" + :aria-disabled="isCategoryNoneSelected(category)" `@click`="!isCategoryNoneSelected(category) && deselectCategory(category)" size="small" > {{ $t('compare.facets.none') }} </ButtonBase>Arrow-key support (keydown handler on the radiogroup container or individual buttons) also needs to be wired up to satisfy the APG pattern.
test/nuxt/components/compare/FacetSelector.spec.ts (1)
167-167:⚠️ Potential issue | 🟡 MinorStale test description — still references
aria-pressedafter switching toaria-checked.🛠️ Suggested fix
- it('applies aria-pressed for selected state', async () => { + it('applies aria-checked for selected state', async () => {
🧹 Nitpick comments (3)
app/components/Compare/FacetSelector.vue (2)
31-35: Identicalaria-labelon both theradiogroupand the innergroupmakes them indistinguishable to screen readers.Both containers use
getCategoryLabel(category)(e.g."Performance") as their accessible name, so a screen reader user would hear the same category label announced twice when moving through the page structure.Consider using
aria-labelledbyon both elements pointing to a single labelling<span>with a generated ID (e.g.,category-label-${category}), and optionally differentiate the inner group's label (e.g.,"Performance facets").♻️ Suggested refactor
<div v-for="category in categoryOrder" :key="category"> - <div - class="flex items-center gap-2 mb-2" - role="radiogroup" - :aria-label="getCategoryLabel(category)" - > - <span class="text-3xs text-fg-subtle uppercase tracking-wider"> - {{ getCategoryLabel(category) }} - </span> + <span + :id="`category-label-${category}`" + class="text-3xs text-fg-subtle uppercase tracking-wider" + > + {{ getCategoryLabel(category) }} + </span> + <div + class="flex items-center gap-2 mb-2" + role="radiogroup" + :aria-labelledby="`category-label-${category}`" + > <!-- All / None buttons --> </div> <div class="flex items-center gap-1.5 flex-wrap" role="group" - :aria-label="getCategoryLabel(category)" + :aria-labelledby="`category-label-${category}`" >Also applies to: 67-71
82-82: Pre-existingfocus-visible:outline-accent/70onButtonBaseviolates the project's global button focus convention.The project applies button focus-visible styling globally via
app/assets/main.css. This inline utility class is redundant and inconsistent with that approach.♻️ Suggested removal
class="gap-1 px-1.5 rounded transition-colors focus-visible:outline-accent/70" + class="gap-1 px-1.5 rounded transition-colors"Based on learnings: "In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in
app/assets/main.css… Do not apply per-element inline utility classes likefocus-visible:outline-accent/70on these elements in Vue templates or components."test/nuxt/components/compare/FacetSelector.spec.ts (1)
173-175:button[aria-checked]now matches All/Nonerole="radio"buttons too, making the assertion ambiguous.With the new implementation, both facet checkboxes and the All/None radio buttons carry
aria-checked. In this test setup (downloadsselected), the None radios for the health, compatibility, and security categories also havearia-checked="true". The test passes because the downloads facet button appears first in the DOM, but this is a DOM-order coincidence.Narrow the selector to target only
role="checkbox"buttons:♻️ Suggested fix
- const buttons = component.findAll('button[aria-checked]') - const selectedButton = buttons.find(b => b.attributes('aria-checked') === 'true') + const buttons = component.findAll('button[role="checkbox"]') + const selectedButton = buttons.find(b => b.attributes('aria-checked') === 'true')
|
Ready for review |
There was a problem hiding this comment.
If we’re switching to radios we’ll need to implement a roving tabindex for the controls inside of the group and control these using arrow keys, since role=radiogroup is a composite widget (i.e. it inherits from the abstract role=select which itself inherits from role=composite).
Furthermore, when operating with a keyboard, the option should become checked when focused (i.e. following focus the arrow keys), rather than on Enter—the latter usually submits a form for the native controls, so we’d likely need to prevent that.
We can get rid of the redundant :aria-label="facet.label" on the <button> elements as those would be computed from their contents which include the label even if we’re setting role=radio and role=checkbox. In the case the the facets are “coming soon” and are disabled, it’s fine that that is included in the label.
It would be nice to just use the built-in elements for this as we’re breaking the first rule of ARIA, but I don’t think it would be very easy with how our components/styling currently work.
| :aria-label="getCategoryLabel(category)" | ||
| > | ||
| <span class="text-3xs text-fg-subtle uppercase tracking-wider"> |
There was a problem hiding this comment.
Nice catch with the labels for the group/radiogroup roles.
Let’s set an id on the <span> with the visual label and use aria-labelledby to reference it.
summary
This PR improves the accessibility of the compare facet selector.
changes
role="radiogroup"androle="radio"aria-checkedinstead ofaria-pressedrole="checkbox"andaria-checkedtesting
notes