-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[core] fix(Icon): reference a11y name via aria-labelledby #5365
Conversation
width={size} | ||
height={size} | ||
viewBox={viewBox} | ||
aria-labelledby={title ? titleId : undefined} |
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.
why not just use aria-label
? the <title>
is not visible on the page anyway, so I'm not sure that aria-labelledby
is the best approach here.
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.
an item does not necessarily have to be visible for aria-labelledby
to reference it and its text contents. In this case, I am following the most common examples of how labelling an SVG is done.
https://www.tpgi.com/using-aria-enhance-svg-accessibility/
https://www.unimelb.edu.au/accessibility/techniques/accessible-svgs
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.
alright, that 1st example seems quite outdated but the 2nd seems alright. still I prefer to go by what MDN says. I've verified that this is legit, according to item 5 here:
The
aria-labelledby
property value can include content from elements that aren't even visible.
width={size} | ||
height={size} | ||
viewBox={viewBox} | ||
aria-labelledby={title ? titleId : undefined} |
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.
why not just use aria-label
? the <title>
is not visible on the page anyway, so I'm not sure that aria-labelledby
is the best approach here.
width={size} | ||
height={size} | ||
viewBox={viewBox} | ||
aria-labelledby={title ? titleId : undefined} |
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.
alright, that 1st example seems quite outdated but the 2nd seems alright. still I prefer to go by what MDN says. I've verified that this is legit, according to item 5 here:
The
aria-labelledby
property value can include content from elements that aren't even visible.
Fixes #0000
Checklist
Changes proposed in this pull request:
Improve a11y for
Icon
elements (when has title, label thesvg
so screen readers can read the title)Reviewers should focus on:
Examples outlined here: https://www.tpgi.com/using-aria-enhance-svg-accessibility/ and here: https://www.unimelb.edu.au/accessibility/techniques/accessible-svgs