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

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

Merged
merged 13 commits into from
May 30, 2019
9 changes: 4 additions & 5 deletions src/js/big-play-button.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,15 @@ class BigPlayButton extends Button {
// exit early if clicked via the mouse
if (this.mouseused_ && event.clientX && event.clientY) {
silencePromise(playPromise);
// call handleFocus manually to get hotkeys working
this.player_.handleFocus({});
this.player_.tech(true).focus();
return;
}

const cb = this.player_.getChild('controlBar');
const playToggle = cb && cb.getChild('playToggle');

if (!playToggle) {
this.player_.focus();
this.player_.tech(true).focus();
return;
}

Expand All @@ -69,10 +68,10 @@ class BigPlayButton extends Button {
}
}

handleKeyPress(event) {
handleKeyDown(event) {
this.mouseused_ = false;

super.handleKeyPress(event);
super.handleKeyDown(event);
}

handleMouseDown(event) {
Expand Down
19 changes: 13 additions & 6 deletions src/js/button.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,13 +104,20 @@ class Button extends ClickableComponent {
*
* @listens keydown
*/
handleKeyPress(event) {
// Ignore Space or Enter key operation, which is handled by the browser for a button.
if (!(keycode.isEventKey(event, 'Space') || keycode.isEventKey(event, 'Enter'))) {

// Pass keypress handling up for unsupported keys
super.handleKeyPress(event);
handleKeyDown(event) {

// Ignore Space or Enter key operation, which is handled by the browser for
// a button - though not for its super class, ClickableComponent. Also,
// prevent the event from propagating through the DOM and triggering Player
// hotkeys. We do not preventDefault here because we _want_ the browser to
// handle it.
if (keycode.isEventKey(event, 'Space') || keycode.isEventKey(event, 'Enter')) {
event.stopPropagation();
return;
}

// Pass keypress handling up for unsupported keys
super.handleKeyDown(event);
}
}

Expand Down
81 changes: 22 additions & 59 deletions src/js/clickable-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,13 @@
*/
import Component from './component';
import * as Dom from './utils/dom.js';
import * as Events from './utils/events.js';
import * as Fn from './utils/fn.js';
import log from './utils/log.js';
import document from 'global/document';
import {assign} from './utils/obj';
import keycode from 'keycode';

/**
* Clickable Component which is clickable or keyboard actionable,
* but is not a native HTML button.
* Component which is clickable or keyboard actionable, but is not a
* native HTML button.
*
* @extends Component
*/
Expand All @@ -36,7 +33,7 @@ class ClickableComponent extends Component {
}

/**
* Create the `Component`s DOM element.
* Create the `ClickableComponent`s DOM element.
*
* @param {string} [tag=div]
* The element's node type.
Expand Down Expand Up @@ -83,7 +80,7 @@ class ClickableComponent extends Component {
}

/**
* Create a control text element on this `Component`
* Create a control text element on this `ClickableComponent`
*
* @param {Element} [el]
* Parent element for the control text.
Expand All @@ -109,7 +106,7 @@ class ClickableComponent extends Component {
}

/**
* Get or set the localize text to use for the controls on the `Component`.
* Get or set the localize text to use for the controls on the `ClickableComponent`.
*
* @param {string} [text]
* Control text for element.
Expand Down Expand Up @@ -146,7 +143,7 @@ class ClickableComponent extends Component {
}

/**
* Enable this `Component`s element.
* Enable this `ClickableComponent`
*/
enable() {
if (!this.enabled_) {
Expand All @@ -157,13 +154,12 @@ class ClickableComponent extends Component {
this.el_.setAttribute('tabIndex', this.tabIndex_);
}
this.on(['tap', 'click'], this.handleClick);
this.on('focus', this.handleFocus);
this.on('blur', this.handleBlur);
this.on('keydown', this.handleKeyDown);
}
}

/**
* Disable this `Component`s element.
* Disable this `ClickableComponent`
*/
disable() {
this.enabled_ = false;
Expand All @@ -173,27 +169,15 @@ class ClickableComponent extends Component {
this.el_.removeAttribute('tabIndex');
}
this.off(['tap', 'click'], this.handleClick);
this.off('focus', this.handleFocus);
this.off('blur', this.handleBlur);
this.off('keydown', this.handleKeyDown);
}

/**
* This gets called when a `ClickableComponent` gets:
* - Clicked (via the `click` event, listening starts in the constructor)
* - Tapped (via the `tap` event, listening starts in the constructor)
* - The following things happen in order:
* 1. {@link ClickableComponent#handleFocus} is called via a `focus` event on the
* `ClickableComponent`.
* 2. {@link ClickableComponent#handleFocus} adds a listener for `keydown` on using
* {@link ClickableComponent#handleKeyPress}.
* 3. `ClickableComponent` has not had a `blur` event (`blur` means that focus was lost). The user presses
* the space or enter key.
* 4. {@link ClickableComponent#handleKeyPress} calls this function with the `keydown`
* event as a parameter.
* Event handler that is called when a `ClickableComponent` receives a
* `click` or `tap` event.
*
* @param {EventTarget~Event} event
* The `keydown`, `tap`, or `click` event that caused this function to be
* called.
* The `tap` or `click` event that caused this function to be called.
*
* @listens tap
* @listens click
Expand All @@ -202,52 +186,31 @@ class ClickableComponent extends Component {
handleClick(event) {}

/**
* This gets called when a `ClickableComponent` gains focus via a `focus` event.
* Turns on listening for `keydown` events. When they happen it
* calls `this.handleKeyPress`.
* Event handler that is called when a `ClickableComponent` receives a
* `keydown` event.
*
* @param {EventTarget~Event} event
* The `focus` event that caused this function to be called.
*
* @listens focus
*/
handleFocus(event) {
Events.on(document, 'keydown', Fn.bind(this, this.handleKeyPress));
}

/**
* Called when this ClickableComponent has focus and a key gets pressed down. By
* default it will call `this.handleClick` when the key is space or enter.
* By default, if the key is Space or Enter, it will trigger a `click` event.
*
* @param {EventTarget~Event} event
* The `keydown` event that caused this function to be called.
*
* @listens keydown
*/
handleKeyPress(event) {
// Support Space or Enter key operation to fire a click event
handleKeyDown(event) {

// Support Space or Enter key operation to fire a click event. Also,
// prevent the event from propagating through the DOM and triggering
// Player hotkeys.
if (keycode.isEventKey(event, 'Space') || keycode.isEventKey(event, 'Enter')) {
event.preventDefault();
event.stopPropagation();
this.trigger('click');
} else {

// Pass keypress handling up for unsupported keys
super.handleKeyPress(event);
super.handleKeyDown(event);
}
}

/**
* Called when a `ClickableComponent` loses focus. Turns off the listener for
* `keydown` events. Which Stops `this.handleKeyPress` from getting called.
*
* @param {EventTarget~Event} event
* The `blur` event that caused this function to be called.
*
* @listens blur
*/
handleBlur(event) {
Events.off(document, 'keydown', Fn.bind(this, this.handleKeyPress));
}
}

Component.registerComponent('ClickableComponent', ClickableComponent);
Expand Down
19 changes: 2 additions & 17 deletions src/js/close-button.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,25 +36,10 @@ class CloseButton extends Button {
return `vjs-close-button ${super.buildCSSClass()}`;
}

/**
* This gets called when a `CloseButton` has focus and `keydown` is triggered via a key
* press.
*
* @param {EventTarget~Event} event
* The event that caused this function to get called.
*
* @listens keydown
*/
handleKeyPress(event) {
// Override the default `Button` behavior, and don't pass the keypress event
// up to the player because this button is part of a `ModalDialog`, which
// doesn't pass keypresses to the player either.
}

/**
* This gets called when a `CloseButton` gets clicked. See
* {@link ClickableComponent#handleClick} for more information on when this will be
* triggered
* {@link ClickableComponent#handleClick} for more information on when
* this will be triggered
*
* @param {EventTarget~Event} event
* The `keydown`, `tap`, or `click` event that caused this function to be
Expand Down
23 changes: 20 additions & 3 deletions src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -1078,18 +1078,35 @@ class Component {
}

/**
* When this Component receives a keydown event which it does not process,
* When this Component receives a `keydown` event which it does not process,
* it passes the event to the Player for handling.
*
* @param {EventTarget~Event} event
* The `keydown` event that caused this function to be called.
*/
handleKeyPress(event) {
handleKeyDown(event) {
if (this.player_) {
this.player_.handleKeyPress(event);

// We only stop propagation here because we want unhandled events to fall
// back to the browser.
event.stopPropagation();
this.player_.handleKeyDown(event);
}
}

/**
* Many components used to have a `handleKeyPress` method, which was poorly
* named because it listened to a `keydown` event. This method name now
* delegates to `handleKeyDown`. This means anyone calling `handleKeyPress`
* will not see their method calls stop working.
*
* @param {EventTarget~Event} event
* The event that caused this function to be called.
*/
handleKeyPress(event) {
this.handleKeyDown(event);
}

/**
* Emit a 'tap' events when touch event support gets detected. This gets used to
* support toggling the controls through a tap on the video. They get enabled
Expand Down
12 changes: 9 additions & 3 deletions src/js/control-bar/progress-control/seek-bar.js
Original file line number Diff line number Diff line change
Expand Up @@ -407,30 +407,36 @@ class SeekBar extends Slider {
*
* @listens keydown
*/
handleKeyPress(event) {
handleKeyDown(event) {
if (keycode.isEventKey(event, 'Space') || keycode.isEventKey(event, 'Enter')) {
event.preventDefault();
event.stopPropagation();
this.handleAction(event);
} else if (keycode.isEventKey(event, 'Home')) {
event.preventDefault();
event.stopPropagation();
this.player_.currentTime(0);
} else if (keycode.isEventKey(event, 'End')) {
event.preventDefault();
event.stopPropagation();
this.player_.currentTime(this.player_.duration());
} else if (/^[0-9]$/.test(keycode(event))) {
event.preventDefault();
event.stopPropagation();
const gotoFraction = (keycode.codes[keycode(event)] - keycode.codes['0']) * 10.0 / 100.0;

this.player_.currentTime(this.player_.duration() * gotoFraction);
} else if (keycode.isEventKey(event, 'PgDn')) {
event.preventDefault();
event.stopPropagation();
this.player_.currentTime(this.player_.currentTime() - (STEP_SECONDS * PAGE_KEY_MULTIPLIER));
} else if (keycode.isEventKey(event, 'PgUp')) {
event.preventDefault();
event.stopPropagation();
this.player_.currentTime(this.player_.currentTime() + (STEP_SECONDS * PAGE_KEY_MULTIPLIER));
} else {
// Pass keypress handling up for unsupported keys
super.handleKeyPress(event);
// Pass keydown handling up for unsupported keys
super.handleKeyDown(event);
}
}
}
Expand Down
Loading