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: rely on browser or tech to handle autoplay #4582

Merged
merged 2 commits into from
Aug 30, 2017
Merged

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Aug 21, 2017

Description

When setting the source, we were calling play if autoplay was set. We shouldn't do that and instead we should rely on the browser or tech (like Flash) to handle it properly.
I'm unsure whether this should be a fix or a feat, thoughts?

There are two more places that we should consider removing but they are probably OK because they're specific/bug fixes rather than general "if autoplay, call play" kind of deals.
There's one in enterFullscreen on the html5 tech, which should only be called on iOS devices.

/**
* Request that the `HTML5` Tech enter fullscreen.
*/
enterFullScreen() {
const video = this.el_;
if (video.paused && video.networkState <= video.HAVE_METADATA) {
// attempt to prime the video element for programmatic access
// this isn't necessary on the desktop but shouldn't hurt
this.el_.play();

In the player, in handleTechReady_, there's another call to play, which is a fix for older chrome and older safari. This one seems more of a candidate for removal than the html5 one above.

video.js/src/js/player.js

Lines 1068 to 1079 in fe63992

// Chrome and Safari both have issues with autoplay.
// In Safari (5.1.1), when we move the video element into the container div, autoplay doesn't work.
// In Chrome (15), if you have autoplay + a poster + no controls, the video gets hidden (but audio plays)
// This fixes both issues. Need to wait for API, so it updates displays correctly
if ((this.src() || this.currentSrc()) && this.tag && this.options_.autoplay && this.paused()) {
try {
// Chrome Fix. Fixed in Chrome v16.
delete this.tag.poster;
} catch (e) {
log('deleting tag.poster throws in some browsers', e);
}
this.play();

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@gkatsev
Copy link
Member Author

gkatsev commented Aug 21, 2017

@videojs/core-committers any thoughts on the removal of those two play() calls?

In the player case, we can either delete the entire block or just the call to play. The try/catch+delete 4e03250 and the whole block was added here: e5595b1#diff-3b0266ff1c037b289ec624ab25b0272eR400

Seems like the html5 one was added 4 years ago for iOS 6 support: 7d67abd

Seems like removing these pieces could potentially cause older browsers to misbehave.

@heff
Copy link
Member

heff commented Aug 22, 2017

Could there be people relying on this behavior when switching videos? If so we might want this to be a major version change, unless it's breaking something now.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 22, 2017

It's theoretically possible but I find it unlikely. I think the reason the code was there was for flash but also because we did a bad job at setting attributes/properties on the video element, which was recently fixed in #4562.
Autoplay should continue to work assuming that the underlying tech supports the autoplay option. I guess we should survey to see if things like the youtube tech would break with this change or not.

@misteroneill
Copy link
Member

misteroneill commented Aug 22, 2017

I think "fix" is appropriate here. If our goal is emulating/mirroring the standard behavior, this gets us closer to that in a way it wasn't before.

For enterFullscreen on the Html5 tech, if it's platform-specific, maybe it should have that as a condition? What happens without that call to play() in iOS?

Yeah, I think that can probably be safely removed, based on my assumptions about browser usage.

But @heff's concerns are valid as well.

@ldayananda
Copy link
Contributor

Although "fix" is appropriate for this project, it does mean a change to the behavior of the underlying tech/player which is concerning and I think would push this towards a "feat" so users have the ability to test and update their integrations.

I agree with updating enterFullScreen to be platform specific if iOS is the only platform it is intended for.

handleTechReady_ does seem like a good candidate for removal.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 22, 2017

For enterFullscreen on the Html5 tech, if it's platform-specific, maybe it should have that as a condition? What happens without that call to play() in iOS?

enterFullscreen gets called if the tech says it supports fullscreen but native FS APIs aren't present. This is basically only the case for iOS. It seems like the issue is clicking the fullscreen button before ever hitting play on iOS used to be a no-op (#424).

Removing play from handleTechReady_ seems to not cause any issues.


Also, I tested this change with videojs-youtube and it autoplays. Looking at a selection of other techs from the wiki.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 22, 2017

I tested it with a few others but either couldn't get them to work at all or they don't autoplay regardless of this change (like the soundcloud tech).

@mister-ben
Copy link
Contributor

An advantage of using play() to "autoplay" would be to use the promise returned by some browsers to know whether autoplay succeeded. We don't do that currently, but being able to track plays that resulted from autoplay distinctly from manual could be useful.

Copy link
Member

@heff heff left a comment

Choose a reason for hiding this comment

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

I feel comfortable with this as fix.

I'd also be interested to explore how we can use the play() promise like @mister-ben suggested, but that can be separate from this.

@gkatsev gkatsev merged commit 95c4ae0 into master Aug 30, 2017
@gkatsev gkatsev deleted the remove-extra-plays branch August 30, 2017 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants