Fixed the issue where the firstplay event wouldn't fire if the play even...

[heff]

...t was fired before loadstart

[dmlap]

Is this comment out of date now?

[heff]

Yeah, the commented code there needs to be removed.

[heff]

@dmlap @gkatsev do either of you know anything that uses the firstplay event's ability to stop propagation on the associated play event? It would help clean things up if we could remove that. https://github.com/videojs/video.js/blob/master/src/js/player.js#L429

I remember ads being the main reason for adding that, and it doesn't appear ads is even using the firstplay event.

[gkatsev]

I do not remember anything that depends on firstplay.

[heff]

I added a hasStarted property that makes it cleaner to add/remove the has-started class and trigger the firstplay event. It also kills that firstplay feature of stopping the play event, but that was never documented and as far as I can tell, never used.

[dmlap]

We had discussed using preventDefault() for ads but ended just pausing "as quickly as possible". Killing it seems like the right move.

[heff]

Great, thanks.

[dmlap]

I think there are still scenarios where the loadstart-play relationship is going to get messed up. For instance, loading a video initially and then replacing it before play(). That would produce two loadstarts and then a play, and would cause the hasStarted handler to get invoked twice. I don't think that particular order would cause errors but it could be confusing. I was proposing something more like this:

/* in Player.init */
this.on('play', function() {
  if (!player.hasClass('vjs-has-started')) {
    player.addClass('vjs-has-started');
    player.trigger('firstplay');
  }
});
this.on('loadstart', function() {
  if (player.hasClass('vjs-has-started')) {
    player.removeClass('vjs-has-started');
  }
});

[heff]

would cause the hasStarted handler to get invoked twice

The hasStarted function has a built-in check to see if it's actually being changed, and only trigger events/change classes if it is. So the firstplay event will on get fired once in that scenario, and amazingly enough there was already a test for that.

I could change it to always call hasStarted(true) whenever the play event is fired. That would prevent the double hasStarted and then be almost identical to your example. I'm essentially using the "local variable" part of your previous suggestion instead of using hasClass. I'm not sure exactly why but I don't love the idea using hasClass to determine the current state.

I don't think you mean for it to fix everything but your example wouldn't work for the play-happens-first scenario when loading a new source. The loadstart event would happen after the play event and remove the has-started class, causing the big play button to keep showing while the video plays. Basically the firstplay event is dependent on both the play and loadstart events, which can happen in any order for a new source, so somewhere we have to check that both have happened before triggering firstplay.

[dmlap]

Very good point, my example code is busted. My main concern is that listeners-that-set-listeners are a finicky way of managing state. It takes a lot of thinking to be sure that everything is being setup and cleaned up correctly and it's easy to break down the line. I wish we could come up with a way of formulating the conditions for firstplay that localized and minimized the state management. I still think it's possible to do a little bit better on that front.

[dmlap]

Follow-up question: do we actually want the big play button to show up again once some amount of video has been played? Can we dodge the entire issue if vjs-has-started is set on the first play event and then never removed? (That would mean firstplay losing some of the semantics it has today but would simplify the problem significantly)

[heff]

I may have found a better approach...

The video element fires an emptied event when switching to a new source. After some testing, this DOES always fire before the play event. So essentially we should be using the emptied event, not the loadstart event to reset the various player states, and that could simplify things. It kind of feels like a "no duh" now that I look at it. The only problem is the swf doesn't fire the emptied event...so that would need to be updated. I think that's out of the scope of this patch, so I'm going to get this one out there to fix the current issue, and then make a new issue use emptied.

do we actually want the big play button to show up again

Yeah, we actually had a number of issues around how to reset the player to its original state for later videos. It used to work the way you're describing before that.

[dmlap]

That sounds much nicer. We can avoid using one()? I don't want to rain on the parade but... are you sure about emptied on mobile?

[heff]

are you sure about emptied on mobile

No, haven't checked that yet. It's a standard event and works in Chrome, Firefox, and Safari desktop, so I'd be surprised if it doesn't work the same on mobile. But then again, that might be naive. Would you guys be able to try on some devices? You can use this jsbin http://jsbin.com/baqoc/1/edit

[dmlap]

Results loading the example. On iOS and Android, I tried to hit the play button as quickly as possible to force bad behavior.

Win 8.1RT/IE11	iPad/iOS 7	Samsung Android 4.2.2 Stock Browser	Samsung Android 4.2.2 Chrome Browser
emptied loadstart play emptied play loadstart	loadstart play emptied play loadstart	loadstart play emptied play loadstart	loadstart play emptied play loadstart

[heff]

Nice, thanks. That looks pretty consistent to me. We'll just have to make sure the Win 8 first emptied event does mess anything up, but it shouldn't.

[heff]

Closed by 71c0a72