diff --git a/package-lock.json b/package-lock.json index 0f03faad0..b523e2c26 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "video.js", - "version": "6.2.8", + "version": "6.4.0", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -1126,14 +1126,10 @@ } }, "babelify": { - "version": "7.3.0", - "resolved": "https://registry.npmjs.org/babelify/-/babelify-7.3.0.tgz", - "integrity": "sha1-qlau3nBn/XvVSWZu4W3ChQh+iOU=", - "dev": true, - "requires": { - "babel-core": "6.26.0", - "object-assign": "4.1.1" - } + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/babelify/-/babelify-8.0.0.tgz", + "integrity": "sha512-xVr63fKEvMWUrrIbqlHYsMcc5Zdw4FSVesAHgkgajyCE1W8gbm9rbMakqavhxKvikGYMhEcqxTwB/gQmQ6lBtw==", + "dev": true }, "babylon": { "version": "6.18.0", diff --git a/src/js/big-play-button.js b/src/js/big-play-button.js index b28eb80e7..d86253cfd 100644 --- a/src/js/big-play-button.js +++ b/src/js/big-play-button.js @@ -3,6 +3,7 @@ */ import Button from './button.js'; import Component from './component.js'; +import {isPromise} from './utils/promise'; /** * The initial play button that shows before the video has played. The hiding of the @@ -58,10 +59,8 @@ class BigPlayButton extends Button { const playFocus = () => playToggle.focus(); - if (playPromise && playPromise.then) { - const ignoreRejectedPlayPromise = () => {}; - - playPromise.then(playFocus, ignoreRejectedPlayPromise); + if (isPromise(playPromise)) { + playPromise.then(playFocus, () => {}); } else { this.setTimeout(playFocus, 1); } diff --git a/src/js/player.js b/src/js/player.js index 20c4ed9a6..5a80f1b2d 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -24,6 +24,7 @@ import MediaError from './media-error.js'; import safeParseTuple from 'safe-json-parse/tuple'; import {assign} from './utils/obj'; import mergeOptions from './utils/merge-options.js'; +import {silencePromise} from './utils/promise'; import textTrackConverter from './tracks/text-track-list-converter.js'; import ModalDialog from './modal-dialog'; import Tech from './tech/tech.js'; @@ -468,6 +469,8 @@ class Player extends Component { this.on('stageclick', this.handleStageClick_); this.changingSrc_ = false; + this.playWaitingForReady_ = false; + this.playOnLoadstart_ = null; } /** @@ -1665,37 +1668,55 @@ class Player extends Component { } /** - * start media playback + * Attempt to begin playback at the first opportunity. * * @return {Promise|undefined} - * Returns a `Promise` if the browser returns one, for most browsers this will - * return undefined. + * Returns a `Promise` only if the browser returns one and the player + * is ready to begin playback. For some browsers and all non-ready + * situations, this will return `undefined`. */ play() { - if (this.changingSrc_) { - this.ready(function() { - const retval = this.techGet_('play'); - // silence errors (unhandled promise from play) - if (retval !== undefined && typeof retval.then === 'function') { - retval.then(null, (e) => {}); - } + // If this is called while we have a play queued up on a loadstart, remove + // that listener to avoid getting in a potentially bad state. + if (this.playOnLoadstart_) { + this.off('loadstart', this.playOnLoadstart_); + } + + // If the player/tech is not ready, queue up another call to `play()` for + // when it is. This will loop back into this method for another attempt at + // playback when the tech is ready. + if (!this.isReady_) { + + // Bail out if we're already waiting for `ready`! + if (this.playWaitingForReady_) { + return; + } + + this.playWaitingForReady_ = true; + this.ready(() => { + this.playWaitingForReady_ = false; + silencePromise(this.play()); }); - // Only calls the tech's play if we already have a src loaded - } else if (this.isReady_ && (this.src() || this.currentSrc())) { + // If the player/tech is ready and we have a source, we can attempt playback. + } else if (!this.changingSrc_ && (this.src() || this.currentSrc())) { return this.techGet_('play'); - } else { - this.ready(function() { - this.tech_.one('loadstart', function() { - const retval = this.play(); - // silence errors (unhandled promise from play) - if (retval !== undefined && typeof retval.then === 'function') { - retval.then(null, (e) => {}); - } - }); - }); + // If the tech is ready, but we do not have a source, we'll need to wait + // for both the `ready` and a `loadstart` when the source is finally + // resolved by middleware and set on the player. + // + // This can happen if `play()` is called while changing sources or before + // one has been set on the player. + } else { + + this.playOnLoadstart_ = () => { + this.playOnLoadstart_ = null; + silencePromise(this.play()); + }; + + this.one('loadstart', this.playOnLoadstart_); } } diff --git a/src/js/utils/promise.js b/src/js/utils/promise.js new file mode 100644 index 000000000..03f8b1923 --- /dev/null +++ b/src/js/utils/promise.js @@ -0,0 +1,28 @@ + +/** + * Returns whether an object is `Promise`-like (i.e. has a `then` method). + * + * @param {Object} value + * An object that may or may not be `Promise`-like. + * + * @return {Boolean} + * Whether or not the object is `Promise`-like. + */ +export function isPromise(value) { + return value !== undefined && typeof value.then === 'function'; +} + +/** + * Silence a Promise-like object. + * + * This is useful for avoiding non-harmful, but potentially confusing "uncaught + * play promise" rejection error messages. + * + * @param {Object} value + * An object that may or may not be `Promise`-like. + */ +export function silencePromise(value) { + if (isPromise(value)) { + value.then(null, (e) => {}); + } +} diff --git a/test/unit/player-play.test.js b/test/unit/player-play.test.js new file mode 100644 index 000000000..66fc1c3bb --- /dev/null +++ b/test/unit/player-play.test.js @@ -0,0 +1,100 @@ +/* eslint-env qunit */ +import sinon from 'sinon'; +import {silencePromise} from '../../src/js/utils/promise'; +import TestHelpers from './test-helpers'; + +QUnit.module('Player#play', { + + beforeEach() { + this.clock = sinon.useFakeTimers(); + this.player = TestHelpers.makePlayer({}); + this.techPlayCallCount = 0; + this.player.tech_.play = () => { + this.techPlayCallCount++; + }; + }, + + afterEach() { + this.player.dispose(); + this.clock.restore(); + } +}); + +QUnit.test('tech not ready + no source = wait for ready, then loadstart', function(assert) { + + // Mock the player/tech not being ready. + this.player.isReady_ = false; + + // Attempt to play. + this.player.play(); + this.clock.tick(100); + assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because the tech was not ready'); + + // Ready the player. + this.player.triggerReady(); + this.clock.tick(100); + assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because there was no source'); + + // Add a source and trigger loadstart. + this.player.src('xyz.mp4'); + this.clock.tick(100); + this.player.trigger('loadstart'); + assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called'); +}); + +QUnit.test('tech not ready + has source = wait for ready', function(assert) { + + // Mock the player/tech not being ready, but having a source. + this.player.isReady_ = false; + this.player.src('xyz.mp4'); + this.clock.tick(100); + + // Attempt to play. + this.player.play(); + this.clock.tick(100); + assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because the tech was not ready'); + + // Ready the player. + this.player.triggerReady(); + this.clock.tick(100); + assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called'); +}); + +QUnit.test('tech ready + no source = wait for loadstart', function(assert) { + + // Attempt to play. + this.player.play(); + this.clock.tick(100); + assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because the tech was not ready'); + + // Add a source and trigger loadstart. + this.player.src('xyz.mp4'); + this.clock.tick(100); + this.player.trigger('loadstart'); + assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called'); +}); + +QUnit.test('tech ready + has source = play immediately!', function(assert) { + + // Mock the player having a source. + this.player.src('xyz.mp4'); + this.clock.tick(100); + + // Attempt to play, but silence the promise that might be returned. + silencePromise(this.player.play()); + assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called'); +}); + +QUnit.test('tech ready + has source + changing source = wait for loadstart', function(assert) { + + // Mock the player having a source and in the process of changing its source. + this.player.src('xyz.mp4'); + this.clock.tick(100); + this.player.src('abc.mp4'); + this.player.play(); + this.clock.tick(100); + assert.strictEqual(this.techPlayCallCount, 0, 'tech_.play was not called because the source was changing'); + + this.player.trigger('loadstart'); + assert.strictEqual(this.techPlayCallCount, 1, 'tech_.play was called'); +}); diff --git a/test/unit/utils/promise.test.js b/test/unit/utils/promise.test.js new file mode 100644 index 000000000..2171024fc --- /dev/null +++ b/test/unit/utils/promise.test.js @@ -0,0 +1,20 @@ +/* eslint-env qunit */ +import window from 'global/window'; +import * as promise from '../../../src/js/utils/promise'; + +QUnit.module('utils/promise'); + +QUnit.test('can correctly identify a native Promise (if supported)', function(assert) { + + // If Promises aren't supported, skip this. + if (!window.Promise) { + return assert.expect(0); + } + + assert.ok(promise.isPromise(new window.Promise((resolve) => resolve())), 'a native Promise was recognized'); +}); + +QUnit.test('can identify a Promise-like object', function(assert) { + assert.notOk(promise.isPromise({}), 'an object without a `then` method is not Promise-like'); + assert.ok(promise.isPromise({then: () => {}}), 'an object with a `then` method is Promise-like'); +});