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

Close button supports Esc key pressed #6050

Merged
merged 26 commits into from Jun 18, 2019

Conversation

gjanblaszczyk
Copy link
Member

Description

The PR fixes a potential regression that was introduced in v7.5.5.
The Modal window can't be closed via Esc key pressed when the close button is focused.

To reproduce the problem:

  1. open a modal window.
  2. use tab key to focus a close button.
  3. when a close button is focused then press Esc button.

Specific Changes proposed

Overrides handleKeyDown method inside CloseButton.
The method handles Esc key pressed and triggers click event.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

Though, would you be able to revert the package-lock.json changes?

@gkatsev gkatsev added needs: LGTM Needs an additional approval tested labels Jun 17, 2019
@gjanblaszczyk
Copy link
Member Author

Yes, of course. I have reverted package-lock.json.

@misteroneill misteroneill added needs: LGTM Needs an additional approval and removed needs: LGTM Needs an additional approval labels Jun 18, 2019
@gkatsev gkatsev added confirmed and removed needs: LGTM Needs an additional approval labels Jun 18, 2019
@gkatsev gkatsev merged commit f5fd94f into videojs:master Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants