Skip to content
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: force autoplay in Chrome #4804

Merged
merged 6 commits into from
Dec 14, 2017
Merged

fix: force autoplay in Chrome #4804

merged 6 commits into from
Dec 14, 2017

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Dec 11, 2017

Chrome has started pausing autoplaying video elements when they are
moved in the DOM. Here we need to make sure that if the video started
autoplaying, after we move the element in the DOM we call play again.

Fixes #4720.

@gkatsev gkatsev added needs: LGTM Needs one or more additional approvals patch This PR can be added to a patch release. labels Dec 11, 2017
@@ -329,6 +333,7 @@ class Html5 extends Tech {
}
}

this.hasStartedPlayback_ = el.paused;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

el.paused, is also true if it's never played.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, this should actually be !el.paused. Also @andriybohdan tested it and found that it just autoplayed everything, probably because of this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might also be too late to check anyway. The element would be moved and apparently paused at https://github.com/videojs/video.js/blob/master/src/js/player.js#L643.

What is the disadvantage to creating a new video el as we do for iphone?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recreating the element in all conditions could potentially be a breaking change.
However, it'll be something to consider if this type of workaround wouldn't work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think this place is fine because we set the autoplay attribute a few lines above. Seems like !el.paused seems to work here but I guess we'd want to do more tests before pulling it in.

@gkatsev
Copy link
Member Author

gkatsev commented Dec 12, 2017

Looks like getting the conditional right also fixed the tests :)

misteroneill
misteroneill previously approved these changes Dec 13, 2017
@misteroneill misteroneill removed the needs: LGTM Needs one or more additional approvals label Dec 13, 2017
@gkatsev
Copy link
Member Author

gkatsev commented Dec 13, 2017

Seems like doing the check here isn't good enough. Seems like there are two options:

  1. call play if autoplay is set, or
  2. recreate the video element.

Option 1 isn't great because we had just removed code like this. Might be ok if we check for chrome and autoplay and we aren't playing already at the end of video.js initialization.
Option 2 isn't great because it's potentially a breaking change. I've seen too much code create video.js players but then proceed to call play and what not on the video element itself.

@gkatsev gkatsev force-pushed the fix-autoplay-chrome branch from 7e57c3d to 9b310b8 Compare December 13, 2017 20:40
@gkatsev gkatsev changed the title fix(html5): keep autoplaying when el moved in DOM fix: force autoplay in Chrome Dec 13, 2017
@misteroneill misteroneill dismissed their stale review December 13, 2017 20:42

New changes.

misteroneill
misteroneill previously approved these changes Dec 13, 2017
@pagarwal123
Copy link

QA-LGTM

@gkatsev gkatsev merged commit 6fe7a9a into master Dec 14, 2017
@gkatsev gkatsev deleted the fix-autoplay-chrome branch December 14, 2017 16:24
gkatsev added a commit that referenced this pull request Mar 13, 2018
gkatsev added a commit that referenced this pull request Mar 13, 2018
This reverts commit 6fe7a9a.

The PR #4804, which fixes #4720, causes a regression #5005. When testing for the regression, the original issue is no longer reproducible so the fix should be backed out. If someone is still using the old version of chrome and seeing the behavior of #4720, they can continue using the versions of Video.js that contain the fixed made in #4804: 6.5.2, 6.6x, 6.7.x.

Fixes #5005.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants