-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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
fix(toolbar): assign label to icons #12734
Conversation
CodSpeed Performance ReportMerging #12734 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn’t use <title>
here as it introduces a duplicate tooltip for those using a mouse to hover:
I also did some screenreader testing and it appears these changes are not required — the buttons already include accessible labels in the accessibility tree when testing with the version on main
:
Instead, we should hide the SVGs using aria-hidden="true"
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @ematipico! They look good. Just one question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. OK, should be good to go I think! Thanks for bearing with me on this one 🙌
Changes
There were some accessibility issues related to the labels of icons and buttons in our toolbar. This PR should fix it.
Testing
CI should pass
Docs
N/A