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

feat: Add global hotkey support #8906

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sinaru
Copy link

@sinaru sinaru commented Nov 2, 2024

Description

The video player does not currently support global hotkeys. Hotkeys only work when we have focus on the player. This PR is to introduce global hotkey support.

With this change, for example, if you land on the page with a video player, and does not have focus on any specific fields like form fields or the video player, you can perform a hotkey event like space bar key press which will play/pause video.

Specific Changes proposed

Global hotkeys are not turned on by default. To turn on, need to set userActions.globalHotkeys option to true. As below:

var player = videojs(vid, {
  userActions: {
    globalHotkeys: true,
    hotkeys: true
  }
});

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 (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
    • Has no DOM changes which impact accessiblilty or trigger warnings (e.g. Chrome issues tab)
    • Has no changes to JSDoc which cause npm run docs:api to error
  • Reviewed by Two Core Contributors

Copy link

welcome bot commented Nov 2, 2024

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@sinaru sinaru changed the title Introduce Global hotkey support Introduce global hotkey support Nov 2, 2024
@sinaru sinaru changed the title Introduce global hotkey support feat: Add global hotkey support Nov 2, 2024
@sinaru sinaru force-pushed the global-hotkey-support branch from f021ba3 to 461bab0 Compare November 2, 2024 18:40
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.23%. Comparing base (ecef37c) to head (461bab0).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/js/player.js 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8906      +/-   ##
==========================================
- Coverage   83.25%   83.23%   -0.02%     
==========================================
  Files         120      120              
  Lines        8097     8106       +9     
  Branches     1944     1947       +3     
==========================================
+ Hits         6741     6747       +6     
- Misses       1356     1359       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mister-ben
Copy link
Contributor

This could use some tests. Especially to cover cases where the a keypress on other elements shouldn't trigger the action.

@sinaru
Copy link
Author

sinaru commented Dec 19, 2024

Hi @mister-ben sorry for getting back to you late. Yes I agree. I tried to add a test and wanted to run locally to check the test but I couldn't figure out how to run the specific test in isolation. Is there a specific command we can use so we can just tell test file path and line number? The only thing I have is a way to run the whole test suite. This is not practical when writing the test.

@mister-ben
Copy link
Contributor

If you change QUnit.test() to QUnit.only(), only that test runs.

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.

2 participants