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

Allow tooltip to be dismissed using Escape key #2914

Merged
merged 1 commit into from Jul 26, 2022

Conversation

saghan
Copy link
Contributor

@saghan saghan commented Jul 25, 2022

This change is trying to enable users to dismiss tooltip using Escape key. According to Web Content Accessibility Guideline, content that appears on hover should be able to be dismissed without moving the mouse. Hopefully, this change will help Recharts users interact with the chart more easily through keyboard. P.S. The change in the test file also contains many style changes that have been automatically applied on save due to eslint rules in Recharts

Copy link

@cscrosati cscrosati left a comment

Choose a reason for hiding this comment

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

Looks good, @xile611 and @arcthur , what do you think? It resolves the "Dismissible" accessibility concern of a "Content on hover" requirement.

@cscrosati
Copy link

Note: this PR provides is supporting #2801

@arcthur arcthur merged commit fbf387b into recharts:master Jul 26, 2022
@colinmorelli
Copy link

I noticed in this PR that the tooltip is only closed if esc is pressed while the tooltip has focus. The result is if the tooltip is open, and (without moving the mouse) you click on the chart, and then tap escape, the tooltip doesn't close.

However, we're also running into another unintended consequence of having focus captured on hover. In our app, we have popovers that close on blur, so if you accidentally move your mouse too far outside of a popover and it lands on a chart, the tooltip will open and focus, which will then close the popover.

I think we can still keep the same accessibility behavior here by just binding an event listener to document.body when the tooltip is opened, and not capturing focus in the tooltip element itself. Would that work/would you accept a PR for this?

ckifer pushed a commit that referenced this pull request Apr 14, 2023
## Description
When the tooltip is displayed it becomes focused so it can be dismissed
by pressing the _escape_ key. However, by focusing the tooltip, several
issues occur on the apps using this lib.
In this pr, based on this
[comment](#2914 (comment)),
I replaced the dismissing handling from the `onKeyDrown` prop for a
event listener

## Related Issue
[Issues/#3063](#3063)

## Motivation and Context
This issue was afecting our product since when mousing over a chart, we
can no longer edit a pre focused input. Other users have complained
about similar issues like closing of modals (which close on blur event)

## How Has This Been Tested?
I tested the behaviour manualy using the storybook. I didn't add any
test since I didn't add a new feature.

## Screenshots (if appropriate):

## Types of changes
- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)

## Checklist:
- [X] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [X] All new and existing tests passed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants