feat(components): Improve Tooltip + Icon Button Accessibility#2907
feat(components): Improve Tooltip + Icon Button Accessibility#2907
Conversation
Deploying atlantis with
|
| Latest commit: |
73f21f7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://37521264.atlantis.pages.dev |
| Branch Preview URL: | https://job-149074-icon-button-acces.atlantis.pages.dev |
| const size = sizeProp || contextSize; | ||
|
|
||
| return <Icon {...props} size={size} />; | ||
| return <Icon {...props} size={size} aria-hidden={"true"} />; |
There was a problem hiding this comment.
We're only saying if it's a Button with icon (and thus ariaLabel is required), the svg does not provide any usage to the screen reader? What about role="presentation"?
There was a problem hiding this comment.
sort of.
unfortunately ariaLabel is only required with one syntax
<Button icon="info" ariaLabel="Find out more" />
if you try to remove ariaLabel types will yell at you.
<Button>
<Button.Icon name="info"/>
</Button>
the types are unable to know what is being passed in as children, so this is fine according to types, but it is completely identical to the previous invocation and exhibits the precise issue the types are trying to prevent :/
it's a blind spot we have with the new sub component syntax
There was a problem hiding this comment.
an svg can provide useful information to a screen reader. you can give it an alt or like you're sayingrole or other things, including aria labels!
however, in this context (a Button) we're not relying on the Icon to do that
the Button should have an aria label, or the tooltip. one of those should be describing this. the Icon is purely there for show.
then if the Button has text, that should have a good label that describes the purpose and you wouldn't look to the icon to be responsible for making it make sense.
There was a problem hiding this comment.
for reference here's the guide I've been reading to inform my decisions
https://www.sarasoueidan.com/blog/accessible-icon-buttons/
there's a section Icon Sitting Next to Text that covers the other scenario I described
I did forget to add the IE focusable false, but we don't support IE anymore.
There was a problem hiding this comment.
thats fair, thanks for the additional context
| */ | ||
| readonly testID?: string; | ||
|
|
||
| readonly "aria-hidden"?: AriaAttributes["aria-hidden"]; |
There was a problem hiding this comment.
Should we add JSDoc like we do for the aria props in AriaInputProps?
There was a problem hiding this comment.
I don't see why not. added.
edison-cy-yang
left a comment
edison-cy-yang
left a comment
There was a problem hiding this comment.
Tested and confirmed Tooltip with Button (and icon prop) only gets aria label read out once by screen reader
2 participants
Motivations
currently, this will get read out 2x on a screen reader
additionally, it will also announce "image" due to the svg not being hidden.
this is because Button with only the
iconprop makesariaLabelrequired, to provide accessibility. the issue is that Tooltip silently injectsaria-descriptiononto the element it is wrapping along with injecting a tabindex. we should not be doing this, but changing it is a bit much for the purposes of this task.Changes
Added
added aria-hidden on Icon, so that we can tell screen readers to ignore the image when it's not of use, which is always. it can't read an svg or turn it into something helpful.
Changed
swapped aria-description for aria-labelledby inside Tooltip. this is because aria-labelledby takes precedence over aria-label, thus ensuring that the duplication in this scenario results in only one accessible label
this will have very widespread effects literally every single Tooltip is currently receiving these injected attributes, making it impossible to know where it introduces issues. some instances may have caught this issue and created a solution in their spot, and now this could break it.
Deprecated
Removed
Fixed
Security
Testing
using voiceover...
try the combination of Tooltip and Button, both the single tag invocation and sub component version. note that the Button.Icon invocation doesn't have this problem because ariaLabel isn't required, however that creates a separate problem where we likely have inaccessible Icon Buttons in the product.
try Tooltip with some other elements, it should still read out a nice, single value of what you have in your Tooltip. it's possible that this will conflict with other values totally depending on the combination, which is part of the problem with this pattern. we have no idea what we are injecting and possibly overwriting with this.
Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.