fix(Player#play): Wait for loadstart in play() when changing sources instead of just ready. (#4743)

The core goal here is to make sure the following works in light of some middleware process that makes setting the source more async than next tick:

```js
player.src('...');
player.ready(() => player.play());
```

In fact, given this change, we should even be able to do:

```js
player.src('...');
player.play();
```

Unlike #4665, which would have clarified/changed the meaning of "ready", it remains a reflection of the tech's state and we make better use of the ability to queue things on that state and on the middleware `setSource` process.

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",

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);
     }

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_);
     }
   }
 

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) => {});
+  }
+}

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');
+});

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');
+});