Skip to content

fix(a11y): put role=slider on handles, not the wrapper (D5) - #17

Merged
BrianGenisio merged 1 commit into
mainfrom
fix/a11y-d5-slider-role
Aug 12, 2026
Merged

fix(a11y): put role=slider on handles, not the wrapper (D5)#17
BrianGenisio merged 1 commit into
mainfrom
fix/a11y-d5-slider-role

Conversation

@BrianGenisio

Copy link
Copy Markdown
Contributor

Summary

Closes #10 ([a11y][D5]). Each slider handle is now the role="slider" control; the wrapper is presentational only. Range mode exposes a single-number aria-valuenow per thumb.

Changes

In components/numeric-slider/:

  • Wrapper: no role, no tabindex, no value attributes
  • Handles: role="slider", aria-valuemin/max/now/valuetext, aria-orientation="horizontal", aria-label
  • Disabled: aria-disabled="true" + tabindex="-1" (alongside native disabled)
  • Keyboard still on handles (arrows, Shift, Home, End); wrapper keydown listener removed

Note for consumers: DOM/ARIA for the slider changed (value attrs moved from .numeric-slider-wrapper onto .numeric-slider-handle). Selectors that read wrapper aria attributes will need updating.

Test plan

  • npm test — Playwright light + dark: single/range role placement, single-number aria-valuenow, disabled attrs, keyboard updates, scoped axe (no nested-interactive)
  • Existing dropdown suite still green
  • Optional: settings modal in ChatCPT after submodule bump (not in this PR)

Made with Cursor

Move value attributes onto each thumb so range mode exposes a single
aria-valuenow per handle and axe nested-interactive no longer fires.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The numeric slider now places slider semantics on each handle instead of the wrapper. Handle ARIA values update with slider changes, and disabled handles expose synchronized disabled state and tab order. Keyboard listeners operate on active handles. Playwright tests cover single, range, and disabled sliders in light and dark themes, including axe checks and keyboard updates. Documentation now describes setup, semantics, and test coverage.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary accessibility change: moving the slider role from the wrapper to the handles.
Description check ✅ Passed The description accurately explains the ARIA structure changes, keyboard behavior, consumer impact, and test coverage.
Linked Issues check ✅ Passed The implementation and tests address issue #10 requirements for per-handle roles, valid values, disabled state, keyboard behavior, axe checks, and color modes.
Out of Scope Changes check ✅ Passed The code, documentation, test fixture, and accessibility tests directly support the linked issue and stated pull request objectives.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
components/numeric-slider/numeric-slider.js (1)

269-275: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Prevent range handles from swapping values on Home and End.

When End runs on the min handle, setValue([max, currentMax]) reorders the range. When Home runs on the max handle, setValue([currentMin, min]) also reorders the range. Clamp the focused handle to the other handle instead. Add regression coverage for both cases.

Proposed fix
       if (this.config.type === 'range' && handleType === 'min') {
         this.setValue([this.config.min, this.values[1]], 'min');
       } else if (this.config.type === 'range' && handleType === 'max') {
-        this.setValue([this.values[0], this.config.min], 'max');
+        this.setValue([this.values[0], this.values[0]], 'max');
...
       if (this.config.type === 'range' && handleType === 'min') {
-        this.setValue([this.config.max, this.values[1]], 'min');
+        this.setValue([this.values[1], this.values[1]], 'min');
       } else if (this.config.type === 'range' && handleType === 'max') {
         this.setValue([this.values[0], this.config.max], 'max');
🤖 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 `@components/numeric-slider/numeric-slider.js` around lines 269 - 275, The
range keyboard handling in handleKeyDown must prevent Home and End from swapping
handle values: clamp the focused min handle to the current max on End, and the
focused max handle to the current min on Home, while preserving normal behavior
for other keys and single sliders. Add regression tests covering both
range-handle cases.
🤖 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 `@components/numeric-slider/numeric-slider.js`:
- Around line 269-275: The range keyboard handling in handleKeyDown must prevent
Home and End from swapping handle values: clamp the focused min handle to the
current max on End, and the focused max handle to the current min on Home, while
preserving normal behavior for other keys and single sliders. Add regression
tests covering both range-handle cases.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 92dc4096-dc3c-402c-b28a-5d3d4140c2ba

📥 Commits

Reviewing files that changed from the base of the PR and between 15a0288 and 41cb3cf.

📒 Files selected for processing (4)
  • components/numeric-slider/README.md
  • components/numeric-slider/numeric-slider.js
  • components/numeric-slider/test.html
  • tests/numeric-slider-a11y.spec.js

@BrianGenisio
BrianGenisio merged commit 4488dc4 into main Aug 12, 2026
2 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.

[a11y][D5] Slider role contains focusable buttons; invalid aria-valuenow in range mode

1 participant