-
Notifications
You must be signed in to change notification settings - Fork 922
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
USWDS - Tooltip: Make tooltip dismissible with escape key #5909
Conversation
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.
Added suggestions. Thanks for creating tests.
- will be added to #5919 instead
@mejiaj @mahoneycm I updated the code based on your recommendations and our offline discussions. This is ready for re-review. Let me know what you think! |
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.
Looks good. I was able to manually test hover, click, and hitting escape closes the tooltips.
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.
This looks good to me!
I did want to flag that it looks like the escape
event listener is active when there are no active tooltips. Adding a console.log
within the handleEscape
shows that the function will trigger on escape.
Is that still a requirement for this functionality? I believe we decided it would be better to add the event listener once rather than every time the triggers are hovered but I wanted to make sure since it was listed in the checklist items.
Testing checklist
- Confirm that active tooltips close when you press
escape
. - Confirm no regressions in behavior.
- Confirm the code meet standards.
- Confirm the
escape
event listener is only added when the tooltip is active- Escape event listener is triggered anytime escape is pressed
- Tooltip escape functionality works as expected in sandbox
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.
Storybook focus quirk
I was able to confirm that clicking into the iframe storybook preview also allows the escape functionality to work without needing to tab to a trigger button.
I think this is because the second body element within the iframe isn’t capturing the keyboard interactions until the relative "browser focus" is put inside the iframe
- Not needed for this PR; will be added in #5909
@amycole501 and @alex-hull can you confirm that the tooltip now conforms with the "dismissible" criteria in WCAG 1.4.13? |
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.
@amyleadem yes it works great!! I was able to use the esc key to close the tooltip on all examples.
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.
Can confirm with Amy C, it should now conform with the dismissible portion of the criteria.
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.
Thanks all!
Summary
Updated tooltip component behavior to close active tooltips when the
escape
key is pressed. This allows the component to meet the "dismissible" standard outlined in WCAG 1.4.13.Note
This issue is slated to be in the same release as #5919, which also edits the tooltip JavaScript. We should confirm there are no conflicts between the two PRs.
Breaking change
This is not a breaking change.
Related issue
Closes #5901
Related pull requests
Changelog PR
Preview link
Tooltip component
Important
In the default storybook view, pressing
escape
will not close a tooltip that is triggered bymouseover
until one of the trigger buttons receives focus. After that, it should work as expected. However, this issue does not happen when the Story canvas is opened in its own tab. For that reason, I do not consider this to be an issue with the component, but rather Storybook.Problem statement
According to W3C, the tooltip component interaction should be dismissible by hitting the "escape" key.
Solution
This PR creates the
handleEscape()
function to hide the tooltip when when theescape
key is pressed.Testing and review
escape
.