IconButton: Put tooltip text as aria-label #36760
Conversation
| @@ -36,7 +36,7 @@ export const IconButton = React.forwardRef<HTMLButtonElement, Props>( | |||
| const styles = getStyles(theme, size, variant); | |||
|
|
|||
| const button = ( | |||
| <button ref={ref} {...restProps} className={cx(styles.button, className)}> | |||
| <button ref={ref} aria-label={tooltip} {...restProps} className={cx(styles.button, className)}> | |||
There was a problem hiding this comment.
Not sure this is the best solution, most icon buttons don't have tooltip, so they still won't be accessible, I think a better way would be to add aria-label prop to the component, so the consumers could provide the labels according to their use case.
There was a problem hiding this comment.
What about taking the tooltip if there is no aria-label? That way we cover a lot of cases that already exist. You can always override it through restProps.
There was a problem hiding this comment.
I'm thinking that tooltip doesn't always correctly represent the button, content. For example when it's some help text, also sometimes tooltips are an html content (not in this case but might happen in the future). Could be an edge case though.
There was a problem hiding this comment.
I don't think it fits the use case for an icon button. Icon buttons are minimalist and should fit a very specific action. Maybe we can add guidelines about it.
There was a problem hiding this comment.
Not sure if this is the BEST approach, but using https://benjaminjohnson.me/blog/accessible-icon-buttons as a reference, probably adding a required label prop, adding its content next to the icon, and "hiding" the text to non-screenreader users via css might be a good approach. wdyt?
big plus imho: an accessible description of the "action" will be required
There was a problem hiding this comment.
maybe it would be interesting to go through a couple well known websites and see how they deal with icon-only buttons, it seems to me the web is full of them :-)
i did a quick check on gmail/google-sheets, and they went with aria-label, the github formatting tools right above the text i am writing right now do the same.
There was a problem hiding this comment.
@Clarity-89 true, then i see no problem in having it optional. maybe we can log a warning in the console before 9.0 if the props isn't used? not sure if we can add a linter rule for that.
There was a problem hiding this comment.
@gabor that's interesting, somehow i always thought using aria-label wasn't the proper way to solve this specific use case, but reading more about the subject it seems it actually is
There was a problem hiding this comment.
@Elfo404 yeah an optional property with a warning sounds good imo.
There was a problem hiding this comment.
I think the big issue here is that our tooltips are not WCAG compliant, otherwise they would've served this purpose.
There was a problem hiding this comment.
my 2c - although you can already specify an aria-label via restProps, think it makes sense to have a specific optional prop as it's more likely to remind a consumer to actually set it.
in terms of using tooltip, i think that would be fine as a backup. e.g. aria-label={ariaLabel ?? tooltip}. i agree with previous comments that it's not the best, but it's definitely better than nothing.
* Make tooltip prop aria-label * Add ariaLabel prop (cherry picked from commit 8af83b8)
* Make tooltip prop aria-label * Add ariaLabel prop (cherry picked from commit 8af83b8) Co-authored-by: Tobias Skarhed <[email protected]>
What this PR does / why we need it:
A lot of IconButtons don't have an aria-label property, but has a tooltip. This is inaccessible and Axe complains about it. Until we can make Tooltips a very accessible solution that possibly covers this use case, I'd suggest doing this.