Hide the poster when play fires #1834

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@dmlap
Member

dmlap commented Jan 29, 2015

Registering a play handler after loadstart means that another play listener could stop propagation and prevent the poster image from being hidden. Instead, handle the poster toggling in a play handler that is registered immediately on initialization so that the poster display logic more closely matches the standard.

Hide the poster when play fires
Registering a play handler after loadstart means that another play listener could stop propagation and prevent the poster image from being hidden. Instead, handle the poster toggling in a play handler that is registered immediately on initialization so that the poster display logic more closely matches the standard.
@dmlap

This comment has been minimized.

Show comment
Hide comment
@dmlap

dmlap Jan 29, 2015

Member

Note the show poster flag should be set to false in step 4.2 of the HTML standard. Toggling it in Player.prototype.onPlay is technically a little late but given Player's listener is guaranteed to be the first registered, it should be indistinguishable to spec behavior from outside.

Member

dmlap commented Jan 29, 2015

Note the show poster flag should be set to false in step 4.2 of the HTML standard. Toggling it in Player.prototype.onPlay is technically a little late but given Player's listener is guaranteed to be the first registered, it should be indistinguishable to spec behavior from outside.

dmlap added a commit to videojs/videojs-contrib-ads that referenced this pull request Jan 29, 2015

Stop propagation and re-emit video events during ad playback
Prefixing events before they were dispatched caused a number of handlers in video.js itself to fail. Instead, stopImmediatePropagation() on video events as they reach the ad plugin and perform the prefixing there. Add a content-resuming state so that events that are not strictly related to ad playback but distracting to downstream developers can be prefixed with 'content' easily as well. The poster image is not correctly removed from the example page without the fix in videojs/video.js#1834.
src/js/player.js
+
+ // hide the poster when the user hits play
+ // https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-play
+ if (!this.hasStarted()) {

This comment has been minimized.

@heff

heff Jan 29, 2015

Member

The hasStarted method already handles this check, if you want to simplify this.

@heff

heff Jan 29, 2015

Member

The hasStarted method already handles this check, if you want to simplify this.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jan 29, 2015

Member

Makes sense to me. I think we originally put it in loadstart to avoid the extra logic each play event, but it's trivial if it's better to have it in the play handler.

I like that we can point to spec here with the show poster flag, but hasStarted is used for other things too so I don't know if it helps to refer to hasStarted as the show poster flag specifically. Or at least I was thrown off by the test a little, because it refers to the poster and adds one, but the test doesn't actually need a poster.

Otherwise this looks good to me.

Member

heff commented Jan 29, 2015

Makes sense to me. I think we originally put it in loadstart to avoid the extra logic each play event, but it's trivial if it's better to have it in the play handler.

I like that we can point to spec here with the show poster flag, but hasStarted is used for other things too so I don't know if it helps to refer to hasStarted as the show poster flag specifically. Or at least I was thrown off by the test a little, because it refers to the poster and adds one, but the test doesn't actually need a poster.

Otherwise this looks good to me.

@dmlap

This comment has been minimized.

Show comment
Hide comment
@dmlap

dmlap Jan 30, 2015

Member

Yeah, I was nervous about equating hasStarted with the show-poster flag a bit too but I couldn't find anything else it was triggering that would interfere with poster behavior. I think it's worth considering an explicit show-poster flag in video.js so we can feel more confident the behavior is correct.

Member

dmlap commented Jan 30, 2015

Yeah, I was nervous about equating hasStarted with the show-poster flag a bit too but I couldn't find anything else it was triggering that would interfere with poster behavior. I think it's worth considering an explicit show-poster flag in video.js so we can feel more confident the behavior is correct.

Remove check around setting hasStarted
hasStarted() already checks before updating the player element's class name so there is no need to check before calling it.

@dmlap dmlap closed this in f980cdb Feb 9, 2015

@dmlap dmlap deleted the dmlap:hotfix/ye-olde-poster-image branch Feb 9, 2015

dmlap added a commit to videojs/videojs-contrib-ads that referenced this pull request Mar 12, 2015

Stop propagation and re-emit video events during ad playback
Prefixing events before they were dispatched caused a number of handlers in video.js itself to fail. Instead, stopImmediatePropagation() on video events as they reach the ad plugin and perform the prefixing there. Add a content-resuming state so that events that are not strictly related to ad playback but distracting to downstream developers can be prefixed with 'content' easily as well. The poster image is not correctly removed from the example page without the fix in videojs/video.js#1834.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment