Icon buttons get their accessible name; the chat's close is "Close chat" - #1219
Conversation
IconButton wraps its Tooltip around a <span> that wraps the real MuiIconButton, so MUI's own
aria-label (derived from `title`) landed on the span — a wrapper nothing focuses or reads — and
every icon-only button in the app was nameless: to a screen reader, and to the e2e suite's
getByRole('button', { name }), which found "New chat" (a text button) and "Usage" but neither
the chat's pop-out, its new-chat, nor its close. The button itself now carries aria-label from
a string title; a ReactNode title is left to the Tooltip as before.
The chat panel's close button is titled "Close chat" (new key chat.closeChat) rather than
"Close": the title is the button's name, and a bare "Close" is ambiguous beside every other
close on the page — for assistive tech, and for the suite, which dismisses a docked chat by that
name. Context: a DEV build (PENDING_FEATURES) and the AI portal open the chat by default, and at
1280px that docks 400px and folds the sidebar into the hamburger by the layout's own rule — so a
suite that keys on the sidebar's Devices link needs a way to put the sidebar back.
typecheck clean, frontend 49/49, i18n:check + extract-idempotency clean, vite build green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a78da0ec3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // button, so MUI's own aria-label landed on the span — a wrapper nothing focuses or reads | ||
| // — and every icon button in the app was nameless to assistive tech and to the e2e suite's | ||
| // getByRole('button', { name }). Name the button itself; a string title is the label. | ||
| aria-label={typeof title === 'string' ? title : undefined} |
There was a problem hiding this comment.
When an IconButton uses the supported ReactNode form of title, this guard still omits its accessible name. For example, ServiceKeySetting.tsx passes a fragment containing “Get the Node.js package” and an icon, so that GitHub button remains unnamed to screen readers despite this change's generic accessibility fix. Either require a separate string label or derive one for these non-string titles.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a78da0ec3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // button, so MUI's own aria-label landed on the span — a wrapper nothing focuses or reads | ||
| // — and every icon button in the app was nameless to assistive tech and to the e2e suite's | ||
| // getByRole('button', { name }). Name the button itself; a string title is the label. | ||
| aria-label={typeof title === 'string' ? title : undefined} |
There was a problem hiding this comment.
When an IconButton uses the supported ReactNode title form—such as the fragment passed by ServiceKeySetting.tsx:104-109—this condition supplies no label. Because the tooltip labels its wrapper <span> rather than the nested button, that GitHub button remains unnamed to assistive technology; accept a separate string accessible label or otherwise ensure non-string tooltip titles also name the button.
Useful? React with 👍 / 👎.
| // button, so MUI's own aria-label landed on the span — a wrapper nothing focuses or reads | ||
| // — and every icon button in the app was nameless to assistive tech and to the e2e suite's | ||
| // getByRole('button', { name }). Name the button itself; a string title is the label. | ||
| aria-label={typeof title === 'string' ? title : undefined} |
There was a problem hiding this comment.
Some callers use title as a disabled-state explanation rather than the control's name: for example, RegisterMenu.tsx:73-85 changes the add-device button's title to “Manage permission required…” when unauthorized, and ProductsActionBar.tsx:72-80 similarly replaces “Delete selected” with “Admin permissions required.” Mapping every string title directly to aria-label makes those buttons lose any accessible indication of what action they represent; provide a stable action label separately and expose the permission text as a description.
Useful? React with 👍 / 👎.
Two shapes of title the string-title rule could not name, both raised in review:
• a React-node title — ServiceKeySetting's "Get the Node.js package" + launch icon — named
nothing (that button had been nameless before this branch too, but the fix should reach it);
• a title that swaps in a disabled-state EXPLANATION — RegisterMenu's "Manage permission
required…", ProductsActionBar's "Admin permissions required" — would have become the name
of what the button DOES.
`label` is the control's stable accessible name and wins over the title; the three call sites
pass it (ConfirmIconButton forwards it). The explanation stays visible in the tooltip.
typecheck clean, frontend 49/49, no new i18n keys, vite build green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fdd371793
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // landed on the span — a wrapper nothing focuses or reads — and every icon button in the | ||
| // app was nameless to assistive tech and to the e2e suite's getByRole('button', { name }). | ||
| // `label` wins; otherwise a string title is the name; a node title names nothing here. | ||
| aria-label={label ?? (typeof title === 'string' ? title : undefined)} |
There was a problem hiding this comment.
When a titled copy control succeeds, this derives its accessible name from the temporary feedback tooltip: CopyIconButton.tsx:59 replaces titles such as “Copy command” with “Copied!” for 800 ms, and ScriptEditPage.tsx:172 does the same for two seconds. The button remains actionable during that interval, but screen-reader and voice-control users can no longer identify it by its copy action, and role-based automation can transiently lose the locator. Pass a stable label for these controls while allowing title to display the status feedback.
Useful? React with 👍 / 👎.
… "Copied!" (Codex) CopyIconButton swaps its title for "Copied!" for 800ms (ScriptEditPage's copy for two seconds); with the title now the button's accessible name, the control was briefly renamed to a status — findable by neither screen reader, voice control nor a role-based locator. The stable `label` carries the action; the tooltip keeps the feedback. The remaining "Copied!" title in the tree (ListItemCopy) is a ListItemButton named by its visible text, not an IconButton. typecheck clean, frontend 49/49, vite build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What
IconButton: the button itself now carriesaria-labelfrom a stringtitle. The Tooltip wraps a<span>around the realMuiIconButton, so MUI's derivedaria-labellanded on the span — a wrapper nothing focuses or reads — and every icon-only button in the app was nameless to assistive tech (and togetByRole('button', { name })).chat.closeChat) instead of "Close" — the title is the button's name, and a bare "Close" is ambiguous beside every other close on the page.Why now
A dev build (
PENDING_FEATURES) and the AI portal open the chat by default; at 1280px that docks 400px and folds the sidebar into the hamburger by the layout's own rule. The e2e suite keys on the sidebar's Devices link, so it needs to be able to dismiss a docked chat by name — which required the button to have a name. Companion: e2e-tests (running the suite genuinely againstlocalhost).Verified
typecheck clean · frontend 49/49 ·
i18n:check+ extraction idempotent · vite build green · e2eauth+multi-accountpass against a local build and against app.dev.🤖 Generated with Claude Code