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

[a11y] Remove space/enter onKeyDown handling for open/close keyboard shortcuts panel #1464

Merged
merged 2 commits into from
Nov 14, 2018
Merged

[a11y] Remove space/enter onKeyDown handling for open/close keyboard shortcuts panel #1464

merged 2 commits into from
Nov 14, 2018

Conversation

cshaver
Copy link
Contributor

@cshaver cshaver commented Nov 12, 2018

Summary

This is to fix an issue where trying to close the keyboard shortcuts panel with the Space and Enter keys doesn't work in Firefox or IE. Pressing space opens and closes the panel immediately and pressing enter seems to do nothing.

As far as I can tell it looks like the onKeyDown event is handled on the close button, but then because the focus is set to the "?" button that opens the panel, the corresponding onKeyUp event triggers an onClick and re-opens the panel.

So – I've removed the Enter and Spacebar handling from the close button's onKeyUp handler, which should just allow onClick to do its thing. Similarly, I removed the onKeyDown handler on the open button (if there's more context on disabling the enter key for that button let me know, but that seemed weird to me).

Gifs

Using keycastr to display keystrokes:

  • = Space
  • = Enter
  • = Esc

Before

before

After

after

Reviewers 🙏

@majapw @backwardok

@ljharb ljharb added the accessibility Anything related to ensuring *everybody* can use react-dates <3 label Nov 12, 2018
@coveralls
Copy link

coveralls commented Nov 12, 2018

Coverage Status

Coverage increased (+0.02%) to 85.071% when pulling ca4b67c on cshaver:crit--keyboard-close-shortcuts-panel into e76e308 on airbnb:master.

Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

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

Seems legit! Thanks for the fix. :)

@majapw majapw merged commit 9426234 into react-dates:master Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Anything related to ensuring *everybody* can use react-dates <3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants