fix(a11y): improve facet selector accessibility#1536
fix(a11y): improve facet selector accessibility#1536Dayifour wants to merge 7 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Dayifour
changed the title
📝 WalkthroughWalkthroughReworks the Compare FacetSelector component to use radio semantics for category-level controls and checkbox semantics for individual facets, adding getCategoryActiveControl(category) to determine the active category control and handleCategoryControlKeydown(category, event) to support Arrow key navigation and activation. Updates ARIA attributes and roles (role="radiogroup"/role="radio"/role="checkbox", aria-checked, aria-disabled, tabindex), adds data attributes for radiogroups and facet groups, and adjusts tests to assert aria-checked/aria-disabled instead of aria-pressed/disabled. Removes per-category i18n keys for select_category and deselect_category from locale and schema files. Possibly related PRs
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
Dayifour
force-pushed
the
feat/facet-selector-a11y
branch
from
42e49c5 to
bd677a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Compare/FacetSelector.vue (1)
93-117:⚠️ Potential issue | 🟠 Major
aria-disabledmust not mirroraria-checkedon radio buttons.Lines 96 and 109 set
:aria-disabledto the same boolean as:aria-checked. When a radio is checked (aria-checked="true"), it becomes simultaneouslyaria-disabled="true", which semantically means the option is unavailable — not that it is the currently selected option. Screen readers announce such a combination as "checked, dimmed" (e.g., VoiceOver says "radio button label dimmed radio button unchecked" for a disabled radio), misleading users into thinking the control is broken or inaccessible rather than simply being the active selection.Per the WAI-ARIA APG, if a radio button is checked the element has
aria-checkedset totrue; if it is not checked, it hasaria-checkedset tofalse.aria-disabledis a separate property reserved for options that are genuinely unavailable, not for indicating the currently selected state.The click handler and arrow-key navigation already make re-activation of the checked radio idempotent, so there is no functional need for
aria-disabledhere.♻️ Proposed fix — remove `aria-disabled` from the radio buttons
<ButtonBase role="radio" :aria-checked="isCategoryAllSelected(category)" - :aria-disabled="isCategoryAllSelected(category)" :tabindex="getCategoryActiveControl(category) === 'all' ? 0 : -1" ... > ... <ButtonBase role="radio" :aria-checked="isCategoryNoneSelected(category)" - :aria-disabled="isCategoryNoneSelected(category)" :tabindex="getCategoryActiveControl(category) === 'none' ? 0 : -1" ... >Note: If
ButtonBaseusesaria-disabledinternally to drive its disabled visual style, use a separate CSS class or prop to convey the "already selected" appearance, rather than relying onaria-disabled.
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/nuxt/components/compare/FacetSelector.spec.ts (1)
235-238: Scope radio queries to the target category container to avoid implicit ordering assumptions.All four tests use
.findAll('button[role="radio"]').find(b => b.text() === 'all'/'none')across the entire component, then assert against'performance'. This silently couples the tests toCATEGORY_ORDER[0]being'performance'; a reorder or insertion would make the tests pass against the wrong category.Prefer scoping to the specific category's radiogroup, e.g.:
♻️ Suggested approach
- const allButton = component.findAll('button[role="radio"]').find(b => b.text() === 'all') + // Scope to the performance category radiogroup + const performanceGroup = component.find('[data-facet-category-radiogroup]') + const allButton = performanceGroup.findAll('button[role="radio"]').find(b => b.text() === 'all') await allButton!.trigger('click') expect(mockSelectCategory).toHaveBeenCalledWith('performance')Apply the same scoping for the 'none', "disables all", and "disables none" tests.
Also applies to: 248-252, 265-267, 277-279
| :aria-disabled="isCategoryAllSelected(category)" | ||
| :tabindex="getCategoryActiveControl(category) === 'all' ? 0 : -1" | ||
| data-radio-type="all" | ||
| @keydown="handleCategoryControlKeydown(category, $event)" | ||
| @click="selectCategory(category)" |
There was a problem hiding this comment.
aria-disabled does not suppress click events — guard the handlers explicitly.
aria-disabled is a semantic-only attribute; the browser still dispatches click events on aria-disabled="true" elements. When 'all' is already fully selected, clicking the 'all' radio still invokes selectCategory(category). The same applies to 'none'. The facet checkboxes already follow the correct pattern with @click="!facet.comingSoon && toggleFacet(facet.id)".
🛡️ Proposed fix
- `@click`="selectCategory(category)"
+ `@click`="!isCategoryAllSelected(category) && selectCategory(category)"- `@click`="deselectCategory(category)"
+ `@click`="!isCategoryNoneSelected(category) && deselectCategory(category)"Also applies to: 110-114
…r/npmx.dev into feat/facet-selector-a11y
Dayifour
force-pushed
the
feat/facet-selector-a11y
branch
from
ff401ab to
16d01da
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/components/Compare/FacetSelector.vue (2)
26-30:getCategoryActiveControlreturns'all'for partial selection — intentional but worth a brief comment.When some (but not all) facets in a category are selected, both
isCategoryAllSelectedandisCategoryNoneSelectedreturnfalse, and the function falls through toreturn 'all'. This setstabindex="0"on the "all" radio, which follows the roving-tabindex pattern correctly. A short inline comment on the fallback would help future readers understand it's deliberate.📝 Suggested clarification
function getCategoryActiveControl(category: string): 'all' | 'none' { if (isCategoryAllSelected(category)) return 'all' if (isCategoryNoneSelected(category)) return 'none' + // Partial selection: default focus to 'all' per roving-tabindex convention return 'all' }
89-99:aria-disabledon radios was removed, but click handlers still fire unconditionally.A previous review flagged that
aria-disableddoes not suppress click events. In the current code thearia-disabledattribute is gone from the radio buttons entirely, but the click handlers still callselectCategory/deselectCategoryeven when the action is a no-op (e.g. clicking "all" when every facet is already selected). IfselectCategoryis idempotent this is harmless, but guarding the handlers keeps the semantics explicit and avoids unnecessary work.♻️ Proposed guard
`@click`="selectCategory(category)" + <!-- Consider: `@click`="!isCategoryAllSelected(category) && selectCategory(category)" -->`@click`="deselectCategory(category)" + <!-- Consider: `@click`="!isCategoryNoneSelected(category) && deselectCategory(category)" -->Also applies to: 101-111
test/nuxt/components/compare/FacetSelector.spec.ts (2)
232-282: Non-null assertions on.find()results will produce opaque errors on failure.Several places (Lines 238, 252, 269, 281) use
!on the result of.find(), e.g.allButton!.trigger('click'). If the element isn't found (e.g. due to a regression), the error will be a genericTypeError: Cannot read properties of undefinedrather than a clear test-failure message. Usingexpect(...).toBeDefined()before the assertion (as done in the checkbox test on Line 175) would improve debuggability.📝 Example for one call site
const allButton = component.findAll('button[role="radio"]').find(b => b.text() === 'all') + expect(allButton).toBeDefined() await allButton!.trigger('click')
257-282: Consider adding an explicit assertion to document the facet category assumption.The test relies on
downloadsbeing in thehealthcategory (notperformance). Whilst the assumption is correct per the current codebase, explicitly asserting this—or selecting a facet from a more obviously distinct category (e.g.,licensefrom security)—would make the test more resilient to future category mapping changes.
|
Hi @knowler ! I've completed the refactor. Here is a summary of the latest changes: A11y & Keyboard: Implemented a proper roving tabindex and arrow-key navigation for the radiogroup. Refined Semantics: Replaced aria-pressed with aria-checked and ensured aria-disabled is only used where appropriate. Clean Labels: Linked groups via aria-labelledby and removed redundant aria-label attributes on checkboxes. Tests & CI: Updated all component tests to match the new ARIA roles. All 17 checks are now green, including i18n schema and TypeScript. I've also addressed the latest feedback regarding decorative separators and event guards |
|
@Dayifour I’ll review this in-depth again once myself and the team come back from “vacation” on Saturday. Upon seeing the changes though, I’m having doubts about whether radio controls the correct tool for the job here as when interacting with the “all” or “none” control, there’s no way to clear that input other than using the individual facet controls and that feels a bit strange to me, especially when using a screen reader. I do agree the existing production implementation does need change as setting I might open an issue where a proper discussion can take place with regard to the design of these controls. |
|
Thanks for the feedback, @knowler! I completely understand the concern. By implementing the strict WAI-ARIA radio pattern, it indeed highlights that the 'All/None' actions behave more like command buttons than persistent states, which can be tricky for screen reader users to mentally map. Enjoy your vacation, and I'll look forward to the team's feedback on Saturday! No rush at all |
2 participants
summary
This PR improves the accessibility of the compare facet selector.
changes
role="radiogroup"androle="radio"aria-checkedinstead ofaria-pressedrole="checkbox"andaria-checkedtesting
notes