fix(a11y): repair broken aria-label, conflicting Toast role, and missing focus indicators#3374
Open
orrgottlieb wants to merge 1 commit into
Open
fix(a11y): repair broken aria-label, conflicting Toast role, and missing focus indicators#3374orrgottlieb wants to merge 1 commit into
orrgottlieb wants to merge 1 commit into
Conversation
…ing focus indicators
- ColorPicker: aria-labelledby was set to literal text instead of an ID, leaving the dialog with no accessible name for screen readers; switched to aria-label and removed the same-shaped aria-describedby bug.
- Toast: role="alert" (implicit assertive) combined with aria-live="polite" produced conflicting semantics across screen readers; switched to role="status" (implicit polite) and dropped the explicit aria-live.
- Focus indicators: replaced bare ":focus { outline: none }" with the ":focus:not(:focus-visible)" pattern in Dialog, DialogContentContainer, AvatarGroupCounterTooltipContent (+ virtualized variant), BreadcrumbContent, Menu, and Tab so keyboard focus rings are no longer suppressed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Code Review by Qodo
1. ColorPicker test asserts aria-labelledby
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
132
to
138
| <DialogContentContainer | ||
| ref={mergedRef} | ||
| className={cx(styles.colorPicker, styles.colorPickerDialogContent, className)} | ||
| aria-labelledby="Color Picker Dialog" | ||
| aria-describedby="Pick color" | ||
| aria-label="Color Picker Dialog" | ||
| style={{ width }} | ||
| data-vibe={ComponentVibeId.COLOR_PICKER} | ||
| > |
Contributor
There was a problem hiding this comment.
1. colorpicker test asserts aria-labelledby 📘 Rule violation ☼ Reliability
ColorPicker now sets aria-label on the dialog container, but the testkit test still asserts against aria-labelledby, so it no longer validates the actual accessibility attribute and may fail or give false confidence. This breaks the requirement that tests must verify real DOM accessibility attributes after behavior changes.
Agent Prompt
## Issue description
`ColorPicker` was changed to use `aria-label` (e.g., `aria-label="Color Picker Dialog"`) on the dialog container, but `packages/testkit/__tests__/ColorPicker.test.ts` still asserts `aria-labelledby`. Update the test so it validates the actual rendered accessibility attribute and does not provide false confidence or fail due to the markup change.
## Issue Context
PR Compliance ID 7 requires tests to validate real DOM accessibility attributes after changes. ColorPicker switched from `aria-labelledby="Color Picker Dialog"` to `aria-label="Color Picker Dialog"`, but the testkit test continues to read/assert `aria-labelledby`; consider also asserting `aria-labelledby` is absent if the container no longer emits empty values.
## Fix Focus Areas
- packages/testkit/__tests__/ColorPicker.test.ts[83-86]
- packages/core/src/components/ColorPicker/ColorPicker.tsx[132-138]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
132
to
137
| <DialogContentContainer | ||
| ref={mergedRef} | ||
| className={cx(styles.colorPicker, styles.colorPickerDialogContent, className)} | ||
| aria-labelledby="Color Picker Dialog" | ||
| aria-describedby="Pick color" | ||
| aria-label="Color Picker Dialog" | ||
| style={{ width }} | ||
| data-vibe={ComponentVibeId.COLOR_PICKER} |
Contributor
There was a problem hiding this comment.
2. Empty aria-labelledby overrides label 🐞 Bug ≡ Correctness
ColorPicker now passes aria-label, but DialogContentContainer still renders aria-labelledby=""/aria-describedby="" by default, which can cause the dialog to remain effectively unlabeled/undescribed for assistive tech. This defeats the PR’s stated fix for the broken accessible name.
Agent Prompt
### Issue description
`DialogContentContainer` defaults `aria-labelledby`/`aria-describedby` to empty strings and always renders those attributes. When ColorPicker switched to `aria-label`, it stopped providing valid IDREFs, but the empty IDREF attributes still render and may take precedence over `aria-label`, leaving the dialog without a computed accessible name/description.
### Issue Context
- ColorPicker now sets `aria-label="Color Picker Dialog"`.
- `DialogContentContainer` renders `aria-labelledby`/`aria-describedby` unconditionally with default `""`.
### Fix Focus Areas
- packages/components/dialog/src/DialogContentContainer/DialogContentContainer.tsx[8-69]
- packages/core/src/components/ColorPicker/__tests__/__snapshots__/ColorPicker.test.tsx.snap[3-12]
### Suggested fix
1. In `DialogContentContainer`, change defaults to `undefined` (not `""`) and only pass `aria-labelledby`/`aria-describedby` when non-empty, e.g. `aria-labelledby={ariaLabelledby || undefined}` and `aria-describedby={ariaDescribedby || undefined}`.
2. (Type safety) Add `"aria-label"?: string;` to `DialogContentContainerProps` (or widen props to allow standard div attributes) so `aria-label` is a supported/typed prop.
3. Update snapshots to reflect that `aria-labelledby`/`aria-describedby` are no longer rendered when not provided.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
📦 Bundle Size Analysis ✅ No bundle size changes detected. Unchanged Components
📊 Summary:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
Three accessibility fixes from the audit, bundled into one PR:
aria-labelledby="Color Picker Dialog"was passing literal text into an ID-reference attribute, so screen readers computed an empty name for the dialog. Same shape onaria-describedby="Pick color". Replaced both with a singlearia-label="Color Picker Dialog".role="alert"(implicitaria-live="assertive") combined with explicitaria-live="polite"produced contradictory behavior across screen readers (some announce immediately, others queue, some ignore). Switched torole="status", which already impliesaria-live="polite", and dropped the explicit attribute.:focus { outline: none }pattern, stripping the focus ring for keyboard users (WCAG 2.4.7 violation). Replaced with the standard:focus:not(:focus-visible)pattern so mouse/programmatic focus stays clean while keyboard focus stays visible:Dialog,DialogContentContainer,AvatarGroupCounterTooltipContent(+ virtualized variant),BreadcrumbContent,Menu,Tab.Components reviewed and intentionally not modified because they already have a custom focus indicator or only receive programmatic focus (Modal, TabList, Avatar, Checkbox, RadioButton, BaseMenuItem, TextField, TextArea, EditableTypography, DatePicker day cells, ColorPicker grid/items, Dropdown trigger, Icon's opt-in
.noFocusStyle). Snapshots regenerated for Toast (6 entries) and ColorPicker (2 entries) to reflect the attribute changes.Test plan
yarn workspace @vibe/core test --run— 141 files / 1338 tests pass; 11 snapshots updated to reflect Toastrole/aria-liveand ColorPickeraria-label/aria-describedbychangeyarn workspace @vibe/dialog test --run— 5 files / 45 tests passyarn workspace @vibe/core lint— 0 errors (only pre-existing warnings)yarn workspace @vibe/core stylelint— 0 errors (only pre-existing design-token warnings)yarn workspace @vibe/dialog lint/stylelint— 0 errors🤖 Generated with Claude Code
PR Type
Bug fix
Description
Fixed ColorPicker dialog accessible name by replacing broken
aria-labelledbywitharia-labelResolved Toast conflicting live-region semantics by switching from
role="alert"torole="status"Restored keyboard focus indicators across seven components using
:focus:not(:focus-visible)patternDiagram Walkthrough
File Walkthrough
9 files
Replace bare focus outline with focus-visible patternReplace bare focus outline with focus-visible patternSimplify focus selector to focus-visible patternSimplify focus selector to focus-visible patternReplace bare focus outline with focus-visible patternReplace bare focus outline with focus-visible patternReplace bare focus outline with focus-visible patternFix broken aria-labelledby with proper aria-labelReplace conflicting alert role with status role