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

'Esc' works for a vertical volume bar and menus #6046

Merged

Conversation

gjanblaszczyk
Copy link
Member

Description

Following #6004 , this PR adds support for 'Esc' functionality in the volume bar and menus.

Specific Changes proposed

The PR improves player accessibility by adding 'Esc' functionality to the volume panel and menu popups.

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

@gjanblaszczyk gjanblaszczyk changed the title 'Esc' works for a vertical volume bar and 'Esc' works for a vertical volume bar and menus Jun 12, 2019
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.

The switch from :hover to .vjs-hover is also so that if you hover over a non-inline volume control it won't show up, right?

src/js/menu/menu-button.js Show resolved Hide resolved
src/js/menu/menu-button.js Show resolved Hide resolved
src/js/control-bar/volume-panel.js Show resolved Hide resolved
@gkatsev gkatsev added the patch This PR can be added to a patch release. label Jul 29, 2019
@gjanblaszczyk
Copy link
Member Author

Hi @gkatsev, Yes, exactly. I had the same issue that was recently reported by marcelo on Slack. So I decided to fix it almost 2 months ago and this PR contains a fix for it.
btw Thanks for the review.

@gkatsev gkatsev added the tested label Aug 19, 2019

/**
* Handles `keydown|keyup` events on the document, looking for ESC, which closes
* the volume panel.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a copy/paste issue here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, nice catch! I have just fixed the comment.

src/js/control-bar/volume-panel.js Show resolved Hide resolved
*/
handleKeyPress(event) {
if (keycode.isEventKey(event, 'Esc')) {
this.removeClass('vjs-hover');
Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to remove the document-level listener here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I have just improved the method following your comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release. tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants