fix: make sure hotkeys are not triggered outside the player or in form fields within the player #5969

misteroneill · 2019-04-29T15:44:30Z

Description

We saw some issues with the recently-added hotkeys feature.

The use of document-level keydown listeners caused players to respond to events they should not have when focus was not in the player.
Those keydown events which do reach the player are considered possible hotkey triggers. However, there are sometimes elements added to the player - such as in modals - that should not trigger hotkeys.

Specific Changes proposed

Rename any handleKeyPress method to handleKeyDown because that's more accurate. Define handleKeyPress as a Component method that delegates to handleKeyDown so that any existing uses of it should not break.
Do not bind/unbind listeners on focus/blur and stop listening for keydown events on the document. It's better to just listen for keydown events on the components themselves.
Add exceptions for certain elements by their tagName.
Call stopPropagation() in key places to prevent double keydown handling.

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
Reviewed by Two Core Contributors

src/js/big-play-button.js

src/js/clickable-component.js

src/js/control-bar/progress-control/seek-bar.js

src/js/menu/menu.js

src/js/player.js

gkatsev · 2019-04-29T16:57:34Z

src/js/player.js


-        this.handleHotkeys(event);
+    // Bail out if the user is focused on an interactive form element.
+    if (activeTag && hotkeysDisallowed.indexOf(activeTag) !== -1) {


I think this would prevent hotkeys from working in any of the control bar buttons, no?

I don't think so, I did try that, but I'll verify.

You're right. I must have added this filtering before actually verifying it! D'oh!

I guess you can verify if the element is contained in the control bar and then skip this check

Well, buttons are generally only activated on Space/Enter, which we pass through to the browser anyway. Other keys (e.g. m, f, k, etc) are unlikely to affect a button. Seems safe to allow buttons and blacklist them if it becomes an issue for whatever reason.

Potentially, though, you can also define buttons as an input tag. It may be worth doing the control bar check regardless of which elements we disallow.

True, true. The control bar check seems dirty, though. I'll ponder it.

src/js/slider/slider.js

misteroneill · 2019-05-02T20:40:40Z

I realized that userActions does not appear to have tests. I will rectify that in this PR.

OwenEdwards

This all LGTM, and also seems to address/fix #5774. The only thing is, I don't have an example of where it was causing problems, to see how the behavior compares after the fix, but I suppose you do?

And thanks for adding a userActions test! This feature could definitely use some automatic testing!

misteroneill · 2019-05-06T15:49:54Z

Glad to hear it. It's fairly easy to reproduce if you use a page with a Video.js player embedded. If you hit the big play button then move focus into a text field or something, you can activate hotkeys when typing into the field.

misteroneill · 2019-05-06T20:38:13Z

For some reason the tests are failing on BrowserStack. Will have to investigate tomorrow.

gkatsev · 2019-05-17T20:15:46Z

Clicking on the BPB or the poster initially no longer enables hotkeys.

…m fields

…m fields within the player (videojs#5969)

Change comprises of commenting out event.stopPropagation calls added in a recent bug fix. The following comment was added in affected places: Videojs 7.5.5 has added event stopPropagation throughout its components to fix some hotkeys behaviour, such as reacting to hotkeys insie of forms inside the player. This broke our keyhandling implemenation, preventing any remote key presses from being handled. See more here: - videojs@79eadac - videojs#5969 The reasons to stop propagation don't seem to apply to our usecase, so it should be safe to leave this commented out. If this causes problems in the future, the alternative solution would perhaps be to use rewrite our key handling to go through the hotkeys system: https://github.com/videojs/video.js/blob/cf6e0e824814f3ccb061b057a9aa5eff3b54ba6e/docs/guides/options.md#useractionshotkeys

misteroneill force-pushed the fix/hotkeys-focus branch from 20c94a5 to 033c5f1 Compare April 29, 2019 16:04

misteroneill requested review from OwenEdwards and gkatsev April 29, 2019 16:11

gkatsev reviewed Apr 29, 2019

View reviewed changes

misteroneill added the needs: updates label May 2, 2019

OwenEdwards approved these changes May 3, 2019

View reviewed changes

misteroneill force-pushed the fix/hotkeys-focus branch 2 times, most recently from 3553538 to cba07fc Compare May 6, 2019 20:20

misteroneill removed the needs: updates label May 7, 2019

misteroneill force-pushed the fix/hotkeys-focus branch from cba07fc to 1ef78cb Compare May 7, 2019 17:06

misteroneill force-pushed the fix/hotkeys-focus branch from d44166a to 3b0356e Compare May 28, 2019 15:15

misteroneill added 13 commits May 28, 2019 11:27

fix: Make sure hotkeys are not triggered outside the player or in for…

a81f1be

…m fields

fix tests

e9fde6d

remove commented code

2fae1c6

CR

734880f

CR also

31cdb41

Better exclusion logic for hotkeys elements

52d7073

Add player user actions tests

fdbb4a0

Add element include/exclude logic tests

e947c20

Mock fullscreen support

6e49d3e

Skip fullscreen tests

c4ed7b5

Bail

06e057f

Skip this one too

2b58ac9

Set focus to tech from BPB or poster

f15e4db

misteroneill force-pushed the fix/hotkeys-focus branch from 3b0356e to f15e4db Compare May 28, 2019 15:27

misteroneill changed the base branch from master to 7.5.x May 28, 2019 15:27

gkatsev approved these changes May 30, 2019

View reviewed changes

gkatsev merged commit d251e97 into 7.5.x May 30, 2019

gkatsev deleted the fix/hotkeys-focus branch May 30, 2019 15:16

gkatsev pushed a commit to gkatsev/video.js that referenced this pull request Jun 10, 2019

fix: make sure hotkeys are not triggered outside the player or in for…

79eadac

…m fields within the player (videojs#5969)

gkatsev mentioned this pull request Nov 30, 2020

Focus Not Trapped in Caption Settings Dialog #6950

Closed

OwenEdwards mentioned this pull request Dec 8, 2020

fix: keep focus trapping contained to modal #6983

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make sure hotkeys are not triggered outside the player or in form fields within the player #5969

fix: make sure hotkeys are not triggered outside the player or in form fields within the player #5969

misteroneill commented Apr 29, 2019 •

edited

Loading

gkatsev Apr 29, 2019

misteroneill Apr 29, 2019

misteroneill Apr 29, 2019

gkatsev Apr 29, 2019

misteroneill Apr 29, 2019

misteroneill Apr 29, 2019

gkatsev Apr 29, 2019

misteroneill Apr 29, 2019

misteroneill commented May 2, 2019

OwenEdwards left a comment

misteroneill commented May 6, 2019

misteroneill commented May 6, 2019 •

edited

Loading

gkatsev commented May 17, 2019

fix: make sure hotkeys are not triggered outside the player or in form fields within the player #5969

fix: make sure hotkeys are not triggered outside the player or in form fields within the player #5969

Conversation

misteroneill commented Apr 29, 2019 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill commented May 2, 2019

OwenEdwards left a comment

Choose a reason for hiding this comment

misteroneill commented May 6, 2019

misteroneill commented May 6, 2019 • edited Loading

gkatsev commented May 17, 2019

misteroneill commented Apr 29, 2019 •

edited

Loading

misteroneill commented May 6, 2019 •

edited

Loading