fix(a11y): associate Settings labels with custom widgets (D6) - #56
Conversation
Wire Temperature and Thinking row labels/descriptions to their slider and dropdown via aria-labelledby / aria-describedby so assistive tech gets real names (D6). Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe settings modal now assigns stable IDs to the Temperature and Thinking labels and descriptions. The Temperature slider references its label and description. The Thinking dropdown toggle and portaled listbox reference their label and description. DOM tests verify these associations and confirm the listbox role. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/dom/render.test.js (1)
567-584: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOpen the Thinking menu before asserting the portaled listbox.
The test queries the first
[id^="portal-dropdown-menu-"]immediately after Settings opens. It does not exercise the toggle's open path, and it can inspect a hidden or unrelated portaled menu. Clicktoggle, callsettle(), then select the open listbox by its role andthinkingLabelassociation.🤖 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 `@tests/dom/render.test.js` around lines 567 - 584, Update the test around “associates Thinking label and description...” to click the Thinking dropdown toggle, await settle(), then query the open portaled listbox by role="listbox" and its aria-labelledby="thinkingLabel" association before asserting its attributes. Keep the existing label, description, and toggle assertions unchanged.
🤖 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.
Inline comments:
In `@public/app.js`:
- Around line 681-693: Update the ARIA description wiring for the thinking
toggle and menu so both include thinkingNote when it is visible, while retaining
thinkingDesc. Recompute both aria-describedby values whenever
thinkingNote.hidden changes, using the existing thinkingDropdownInstance and
thinkingNote symbols.
---
Nitpick comments:
In `@tests/dom/render.test.js`:
- Around line 567-584: Update the test around “associates Thinking label and
description...” to click the Thinking dropdown toggle, await settle(), then
query the open portaled listbox by role="listbox" and its
aria-labelledby="thinkingLabel" association before asserting its attributes.
Keep the existing label, description, and toggle assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fc63166-c96e-4bee-9879-bc24d1662d40
📒 Files selected for processing (2)
public/app.jstests/dom/render.test.js
| // Custom widgets are not labelable via <label for>, so wire the | ||
| // visible row label/description through ARIA (D6). | ||
| const thinkingToggleValue = thinkingDropdownInstance.toggle.querySelector('.dropdown-toggle-label'); | ||
| if (thinkingToggleValue && !thinkingToggleValue.id) { | ||
| thinkingToggleValue.id = 'thinkingToggleValue'; | ||
| } | ||
| thinkingDropdownInstance.toggle.setAttribute( | ||
| 'aria-labelledby', | ||
| thinkingToggleValue?.id ? 'thinkingLabel thinkingToggleValue' : 'thinkingLabel', | ||
| ); | ||
| thinkingDropdownInstance.toggle.setAttribute('aria-describedby', 'thinkingDesc'); | ||
| thinkingDropdownInstance.menu.setAttribute('aria-labelledby', 'thinkingLabel'); | ||
| thinkingDropdownInstance.menu.setAttribute('aria-describedby', 'thinkingDesc'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the visible unsupported-state note in aria-describedby.
When canThink is false, thinkingNote becomes visible at Lines 700-703. The toggle and listbox still reference only thinkingDesc at Lines 691-693. Assistive technology does not receive the visible explanation as the widget description. Update both aria-describedby values whenever thinkingNote.hidden changes.
🤖 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 `@public/app.js` around lines 681 - 693, Update the ARIA description wiring for
the thinking toggle and menu so both include thinkingNote when it is visible,
while retaining thinkingDesc. Recompute both aria-describedby values whenever
thinkingNote.hidden changes, using the existing thinkingDropdownInstance and
thinkingNote symbols.
1 participant
Summary
Closes #50 ([a11y][D6]). Temperature and Thinking in Settings were orphaned labels: custom widgets rendered into separate containers, so AT never got the visible label or description.
Changes
App-only fix. Each row label/description now has a stable
id. After constructing the widgets we set:role="slider"):aria-labelledby+aria-describedbyaria-labelledbywins over the DS default slideraria-label)No design-system API change. D2–D5 and D7 left alone.
Test plan
npm test(DOM harness asserts label/description association for both widgets)A11Y_CI=1) — passed; no baseline shrink (rule was already absent from the checked-in baseline)for="customInstructionsEl"