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.
|
@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?
2 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.