New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NO_SRC when play is hit before the video elem src is set #3378

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ldayananda
Contributor

ldayananda commented Jun 17, 2016

Description

Previously, if play is triggered before the video element has a source, Video.js would throw PLAYER_ERR_NO_SRC and this message would flash in the player briefly.

Specific Changes proposed

This checks whether the player has a source set on play, and if not, adds a loadstart handler to the tech to ensure that the source is loaded before playing.

Requirements Checklist

  • Bug fixed
  • Reviewed by Two Core Contributors
Show outdated Hide outdated src/js/player.js
if (this.player_.currentSrc()) {
this.techCall_('play');
} else {
this.autoplay(true);

This comment has been minimized.

@incompl

incompl Jun 17, 2016

Contributor

What happens if the source is changed afterwards? Does that video autoplay too since we changed the setting?

@incompl

incompl Jun 17, 2016

Contributor

What happens if the source is changed afterwards? Does that video autoplay too since we changed the setting?

This comment has been minimized.

@gkatsev

gkatsev Jun 17, 2016

Member

Another potential issue, on firefox when you hit play without a source, autplay is still set to false: https://jsfiddle.net/gkatsev/xnm01byt/

@gkatsev

gkatsev Jun 17, 2016

Member

Another potential issue, on firefox when you hit play without a source, autplay is still set to false: https://jsfiddle.net/gkatsev/xnm01byt/

This comment has been minimized.

@gkatsev

gkatsev Jun 17, 2016

Member

And I just realized that if we want it to autoplay after a source is given, we need to make sure that the play event is fired when this is called but it isn't going to right now, I don't think...

@gkatsev

gkatsev Jun 17, 2016

Member

And I just realized that if we want it to autoplay after a source is given, we need to make sure that the play event is fired when this is called but it isn't going to right now, I don't think...

This comment has been minimized.

@ldayananda

ldayananda Jun 17, 2016

Contributor

With respect to the play event firing when autoplay is set, player.src(...) actually handles updating the video source and fires the play event if autoplay is true. In my general testing of this on different browsers, this seems to work.

@ldayananda

ldayananda Jun 17, 2016

Contributor

With respect to the play event firing when autoplay is set, player.src(...) actually handles updating the video source and fires the play event if autoplay is true. In my general testing of this on different browsers, this seems to work.

This comment has been minimized.

@gkatsev

gkatsev Jun 17, 2016

Member

Sorry, what I meant is that play should be fired when this function is called and not when the src is added and it starts playing.

@gkatsev

gkatsev Jun 17, 2016

Member

Sorry, what I meant is that play should be fired when this function is called and not when the src is added and it starts playing.

@@ -247,15 +247,15 @@ test('should hide the poster when play is called', function() {
});
equal(player.hasStarted(), false, 'the show poster flag is true before play');
player.play();
player.tech_.trigger('play');

This comment has been minimized.

@incompl

incompl Jun 17, 2016

Contributor

Could you also fix this by mocking currentSrc()?

@incompl

incompl Jun 17, 2016

Contributor

Could you also fix this by mocking currentSrc()?

This comment has been minimized.

@ldayananda

ldayananda Jun 17, 2016

Contributor

I was originally getting an error about the method currentSrc() not existing(likely since the tests use a fake tech.) I looked at how some of the other tests were written and saw that they almost exclusively trigger play in this way so I went with this instead.

@ldayananda

ldayananda Jun 17, 2016

Contributor

I was originally getting an error about the method currentSrc() not existing(likely since the tests use a fake tech.) I looked at how some of the other tests were written and saw that they almost exclusively trigger play in this way so I went with this instead.

This comment has been minimized.

@incompl

incompl Jun 17, 2016

Contributor

If it's consistent with the other tests that seems reasonable

@incompl

incompl Jun 17, 2016

Contributor

If it's consistent with the other tests that seems reasonable

Show outdated Hide outdated src/js/tech/tech.js
@@ -308,6 +308,12 @@ class Tech extends Component {
return createTimeRange();
}
delayedPlay() {
this.el_.onloadstart = function() {

This comment has been minimized.

@gkatsev

gkatsev Jun 20, 2016

Member

you should be able to do this.tech_.one('loadstart'

@gkatsev

gkatsev Jun 20, 2016

Member

you should be able to do this.tech_.one('loadstart'

@ldayananda

This comment has been minimized.

Show comment
Hide comment
@ldayananda

ldayananda Jun 20, 2016

Contributor

Using .one at the player.js level as suggested by @gkatsev.

Contributor

ldayananda commented Jun 20, 2016

Using .one at the player.js level as suggested by @gkatsev.

Show outdated Hide outdated src/js/player.js
@@ -1292,7 +1292,15 @@ class Player extends Component {
* @method play
*/
play() {
this.techCall_('play');
// Only calls the tech's play if we already have a src loaded
if (this.player_.currentSrc()) {

This comment has been minimized.

@gkatsev

gkatsev Jun 20, 2016

Member

You should be able to just do this.currentSrc() here, since the context is already the player itself. We also may want to do this.src() || this.currentSrc() to make sure that we catch issues where the src is set but currentSrc isn't (can happen when adblock is involved.

@gkatsev

gkatsev Jun 20, 2016

Member

You should be able to just do this.currentSrc() here, since the context is already the player itself. We also may want to do this.src() || this.currentSrc() to make sure that we catch issues where the src is set but currentSrc isn't (can happen when adblock is involved.

ldayananda
@ldayananda

This comment has been minimized.

Show comment
Hide comment
@ldayananda

ldayananda Jun 20, 2016

Contributor

Addressing potential holes when adblockers are used as per @gkatsev 's comment.

Contributor

ldayananda commented Jun 20, 2016

Addressing potential holes when adblockers are used as per @gkatsev 's comment.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jun 20, 2016

Member

LGTM

Member

gkatsev commented Jun 20, 2016

LGTM

@misteroneill

This comment has been minimized.

Show comment
Hide comment
@misteroneill
Member

misteroneill commented Jun 20, 2016

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment