Skip to content

fix(a11y): repair broken aria-label, conflicting Toast role, and missing focus indicators #3374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
orrgottlieb wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(a11y): repair broken aria-label, conflicting Toast role, and missing focus indicators #3374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/components/dialog/src/Dialog/components/DialogContent/DialogContent.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
pointer-events: none;
}

.contentComponent:focus {
.contentComponent:focus:not(:focus-visible) {
outline: none;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/components/dialog/src/DialogContentContainer/DialogContentContainer.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.dialogContentContainer:focus {
.dialogContentContainer:focus:not(:focus-visible) {
outline: none;
}

Expand Down
4 changes: 1 addition & 3 deletions packages/core/src/components/AvatarGroup/AvatarGroupCounterTooltipContent.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
overflow-x: visible;
overflow-y: auto;
margin-top: var(--space-8);
&:focus,
&:focus-visible,
&.focus-visible { /* stylelint-disable-line selector-class-pattern */
&:focus:not(:focus-visible) {
outline: none;
}
}
Expand Down
4 changes: 1 addition & 3 deletions ...re/src/components/AvatarGroup/AvatarGroupCounterTooltipContentVirtualizedList.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
display: flex;
overflow-y: hidden;
overflow-x: visible;
&:focus,
&:focus-visible,
&.focus-visible { /* stylelint-disable-line selector-class-pattern */
&:focus:not(:focus-visible) {
outline: none;
}
}
2 changes: 1 addition & 1 deletion .../components/BreadcrumbsBar/BreadcrumbItem/BreadcrumbContent/BreadcrumbContent.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ a.breadcrumbContent:hover {
flex-shrink: 0;
}

.breadcrumbContent:focus {
.breadcrumbContent:focus:not(:focus-visible) {
outline: none;
}

Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/components/ColorPicker/ColorPicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,7 @@ const ColorPicker = forwardRef(
<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}
Comment on lines 132 to 137

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

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

>
Comment on lines 132 to 138

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

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

Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/components/ColorPicker/__tests__/__snapshots__/ColorPicker.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

exports[`renders correctly with empty props 1`] = `
<div
aria-describedby="Pick color"
aria-labelledby="Color Picker Dialog"
aria-describedby=""
aria-label="Color Picker Dialog"
aria-labelledby=""
className="dialogContentContainer colorPicker colorPickerDialogContent typePopover sizeSmall"
data-testid="dialog-content-container"
data-vibe="ColorPicker"
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/components/Menu/Menu/Menu.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
position: relative;
}

.menu:focus {
.menu:focus:not(:focus-visible) {
outline: none;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/components/Tabs/Tab/Tab.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
color: var(--primary-text-color);
}

.tabWrapper .tabInner:focus {
.tabWrapper .tabInner:focus:not(:focus-visible) {
outline: none;
}

Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/components/Toast/Toast.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,7 @@ const Toast = ({
element="div"
color="fixedLight"
className={classNames}
role="alert"
aria-live="polite"
role="status"
>
{iconElement && <div className={cx(styles.icon)}>{iconElement}</div>}
<Flex align="center" gap="large" className={styles.content}>
Expand Down
30 changes: 10 additions & 20 deletions packages/core/src/components/Toast/__tests__/__snapshots__/Toast.snapshot.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

exports[`Toast renders correctly > (renders nothing) with empty props 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNormal"
data-testid="toast"
role="alert"
role="status"
>
<div
className="icon"
Expand Down Expand Up @@ -91,10 +90,9 @@ exports[`Toast renders correctly > (renders nothing) with empty props 1`] = `

exports[`Toast renders correctly > and don't renders close button if closeable=false 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNormal"
data-testid="toast"
role="alert"
role="status"
>
<div
className="icon"
Expand Down Expand Up @@ -144,10 +142,9 @@ exports[`Toast renders correctly > and don't renders close button if closeable=f

exports[`Toast renders correctly > renders nothing when open is false 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNormal"
data-testid="toast"
role="alert"
role="status"
>
<div
className="icon"
Expand Down Expand Up @@ -235,10 +232,9 @@ exports[`Toast renders correctly > renders nothing when open is false 1`] = `

exports[`Toast renders correctly > when icon is hidden 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNormal"
data-testid="toast"
role="alert"
role="status"
>
<div
className="container directionRow justifyStart alignCenter content"
Expand Down Expand Up @@ -305,10 +301,9 @@ exports[`Toast renders correctly > when icon is hidden 1`] = `

exports[`Toast renders correctly > when open is true 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNormal"
data-testid="toast"
role="alert"
role="status"
>
<div
className="icon"
Expand Down Expand Up @@ -396,10 +391,9 @@ exports[`Toast renders correctly > when open is true 1`] = `

exports[`Toast renders correctly > with button 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNormal"
data-testid="toast"
role="alert"
role="status"
>
<div
className="icon"
Expand Down Expand Up @@ -502,10 +496,9 @@ exports[`Toast renders correctly > with button 1`] = `

exports[`Toast renders correctly > with button and link 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNormal"
data-testid="toast"
role="alert"
role="status"
>
<div
className="icon"
Expand Down Expand Up @@ -627,10 +620,9 @@ exports[`Toast renders correctly > with button and link 1`] = `

exports[`Toast renders correctly > with link 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNormal"
data-testid="toast"
role="alert"
role="status"
>
<div
className="icon"
Expand Down Expand Up @@ -737,10 +729,9 @@ exports[`Toast renders correctly > with link 1`] = `

exports[`Toast renders correctly > with loading 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNormal"
data-testid="toast"
role="alert"
role="status"
>
<div
className="icon"
Expand Down Expand Up @@ -855,10 +846,9 @@ exports[`Toast renders correctly > with loading 1`] = `

exports[`Toast renders correctly > with negative type 1`] = `
<div
aria-live="polite"
className="typography fixedLight start singleLineEllipsis text text2Normal toast typeNegative"
data-testid="toast"
role="alert"
role="status"
>
<div
className="icon"
Expand Down
Loading