-
Notifications
You must be signed in to change notification settings - Fork 355
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(Tabs): make tooltips more accessible #6940
Conversation
Preview: https://patternfly-react-pr-6940.surge.sh A11y report: https://patternfly-react-pr-6940-a11y.surge.sh |
I'd love to have @jessiehuff test this with screen reader. I feel like i'm getting inconsistent results when it comes to having the screen reader announce the tooltip - but I don't trust that my approach is consistent enough to be sure it's not user error. |
@nicolethoen are both examples not working as expected, or is there only an issue with one of the examples? |
This needs to be rebased to fix conflicts, but I believe that i'm just seeing a VO user error, not a code error. I'm good with it if Jessie or someone sanity checks it |
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.
So in testing, I noticed that with VO, the default and tooltip react ref examples work (the react ref example is a little wonky in Chrome in that in only appears the second time you get to it, as I think you mentioned in your description, but it works in Safari for me). However, all other examples don't seem to work for me in Chrome or Safari (i.e. uncontrolled, box light variation, and vertical). This is definitely an improvement to what we have right now though!!
6aa1d36
to
938144b
Compare
938144b
to
d747f64
Compare
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.
I'm no VO expert by any means but this looks good from an implementation standpoint IMO and based on my testing by tabbing and using VO in FF, Chrome, and Safari things seem to be working well to me.
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.
Really awesome job with this! 🏆 🎉
What: Closes #6311
Due to it seeming like there are issues when utilizing React refs to add aria attributes, I opted to include another way for linking a tooltip with a Tab component. The issue with React refs + Tooltip component seems to be that a) depending on the structure, a Tooltip may not render unless a user presses Enter on their keyboard (which this issue mentions), and b) Voice Over may not immediately announce the Tooltip (I've found that you have to focus the Tab, leave the Tab, then focus the same Tab again just to get VO to announce the Tooltip text contents, despite the fix to refs introduced here).
The original way of using a React ref + the Tooltip's
reference
prop will still work (with instructions indicating that an ID must be manually passed into the Tooltip, with the Tab having the same ID passed into thearia-describedby
attribute).The additional way introduced here essentially just requires passing in the actual Tooltip component to a
tooltip
prop of the Tab component. This resolves both the extraneous keyboard interaction and the screen reader issue present in the React ref approach, as it wraps the inner TabButton component as a child inside the Tooltip itself.Additional issues:
N/A