diff --git a/CHANGELOG.md b/CHANGELOG.md index 39c9a940c3..a59b6f267d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,7 @@ CHANGELOG * @heff made tech related functions private in the player ([view](https://github.com/videojs/video.js/pull/2590)) * @heff removed the loadedalldata event ([view](https://github.com/videojs/video.js/pull/2591)) * @dmlap switched to using raynos/xhr for requests ([view](https://github.com/videojs/video.js/pull/2594)) +* @heff Fixed double loadstart and ready events ([view](https://github.com/videojs/video.js/pull/2605)) -------------------- diff --git a/src/js/player.js b/src/js/player.js index 761688abc1..11162790f2 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -495,10 +495,6 @@ class Player extends Component { // Turn off API access because we're loading a new tech that might load asynchronously this.isReady_ = false; - var techReady = Fn.bind(this, function() { - this.triggerReady(); - }); - // Grab tech-specific options from player options and add source and parent element to use. var techOptions = assign({ 'nativeControlsForTouch': this.options_.nativeControlsForTouch, @@ -532,11 +528,12 @@ class Player extends Component { let techComponent = Component.getComponent(techName); this.tech_ = new techComponent(techOptions); - textTrackConverter.jsonToTextTracks(this.textTracksJson_ || [], this.tech_); + // player.triggerReady is always async, so don't need this to be async + this.tech_.ready(Fn.bind(this, this.handleTechReady_), true); - this.on(this.tech_, 'ready', this.handleTechReady_); + textTrackConverter.jsonToTextTracks(this.textTracksJson_ || [], this.tech_); - // Listen to every HTML5 events and trigger them back on the player for the plugins + // Listen to all HTML5-defined events and trigger them on the player this.on(this.tech_, 'loadstart', this.handleTechLoadStart_); this.on(this.tech_, 'waiting', this.handleTechWaiting_); this.on(this.tech_, 'canplay', this.handleTechCanPlay_); @@ -581,9 +578,6 @@ class Player extends Component { this.tag.player = null; this.tag = null; } - - // player.triggerReady is always async, so don't need this to be async - this.tech_.ready(techReady, true); } /** @@ -605,7 +599,22 @@ class Player extends Component { } /** - * Add playback technology listeners + * Set up click and touch listeners for the playback element + * + * On desktops, a click on the video itself will toggle playback, + * on a mobile device a click on the video toggles controls. + * (toggling controls is done by toggling the user state between active and + * inactive) + * A tap can signal that a user has become active, or has become inactive + * e.g. a quick tap on an iPhone movie should reveal the controls. Another + * quick tap should hide them again (signaling the user is in an inactive + * viewing state) + * In addition to this, we still want the user to be considered inactive after + * a few seconds of inactivity. + * Note: the only part of iOS interaction we can't mimic with this setup + * is a touch and hold on the video element counting as activity in order to + * keep the controls showing, but that shouldn't be an issue. A touch and hold + * on any controls will still keep the user active * * @private * @method addTechControlsListeners_ diff --git a/src/js/tech/html5.js b/src/js/tech/html5.js index 378381a2f7..005d2a845a 100644 --- a/src/js/tech/html5.js +++ b/src/js/tech/html5.js @@ -36,6 +36,8 @@ class Html5 extends Tech { // anyway so the error gets fired. if (source && (this.el_.currentSrc !== source.src || (options.tag && options.tag.initNetworkState_ === 3))) { this.setSource(source); + } else { + this.handleLateInit_(this.el_); } if (this.el_.hasChildNodes()) { @@ -167,6 +169,87 @@ class Html5 extends Tech { // jenniisawesome = true; } + // If we're loading the playback object after it has started loading + // or playing the video (often with autoplay on) then the loadstart event + // has already fired and we need to fire it manually because many things + // rely on it. + handleLateInit_(el) { + if (el.networkState === 0 || el.networkState === 3) { + // The video element hasn't started loading the source yet + // or didn't find a source + return; + } + + if (el.readyState === 0) { + // NetworkState is set synchronously BUT loadstart is fired at the + // end of the current stack, usually before setInterval(fn, 0). + // So at this point we know loadstart may have already fired or is + // about to fire, and either way the player hasn't seen it yet. + // We don't want to fire loadstart prematurely here and cause a + // double loadstart so we'll wait and see if it happens between now + // and the next loop, and fire it if not. + // HOWEVER, we also want to make sure it fires before loadedmetadata + // which could also happen between now and the next loop, so we'll + // watch for that also. + let loadstartFired = false; + let setLoadstartFired = function() { + loadstartFired = true; + }; + this.on('loadstart', setLoadstartFired); + + let triggerLoadstart = function() { + // We did miss the original loadstart. Make sure the player + // sees loadstart before loadedmetadata + if (!loadstartFired) { + this.trigger('loadstart'); + } + }; + this.on('loadedmetadata', triggerLoadstart); + + this.ready(function(){ + this.off('loadstart', setLoadstartFired); + this.off('loadedmetadata', triggerLoadstart); + + if (!loadstartFired) { + // We did miss the original native loadstart. Fire it now. + this.trigger('loadstart'); + } + }); + + return; + } + + // From here on we know that loadstart already fired and we missed it. + // The other readyState events aren't as much of a problem if we double + // them, so not going to go to as much trouble as loadstart to prevent + // that unless we find reason to. + let eventsToTrigger = ['loadstart']; + + // loadedmetadata: newly equal to HAVE_METADATA (1) or greater + eventsToTrigger.push('loadedmetadata'); + + // loadeddata: newly increased to HAVE_CURRENT_DATA (2) or greater + if (el.readyState >= 2) { + eventsToTrigger.push('loadeddata'); + } + + // canplay: newly increased to HAVE_FUTURE_DATA (3) or greater + if (el.readyState >= 3) { + eventsToTrigger.push('canplay'); + } + + // canplaythrough: newly equal to HAVE_ENOUGH_DATA (4) + if (el.readyState >= 4) { + eventsToTrigger.push('canplaythrough'); + } + + // We still need to give the player time to add event listeners + this.ready(function(){ + eventsToTrigger.forEach(function(type){ + this.trigger(type); + }, this); + }); + } proxyNativeTextTracks_() { let tt = this.el().textTracks; @@ -390,14 +473,18 @@ class Html5 extends Tech { * @deprecated * @method setSrc */ - setSrc(src) { this.el_.src = src; } + setSrc(src) { + this.el_.src = src; + } /** * Load media into player * * @method load */ - load(){ this.el_.load(); } + load(){ + this.el_.load(); + } /** * Get current source diff --git a/src/js/tech/tech.js b/src/js/tech/tech.js index 485e9b33e6..a6e312767d 100644 --- a/src/js/tech/tech.js +++ b/src/js/tech/tech.js @@ -53,8 +53,6 @@ class Tech extends Component { this.manualTimeUpdatesOn(); } - this.initControlsListeners(); - if (options.nativeCaptions === false || options.nativeTextTracks === false) { this.featuresNativeTextTracks = false; } @@ -69,39 +67,6 @@ class Tech extends Component { this.emitTapEvents(); } - /** - * Set up click and touch listeners for the playback element - * On desktops, a click on the video itself will toggle playback, - * on a mobile device a click on the video toggles controls. - * (toggling controls is done by toggling the user state between active and - * inactive) - * A tap can signal that a user has become active, or has become inactive - * e.g. a quick tap on an iPhone movie should reveal the controls. Another - * quick tap should hide them again (signaling the user is in an inactive - * viewing state) - * In addition to this, we still want the user to be considered inactive after - * a few seconds of inactivity. - * Note: the only part of iOS interaction we can't mimic with this setup - * is a touch and hold on the video element counting as activity in order to - * keep the controls showing, but that shouldn't be an issue. A touch and hold on - * any controls will still keep the user active - * - * @method initControlsListeners - */ - initControlsListeners() { - // if we're loading the playback object after it has started loading or playing the - // video (often with autoplay on) then the loadstart event has already fired and we - // need to fire it manually because many things rely on it. - // Long term we might consider how we would do this for other events like 'canplay' - // that may also have fired. - this.ready(function(){ - if (this.networkState && this.networkState() > 0) { - this.trigger('loadstart'); - } - // Allow the tech ready event to handle synchronisity - }, true); - } - /* Fallbacks for unsupported event types ================================================================================ */ // Manually trigger progress events based on changes to the buffered amount diff --git a/test/unit/tech/html5.test.js b/test/unit/tech/html5.test.js index ad986dfdaf..20564f94f0 100644 --- a/test/unit/tech/html5.test.js +++ b/test/unit/tech/html5.test.js @@ -228,3 +228,52 @@ if (Html5.supportsNativeTextTracks()) { equal(adds[2][0], rems[2][0], 'removetrack event handler removed'); }); } + +test('should fire makeup events when a video tag is initialized late', function(){ + let lateInit = Html5.prototype.handleLateInit_; + let triggeredEvents = []; + let mockHtml5 = { + readyListeners: [], + ready(listener){ + this.readyListeners.push(listener); + }, + triggerReady(){ + this.readyListeners.forEach(function(listener){ + listener.call(this); + }, this); + }, + trigger(type){ + triggeredEvents.push(type); + }, + on: function(){}, + off: function(){} + }; + + function resetMock() { + triggeredEvents = {}; + mockHtml5.readyListeners = []; + } + + function testStates(statesObject, expectedEvents) { + lateInit.call(mockHtml5, statesObject); + mockHtml5.triggerReady(); + deepEqual(triggeredEvents, expectedEvents, `wrong events triggered for networkState:${statesObject.networkState} and readyState:${statesObject.readyState || 'no readyState'}`); + + // reset mock + triggeredEvents = []; + mockHtml5.readyListeners = []; + } + + // Network States + testStates({ networkState: 0, readyState: 0 }, []); + testStates({ networkState: 1, readyState: 0 }, ['loadstart']); + testStates({ networkState: 2, readyState: 0 }, ['loadstart']); + testStates({ networkState: 3, readyState: 0 }, []); + + // Ready States + testStates({ networkState: 1, readyState: 0 }, ['loadstart']); + testStates({ networkState: 1, readyState: 1 }, ['loadstart', 'loadedmetadata']); + testStates({ networkState: 1, readyState: 2 }, ['loadstart', 'loadedmetadata', 'loadeddata']); + testStates({ networkState: 1, readyState: 3 }, ['loadstart', 'loadedmetadata', 'loadeddata', 'canplay']); + testStates({ networkState: 1, readyState: 4 }, ['loadstart', 'loadedmetadata', 'loadeddata', 'canplay', 'canplaythrough']); +});