From fb6be4dc63c1f16ef80da9dcdb663b545cf98819 Mon Sep 17 00:00:00 2001 From: Steve Heffernan Date: Mon, 9 Jun 2014 17:42:05 -0700 Subject: [PATCH 1/3] Fixed the issue where the firstplay event wouldn't fire if the play event was fired before loadstart --- src/js/media/html5.js | 4 +-- src/js/player.js | 61 +++++++++++++++++++++++++---------------- test/unit/mediafaker.js | 1 + test/unit/player.js | 31 ++++++++++++++++++++- 4 files changed, 70 insertions(+), 27 deletions(-) diff --git a/src/js/media/html5.js b/src/js/media/html5.js index 6cb8ff46c1..4bf713468a 100644 --- a/src/js/media/html5.js +++ b/src/js/media/html5.js @@ -33,9 +33,9 @@ vjs.Html5 = vjs.MediaTechController.extend({ // We don't want to set the source again and interrupt playback. if (source && this.el_.currentSrc === source.src && this.el_.networkState > 0) { // wait for the player to be ready so the player listeners are attached - player.ready(function(){ + // player.ready(function(){ player.trigger('loadstart'); - }); + // }); // Otherwise set the source if one was provided. } else if (source) { this.el_.src = source.src; diff --git a/src/js/player.js b/src/js/player.js index 9fdd95cca5..10ae18861a 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -58,20 +58,6 @@ vjs.Player = vjs.Component.extend({ // see enableTouchActivity in Component options.reportTouchActivity = false; - // Make sure the event listeners are the first things to happen when - // the player is ready. See #1208 - // If not, the tech might fire events before the listeners are attached. - this.ready(function(){ - this.on('loadstart', this.onLoadStart); - this.on('ended', this.onEnded); - this.on('play', this.onPlay); - this.on('firstplay', this.onFirstPlay); - this.on('pause', this.onPause); - this.on('progress', this.onProgress); - this.on('durationchange', this.onDurationChange); - this.on('fullscreenchange', this.onFullscreenChange); - }); - // Run base component initializing with new options. // Builds the element through createEl() // Inits and embeds any child components in opts @@ -230,6 +216,22 @@ vjs.Player.prototype.createEl = function(){ } vjs.insertFirst(tag, el); // Breaks iPhone, fixed in HTML5 setup. + // The event listeners need to be added before the children are added + // in the component init because the tech (loaded with mediaLoader) may + // fire events, like loadstart, that these events need to capture. + // Long term it might be better to expose a way to do this in component.init + // like component.initEventListeners() that runs between el creation and + // adding children + this.el_ = el; + this.on('loadstart', this.onLoadStart); + this.on('ended', this.onEnded); + this.on('play', this.onPlay); + this.on('firstplay', this.onFirstPlay); + this.on('pause', this.onPause); + this.on('progress', this.onProgress); + this.on('durationchange', this.onDurationChange); + this.on('fullscreenchange', this.onFullscreenChange); + return el; }; @@ -408,9 +410,19 @@ vjs.Player.prototype.stopTrackingCurrentTime = function(){ * @event loadstart */ vjs.Player.prototype.onLoadStart = function() { + // If it's currently playing we want to trigger a firstplay event. + // The case comes from when a `play` event is fired before the `loadstart` + // event. This is easy to do by calling vidEl.play() immediately after it's + // created. In this case the `play` event will not be fired after this point + // without first pausing the video, meaning the neither will the firstplay. + if (!this.paused()) { + this.triggerFirstPlay(); + return; + } + // remove any first play listeners that weren't triggered from a previous video. - this.off('play', initFirstPlay); - this.one('play', initFirstPlay); + this.off('play', this.triggerFirstPlay); + this.one('play', this.triggerFirstPlay); if (this.error()) { this.error(null); @@ -419,19 +431,20 @@ vjs.Player.prototype.onLoadStart = function() { vjs.removeClass(this.el_, 'vjs-has-started'); }; - // Need to create this outside the scope of onLoadStart so it - // can be added and removed (to avoid piling first play listeners). -function initFirstPlay(e) { +// Need to create this outside the scope of onLoadStart so it +// can be added and removed (to avoid piling first play listeners). +vjs.Player.prototype.triggerFirstPlay = function(playEvent){ var fpEvent = { type: 'firstplay', target: this.el_ }; // Using vjs.trigger so we can check if default was prevented var keepGoing = vjs.trigger(this.el_, fpEvent); - if (!keepGoing) { - e.preventDefault(); - e.stopPropagation(); - e.stopImmediatePropagation(); + // allow for stopping propagation of the original 'play' event + if (playEvent && !keepGoing) { + playEvent.preventDefault(); + playEvent.stopPropagation(); + playEvent.stopImmediatePropagation(); } -} +}; /** * Fired when the player has initial duration and dimension information diff --git a/test/unit/mediafaker.js b/test/unit/mediafaker.js index 124e5a555d..222a9c10e1 100644 --- a/test/unit/mediafaker.js +++ b/test/unit/mediafaker.js @@ -40,6 +40,7 @@ vjs.MediaFaker.prototype.src = function(){ return 'movie.mp4'; }; vjs.MediaFaker.prototype.volume = function(){ return 0; }; vjs.MediaFaker.prototype.muted = function(){ return false; }; vjs.MediaFaker.prototype.pause = function(){ return false; }; +vjs.MediaFaker.prototype.paused = function(){ return true; }; vjs.MediaFaker.prototype.supportsFullScreen = function(){ return false; }; vjs.MediaFaker.prototype.features = {}; vjs.MediaFaker.prototype.buffered = function(){ return {}; }; diff --git a/test/unit/player.js b/test/unit/player.js index 70831b45d2..7eb6db94e6 100644 --- a/test/unit/player.js +++ b/test/unit/player.js @@ -378,7 +378,7 @@ test('should not add multiple first play events despite subsequent loads', funct var player = PlayerTest.makePlayer({}); player.on('firstplay', function(){ - ok('First play should fire once.'); + ok(true, 'First play should fire once.'); }); // Checking to make sure onLoadStart removes first play listener before adding a new one. @@ -387,6 +387,35 @@ test('should not add multiple first play events despite subsequent loads', funct player.trigger('play'); }); +test('should fire firstplay after resetting the player', function() { + var player = PlayerTest.makePlayer({}); + + var fpFired = false; + player.on('firstplay', function(){ + fpFired = true; + }); + + // init firstplay listeners + player.trigger('loadstart'); + player.trigger('play'); + ok(fpFired, 'First firstplay fired'); + + // reset the player + player.trigger('loadstart'); + fpFired = false; + player.trigger('play'); + ok(fpFired, 'Second firstplay fired'); + + // the play event can fire before the loadstart event. + // in that case we still want the firstplay even to fire. + player.tech.paused = function(){ return false; }; + fpFired = false; + // reset the player + player.trigger('loadstart'); + // player.trigger('play'); + ok(fpFired, 'Third firstplay fired'); +}); + test('should remove vjs-has-started class', function(){ expect(3); From 516ab19b3751680cf48dc4d2f57c035ff995a815 Mon Sep 17 00:00:00 2001 From: Steve Heffernan Date: Tue, 10 Jun 2014 10:16:29 -0700 Subject: [PATCH 2/3] Added hasPlayed property to clean up firstplay operation --- src/js/media/html5.js | 5 +---- src/js/player.js | 48 ++++++++++++++++++++++++------------------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/js/media/html5.js b/src/js/media/html5.js index 4bf713468a..66fcdb1586 100644 --- a/src/js/media/html5.js +++ b/src/js/media/html5.js @@ -32,10 +32,7 @@ vjs.Html5 = vjs.MediaTechController.extend({ // If the element source is already set, we may have missed the loadstart event, and want to trigger it. // We don't want to set the source again and interrupt playback. if (source && this.el_.currentSrc === source.src && this.el_.networkState > 0) { - // wait for the player to be ready so the player listeners are attached - // player.ready(function(){ - player.trigger('loadstart'); - // }); + player.trigger('loadstart'); // Otherwise set the source if one was provided. } else if (source) { this.el_.src = source.src; diff --git a/src/js/player.js b/src/js/player.js index 10ae18861a..84072843da 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -416,34 +416,40 @@ vjs.Player.prototype.onLoadStart = function() { // created. In this case the `play` event will not be fired after this point // without first pausing the video, meaning the neither will the firstplay. if (!this.paused()) { - this.triggerFirstPlay(); + // usually hasStarted() triggers firstplay, but we dont' need to + // change that state here. + this.trigger('firstplay'); return; } - // remove any first play listeners that weren't triggered from a previous video. - this.off('play', this.triggerFirstPlay); - this.one('play', this.triggerFirstPlay); - - if (this.error()) { - this.error(null); - } + // reset the hasStarted state + this.hasStarted(false); + this.one('play', function(){ + this.hasStarted(true); + }); - vjs.removeClass(this.el_, 'vjs-has-started'); + // reset the error state + this.error(null); }; -// Need to create this outside the scope of onLoadStart so it -// can be added and removed (to avoid piling first play listeners). -vjs.Player.prototype.triggerFirstPlay = function(playEvent){ - var fpEvent = { type: 'firstplay', target: this.el_ }; - // Using vjs.trigger so we can check if default was prevented - var keepGoing = vjs.trigger(this.el_, fpEvent); - - // allow for stopping propagation of the original 'play' event - if (playEvent && !keepGoing) { - playEvent.preventDefault(); - playEvent.stopPropagation(); - playEvent.stopImmediatePropagation(); +vjs.Player.prototype.hasStarted_ = false; + +vjs.Player.prototype.hasStarted = function(hasStarted){ + if (hasStarted !== undefined) { + // only update if this is a new value + if (this.hasStarted_ !== hasStarted) { + this.hasStarted_ = hasStarted; + if (hasStarted) { + this.addClass('vjs-has-started'); + // trigger the firstplay event if this newly has played + this.trigger('firstplay'); + } else { + this.removeClass('vjs-has-started'); + } + } + return this; } + return this.hasStarted_; }; /** From 539e9954530d6a64886a900a0730b73ed24973b0 Mon Sep 17 00:00:00 2001 From: Steve Heffernan Date: Tue, 10 Jun 2014 16:31:57 -0700 Subject: [PATCH 3/3] Cleaned up the onLoadStart handler --- src/js/player.js | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/js/player.js b/src/js/player.js index 84072843da..c8d4616828 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -410,26 +410,23 @@ vjs.Player.prototype.stopTrackingCurrentTime = function(){ * @event loadstart */ vjs.Player.prototype.onLoadStart = function() { - // If it's currently playing we want to trigger a firstplay event. - // The case comes from when a `play` event is fired before the `loadstart` - // event. This is easy to do by calling vidEl.play() immediately after it's - // created. In this case the `play` event will not be fired after this point - // without first pausing the video, meaning the neither will the firstplay. - if (!this.paused()) { - // usually hasStarted() triggers firstplay, but we dont' need to - // change that state here. - this.trigger('firstplay'); - return; - } - - // reset the hasStarted state - this.hasStarted(false); - this.one('play', function(){ - this.hasStarted(true); - }); + // TODO: Update to use `emptied` event instead. See #1277. // reset the error state this.error(null); + + // If it's already playing we want to trigger a firstplay event now. + // The firstplay event relies on both the play and loadstart events + // which can happen in any order for a new source + if (!this.paused()) { + this.trigger('firstplay'); + } else { + // reset the hasStarted state + this.hasStarted(false); + this.one('play', function(){ + this.hasStarted(true); + }); + } }; vjs.Player.prototype.hasStarted_ = false;