fix(ui): add aria-labels to icon-only buttons for screen readers (#473) #579
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 02924bd in 2 minutes and 35 seconds. Click for details.
- Reviewed
275lines of code in17files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/settings/src/ui/settings-ui-wallet-export-mnemonic-button.tsx:16
- Draft comment:
Consider replacing hard-coded toast messages and prompt text with i18n translation keys for multilingual support. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. packages/tools/src/tools-feature-create-token.tsx:112
- Draft comment:
Consider using an i18n translation key for the Refresh button’s aria‑label instead of a hard-coded 'Refresh'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is suggesting a code quality improvement (using i18n). However, looking at the entire file, there's no evidence of i18n being used anywhere - all text is hard-coded. The comment singles out this one aria-label when there are dozens of other hard-coded strings in the same file. This seems inconsistent and not actionable without broader context about whether i18n is even set up in this project. The comment also doesn't provide clear guidance on what translation key to use or how to implement it. This appears to be a speculative suggestion rather than addressing a clear issue with the change. Perhaps i18n is a standard in this codebase that should be applied to all new strings, and this is a valid reminder. The comment might be highlighting a broader issue that the PR author should be aware of. If i18n were a standard in this codebase, we would expect to see it used elsewhere in this file, but we don't. The file has many hard-coded strings that weren't flagged. Without evidence that i18n is actually used in this project, this comment is speculative and not actionable. It's also not about a bug or clear code issue - it's a suggestion for a pattern that may not even be established in this codebase. This comment should be deleted. It suggests using i18n without any evidence that i18n is used in this codebase or file. The entire file contains hard-coded strings, and singling out this one aria-label is inconsistent. The comment is speculative and not actionable without broader context.
3. packages/settings/src/ui/settings-ui-network-list.tsx:45
- Draft comment:
Consider using a translation key for the confirmation dialog’s actionLabel instead of a static 'Delete'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/settings/src/ui/settings-ui-wallet-list-item.tsx:41
- Draft comment:
Consider using a translation key for the confirmation dialog’s actionLabel instead of a hard-coded 'Delete'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. packages/shell/src/ui/shell-ui-menu-actions-popup.tsx:28
- Draft comment:
For better async error handling, consider awaiting the async function call within the onClick handler (e.g. use an async arrow function with 'await openSidePanel(browser)'). - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_ntnLvWDEQWC8hdTn
You can customize 
| const { t } = useTranslation('ui') | ||
| return ( | ||
| <Button | ||
| aria-label="Copy" |
There was a problem hiding this comment.
Consider using a translation key (e.g. t(() => ...)) instead of a hard-coded 'Copy' for the aria-label to support i18n consistency.
| aria-label="Copy" | |
| aria-label={t(() => $.copy)} |
There was a problem hiding this comment.
request: please improve these aria-labels. They are not descriptive and not useful for screenreaders.
There was a problem hiding this comment.
These still do not feel descriptive to me.
- Where do we go back to?
- What dialog are we closing
- What are we copying to the clipboard?
- What are we refreshing?
There was a problem hiding this comment.
so these are aria-labels only for the buttons used, according to me those buttons are probably doing these things
1: we are going back to previous page or component
2: any dialog window where the button is used for closing
3: copying the displayed value that we are trying to copy using the copy button in ui
4: we are refreshing wallet address or anything where refresh is used
There was a problem hiding this comment.
Unfortunately probably is not going to be good enough here. The purpose for aria-labels is for screen readers for people with disabilities. Generic values are not going to be useful here. We need each aria-label to be specific for the action being performed.
There was a problem hiding this comment.
@tobeycodes i have updates the required ones accordingly
There was a problem hiding this comment.
@unsafe0x0 These aria-labels still need to be updated. I would recommend enabling the voice over utility on your device and navigating the wallet with your keyboard. You will hover over things and they will say "Go back" but a visually impaired user will not know where they are going back to.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-description
https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA8
|
@tobeycodes i think i have resolved the requested changes let me know if anything i need to change |
tobeycodes
left a comment
tobeycodes
left a comment
There was a problem hiding this comment.
request: merge conflicts need to be resolved
There was a problem hiding this comment.
These still do not feel descriptive to me.
- Where do we go back to?
- What dialog are we closing
- What are we copying to the clipboard?
- What are we refreshing?
There was a problem hiding this comment.
question: do you speak spanish or are you using google translate / llm here?
There was a problem hiding this comment.
let's use null then, so we can have them translated correctly
There was a problem hiding this comment.
so i have translated them using google translator
There was a problem hiding this comment.
google translate is arguably worse than an llm. per my previous comment can you use null?
|
Hey @unsafe0x0 - make sure to rebase your branch instead of merging We explained the steps here. Feel free to ask questions on Discord if you're stuck. |
unsafe0x0
force-pushed
the
unsafe0x0
branch
5 times, most recently
from
e2d03b8 to
6bbd2a2
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances accessibility by adding aria-label attributes to icon-only buttons throughout the application, making them accessible to screen readers and assistive technologies. Previously, some buttons relied on title attributes which are not reliably announced by screen readers.
Key Changes:
- Replaced
titleattributes witharia-labelattributes on icon-only buttons - Added new translation keys (
buttonBack,buttonClose,buttonCopy,buttonRefresh) with internationalization support - Updated TypeScript type definitions to include the new translation keys
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/components/ui-text-copy-button.tsx | Added aria-label to copy button with translation support |
| packages/ui/src/components/ui-experimental-warning.tsx | Added aria-label to close button for experimental warning |
| packages/ui/src/components/ui-back.tsx | Added aria-label to back navigation button |
| packages/ui/src/components/ui-back-button.tsx | Added aria-label to back button component |
| packages/shell/src/ui/shell-ui-menu-actions-sidebar.tsx | Replaced title with aria-label for sidebar hide button |
| packages/shell/src/ui/shell-ui-menu-actions-popup.tsx | Replaced title with aria-label for sidebar show button |
| packages/settings/src/ui/settings-ui-wallet-list-item.tsx | Replaced title with aria-label for edit and delete buttons |
| packages/portfolio/src/portfolio-feature-tab-activity.tsx | Added aria-label to refresh button |
| packages/i18n/src/resources.d.ts | Added TypeScript definitions for new translation keys |
| packages/i18n/locales/en/ui.json | Added English translations for UI button labels |
| packages/i18n/locales/es/ui.json | Added placeholder entries for Spanish UI translations |
| packages/i18n/locales/en/tools.json | Added English translation for tools keypair generation |
| packages/i18n/locales/es/tools.json | Added placeholder entry for Spanish tools translations |
| packages/i18n/locales/en/portfolio.json | Added English translation for portfolio refresh button |
| packages/i18n/locales/es/portfolio.json | Added placeholder entry for Spanish portfolio translation |
| packages/tools/package.json | Added i18n workspace dependency |
| bun.lock | Updated lock file with i18n dependency for tools package |
| const { t } = useTranslation('ui') | ||
| return ( | ||
| <Button asChild size="icon" variant="outline" {...props}> | ||
| <Button asChild size="icon" variant="outline" {...props} aria-label={t(($) => $.buttonBack)}> |
There was a problem hiding this comment.
The aria-label attribute is placed after the spread operator {...props}, which means any aria-label passed via props would override this accessibility label. The aria-label should be placed after the spread operator to ensure it takes precedence, or alternatively, move it before the spread but after other attributes.
Suggested fix:
<Button aria-label={t(($) => $.buttonBack)} asChild size="icon" variant="outline" {...props}>| @@ -1,4 +1,7 @@ | |||
| { | |||
| "buttonBack": "Go back to previous page", | |||
| "buttonClose": "Close experiment warning message", | |||
There was a problem hiding this comment.
The aria-label text "Close experiment warning message" is overly specific and verbose. Screen reader users typically understand that clicking a close button will close the current element. Consider simplifying this to just "Close" or "Close warning" for a better user experience.
For comparison, WCAG guidelines recommend keeping aria-labels concise and descriptive.
| "buttonClose": "Close experiment warning message", | |
| "buttonClose": "Close warning", |
| @@ -1,4 +1,7 @@ | |||
| { | |||
| "buttonBack": "Go back to previous page", | |||
There was a problem hiding this comment.
The aria-label "Go back to previous page" is verbose and may be redundant. Screen readers already provide context about navigation. Consider simplifying to just "Back" or "Go back" for a more concise experience while maintaining clarity.
| "buttonBack": "Go back to previous page", | |
| "buttonBack": "Back", |
|
@unsafe0x0 and I discussed this on Discord. It makes more sense for us to finish up this pull request to get the aria-labels correct across the codebase. As it is low priority I am closing for the time being but will re-use this branch when we are ready to pick it up again. We appreciate your contribution here and we are excited to see your future pull requests. |
3 participants
Description
fix: #473
This PR adds descriptive
aria-labelattributes to all icon-only buttons throughout the codebase to improve accessibility for screen readers and assistive technologies.Problem:
Icon-only buttons (buttons containing only SVG icons without visible text) were not accessible to screen readers. Some buttons used
titleattributes, which are not reliably read by assistive technologies, creating barriers for users relying on screen readers.Solution:
aria-labelattributes to all icon-only buttons with descriptive texttitleattributes witharia-label(title is not accessible to screen readers)Components Updated:
ui-back-button.tsx,ui-back.tsx)ui-experimental-warning.tsx)ui-text-copy-button.tsx)No visual changes - this is an accessibility-only improvement. The buttons look identical but are now accessible to screen readers. The
aria-labelattributes are only read by assistive technologies and do not affect the visual appearance.Important
Add
aria-labelattributes to icon-only buttons for screen reader accessibility and update translation keys for internationalization.aria-labelattributes to icon-only buttons inportfolio-feature-tab-activity.tsx,account-ui-export-secret-button.tsx, andsettings-ui-network-list.tsxfor screen reader support.titleattributes witharia-labelinshell-ui-menu-actions-popup.tsxandshell-ui-menu-wallets.tsx.buttonBackandbuttonCloseinlocales/en/ui.jsonandlocales/es/ui.json.resources.d.tsto include new translation keys.ui-back-button.tsxandui-back.tsx.ui-experimental-warning.tsx.ui-text-copy-button.tsx.This description was created by
for 02924bd. You can customize this summary. It will automatically update as commits are pushed.