Skip to content
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

Tooltip: Ensure tooltip text is correctly announced by screenreaders #76683

Merged
merged 7 commits into from
Oct 18, 2023

Conversation

ashharrison90
Copy link
Contributor

@ashharrison90 ashharrison90 commented Oct 17, 2023

What is this feature?

  • adds the appropriate aria-describedby to the tooltip trigger
  • adds the tooltip role to the tooltip
  • removes the delayShow as this prevents the tooltip text being announced correctly by screenreaders (see https://raintank-corp.slack.com/archives/C025WTVRBSN/p1697541819161599 for context)
  • adds a unit test to check the description is being set correctly

Why do we need this feature?

  • so tooltip text is accessible to screenreaders

Who is this feature for?

  • everyone!

Which issue(s) does this PR fix?:

For #57207

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

ashharrison90 and others added 2 commits October 17, 2023 11:40
Co-authored-by: L-M-K-B <48948963+L-M-K-B@users.noreply.github.com>
Co-authored-by: joshhunt <josh@trtr.co>
@ashharrison90 ashharrison90 added type/bug type/accessibility Accessibility problem / enhancement mob session Something solved during a mob session no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Oct 17, 2023
@ashharrison90 ashharrison90 added this to the 10.2.x milestone Oct 17, 2023
@ashharrison90 ashharrison90 self-assigned this Oct 17, 2023
@ashharrison90 ashharrison90 requested a review from a team as a code owner October 17, 2023 12:06
@ashharrison90 ashharrison90 requested review from tskarhed and JoaoSilvaGrafana and removed request for a team October 17, 2023 12:06
@joshhunt joshhunt added add to changelog and removed no-changelog Skip including change in changelog/release notes labels Oct 17, 2023
Copy link
Contributor

@joshhunt joshhunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supportive of this 👍🏻

would like to test, especially with IconButton. I think there might be some automatic aria-label behaviour we can remove from it now.

@ashharrison90 ashharrison90 requested a review from a team as a code owner October 17, 2023 16:22
@ashharrison90 ashharrison90 requested review from asimpson, bossinc, aangelisc and alyssabull and removed request for a team October 17, 2023 16:22
@grafana-pr-automation grafana-pr-automation bot added the datasource/Azure Azure Monitor Datasource label Oct 17, 2023
@ashharrison90
Copy link
Contributor Author

@joshhunt tested removing the aria-label from IconButton, problem with that is it breaks a lot of tests since they will do something like screen.getByRole('button', { name: <whateverTheAriaLabelWas> }), and since it doesn't set the aria-describedby unless the tooltip is visible, there is no accessible name for the button until the tooltip is visible (i.e. it's focused).

not sure how to solve that really 🤔

@joshhunt
Copy link
Contributor

@ashharrison90 before you removed aria-label from IconButton, are screen readers double announcing the tooltip content? What's the impact of "doing nothing" for IconButton?

Maybe we could force (uncontrolled?) tooltips open when running in unit tests? 😬

Or, in Tooltip we could hackily inspect the props of the children trigger and not apply aria-describedby if it has an aria-label?

@ashharrison90
Copy link
Contributor Author

@joshhunt yeah it double announces the tooltip content

Maybe we could force (uncontrolled?) tooltips open when running in unit tests? 😬

i think having a label present always might be useful in the actual product though. if you know you want to find a certain button immediately on a page, you don't want to have to tab through every button until you find it.

Or, in Tooltip we could hackily inspect the props of the children trigger and not apply aria-describedby if it has an aria-label?

this was my initial thought tbh. i'm not sure if it's that bad... i would kind of expect an aria-label to take precedence anyway. i've tested and this does work 🤔 lmk what you think!

Copy link
Contributor

@joshhunt joshhunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this and I think it's a good improvement.

Specifically though, this doesn't resolve the issue with the dashboard permissions page where it puts a Tooltip on an Icon

<Tooltip content="Here's some access control info">
  <Icon name="info-circle" />
</Tooltip>

This is reproducable in storybook with this markup. Haven't digged into it further yet, but perhaps aria-describedby cannot be put on a div? Maybe it only goes on interactive elements?

I think this kind of checks out - you would normally put an aria-label on an icon, so perhaps for <Tooltip><Icon /></Tooltip> aria-labelledby makes more sense? Unsure the best way to resolve this.........

@ashharrison90
Copy link
Contributor Author

@joshhunt good spot!

i don't know why tbh. looking at the tooltip spec, all it says is that the tooltip should have role="tooltip" and the trigger should have aria-describedby=<tooltipId>

and looking at the docs for aria-describedby says that it's applicable to all html elements 😕 although here it says only semantic elements or elements with a role attribute. setting role="button" for example then makes it announce correctly, but that doesn't feel like the correct role for a tooltip trigger - i don't think an appropriate role exists tbh.

either way i'll merge for now and we can discuss best approach going forward.

@ashharrison90
Copy link
Contributor Author

ashharrison90 commented Oct 18, 2023

actually @joshhunt if we set role="img" on the wrapping <div> in Icon that also fixes the problem. thoughts? 🤔

Copy link
Contributor

@joshhunt joshhunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Lets merge this now. It's a good enough stand alone improvement.

Created #76762 for the enhancement to Icon - it's seperate enough from this work.

Leave it up to you whether you want to actually close #57207 with this PR.

@ashharrison90 ashharrison90 modified the milestones: 10.2.x, 10.3.x Oct 18, 2023
@ashharrison90 ashharrison90 merged commit d632dd6 into main Oct 18, 2023
21 checks passed
@ashharrison90 ashharrison90 deleted the ash/tooltip-a11y branch October 18, 2023 15:05
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend datasource/Azure Azure Monitor Datasource mob session Something solved during a mob session no-backport Skip backport of PR type/accessibility Accessibility problem / enhancement type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants