fix(Slider): bind form aria attributes on thumbs instead of root - #6768
fix(Slider): bind form aria attributes on thumbs instead of root#676861021 wants to merge 2 commits into
Conversation
aria-invalid and aria-describedby from useFormField were bound on SliderRoot, which renders a span with no role or tabindex, so assistive technologies never announced them. Bind them on each SliderThumb (role=slider, tabindex=0) instead — per APG each thumb is a separate slider control, so both thumbs of a range slider sharing the same description is correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughSlider form accessibility attributes are no longer applied to Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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.
🧹 Nitpick comments (1)
test/components/FormField.spec.ts (1)
123-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand Slider accessibility coverage.
This test covers only the plain, single-thumb path and error-derived attributes. Add cases for the
tooltipbranch, multiple thumbs, andaria-labelledby/aria-requiredso both modified focusable-control paths are protected.As per coding guidelines, component tests should cover props, slots, and accessibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/components/FormField.spec.ts` around lines 123 - 134, Expand the Slider accessibility tests in the existing “Slider” block to cover the tooltip variant and multiple-thumb rendering, in addition to the current error attributes. Add assertions for aria-labelledby and aria-required on each modified focusable thumb path, preserving the existing single-thumb error coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/components/FormField.spec.ts`:
- Around line 123-134: Expand the Slider accessibility tests in the existing
“Slider” block to cover the tooltip variant and multiple-thumb rendering, in
addition to the current error attributes. Add assertions for aria-labelledby and
aria-required on each modified focusable thumb path, preserving the existing
single-thumb error coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dd1e686e-bc42-4ea6-b351-f6c055096fa3
📒 Files selected for processing (2)
src/runtime/components/Slider.vuetest/components/FormField.spec.ts
commit: |
|
Bumping this one — CI green. Small a11y correctness fix: the form aria attributes were landing on the root instead of the thumbs. Can add a test for the aria wiring if useful. |
| } | ||
|
|
||
| if (name !== 'RadioGroup') { |
There was a problem hiding this comment.
Why not just leave the if-else statement from before and move this if (name === 'Slider') condition underneath it? Seems cleaner than shoving this new condition in between the old if-else blocks.
There was a problem hiding this comment.
Good call — done in 00f9746. The original if/else is back untouched and the Slider case is appended after it, so the diff on this file is purely additive now. Thanks!
Review feedback: keep the RadioGroup/label if/else pair intact and append the Slider aria case after it, so the diff stays purely additive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 (1)
test/components/FormField.spec.ts (1)
135-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover every focusable Slider thumb and all required ARIA attributes.
This test covers
aria-invalid,aria-describedby, androle="slider"only. The PR objective also requiresaria-labelledbyandaria-requiredon eachSliderThumbin both tooltip and plain branches. Render a multi-thumb Slider for each branch and assert the complete attribute set on every[role="slider"]element. Otherwise, a regression in a second thumb or one rendering branch can pass this test.As per coding guidelines, component tests should cover props, slots, and accessibility, following the repository's Vitest and snapshot-testing patterns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/components/FormField.spec.ts` around lines 135 - 148, Expand the Slider accessibility test around the “Slider” branch to render multi-thumb sliders through both tooltip and plain branches. For every element matching [role="slider"], assert role, aria-invalid, aria-describedby, aria-labelledby, and aria-required, ensuring all thumbs receive the complete attribute set and preserving the repository’s Vitest test patterns.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/components/FormField.spec.ts`:
- Around line 135-148: Expand the Slider accessibility test around the “Slider”
branch to render multi-thumb sliders through both tooltip and plain branches.
For every element matching [role="slider"], assert role, aria-invalid,
aria-describedby, aria-labelledby, and aria-required, ensuring all thumbs
receive the complete attribute set and preserving the repository’s Vitest test
patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e4f8547-0e6d-44b8-b586-aa3f15c97b3d
📒 Files selected for processing (1)
test/components/FormField.spec.ts
2 participants
🔗 Linked issue
Resolves #6749
❓ Type of change
📚 Description
useFormField'sariaAttrs(aria-invalid,aria-describedby) were bound onSliderRoot, which reka-ui renders as a plain<span>with no role and notabindex— assistive technologies never announce attributes there. The focusable control is the thumb (role="slider",tabindex="0"), so this moves the binding onto eachSliderThumb(both the tooltip and plain branches).Per the APG slider pattern each thumb is a separate slider control, so both thumbs of a range slider sharing the same
aria-describedbyis correct.:idstays on the root (multiple thumbs can't share one id), keeping the existinglabel[for]association tests green. When used outside aUFormField,ariaAttrsisundefinedandv-bind="undefined"is a no-op — no snapshot changes.Includes a placement-specific regression test in
FormField.spec.tsasserting the attributes land on therole="slider"element (exactly one match) rather than anywhere in the tree.Credit: #6750 by @nononavasgit proposed the same direction before being closed by its author.
📝 Checklist