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.

Sign up for GitHub

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

Jump to bottom

fix: autoplay throws 'undefined promise' error on some browsers. #5283

Merged
merged 3 commits into from
Jul 3, 2018
Merged

fix: autoplay throws 'undefined promise' error on some browsers. #5283

merged 3 commits into from
Jul 3, 2018

Conversation

ayamamori
Copy link

@ayamamori ayamamori commented Jun 28, 2018 •
edited by gkatsev
Loading

Description

Handling possible exception when calling player.autoplay() so that the video-playing fallback implementation should work.

Specific Changes proposed

If the source is not loaded, player.play() returns undefined on
Firefox and Edge.
This causes an exception when player.autoplay() called.
With this fix, player will not make an exception and the fallback
(Retrying video-play after loading video) will be enabled.

Reproduce

Reproduced on Firefox(Mac) and Edge.
Not reproduced on Chrome(Mac) and Safari

  • Reproducing code
<!DOCTYPE html>
<html>
  <head> 
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
    <link href="http://vjs.zencdn.net/7.0/video-js.min.css" rel="stylesheet">
    <script src="./video.js"></script>
  </head>
  <body>
    <video id="test-video" class="video-js vjs-default-skin" controls>
      <source src="https://bitdash-a.akamaihd.net/content/sintel/hls/playlist.m3u8" type='application/vnd.apple.mpegurl' />
      <!-- <source id="test-source" src="https://www.sample-videos.com/video/mp4/240/big_buck_bunny_240p_1mb.mp4" type='video/mp4' /> -->

    </video>
    <script>
      var main = function(){
        player = videojs("test-video");
        player.on(player, 'loadstart', function(){player.autoplay('any');});
      }
      main();
    </script>
  </body>
</html>
  • Expected

Video is played as either muted or unmuted, without clicking the video element.

  • Actual

video was not played, until clicking the video element.

  • Log

No related console logs were generated.

  • Other

player.autoplay() is a very nice and comfortable feature!
I hope it will release as soon as possible.

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

@gecko655
fix: autoplay throws 'undefined promise' error on some browsers.
1579749 
If the source is not loaded, `player.play();` returns `undefined` on
Firefox and Edge, which causes exception.
With this fix, player will not make exception and the fallback
(`player.play()` execution after loading video) will be enabled.
gkatsev
gkatsev previously approved these changes Jul 2, 2018
View reviewed changes
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Thanks!

@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label Jul 2, 2018
mister-ben
mister-ben reviewed Jul 2, 2018
View reviewed changes
Copy link
Contributor

@mister-ben mister-ben left a comment

Choose a reason for hiding this comment

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

A lack of a promise does not mean autoplay has failed. IE would autoplay but does not return a promise.

@gkatsev
Copy link
Member

gkatsev commented Jul 2, 2018

Good catch @mister-ben. I guess we should just return if there is no promise. Then as part of #3927, we can make it handle things properly if there's no play promise.

@gkatsev gkatsev dismissed their stale review July 2, 2018 18:16

Lack of play promise isn't necessary an autoplay error.

@ayamamori
Copy link
Author

I agree that changing play() to always-returning-Promise function should resolve the issue.

I tried this approach and I could resolve the issue itself, but some other unrelated tests, I think, were getting failed.
I couldn't figure out that these tests were actually broken, or tests were outdated and should be revised.
Can someone investigate this?

...
Ah, wait. Already working on it in #5227? If so and #5227 is merged, please close this PR.

@gkatsev
Copy link
Member

gkatsev commented Jul 3, 2018

There's some more work required for #5227 before it can be merged. I think updating this PR to just return if a promise isn't given will fix any errors and moves us closer to where we want to be.

@ayamamori
Copy link
Author

So, what kind of updates are needed for this PR?
You mean

// src/js/player.js#L1262
+      this.trigger({type: 'autoplay-failure', autoplay: type});

should be removed?

@mister-ben
Copy link
Contributor

Yes, just that.

@ayamamori
Copy link
Author

Done!

@gkatsev gkatsev merged commit c9d1e8a into videojs:master Jul 3, 2018
gkatsev pushed a commit that referenced this pull request Jul 9, 2018
@gecko655 @gkatsev
fix: autoplay throws 'undefined promise' error on some browsers. (#5283)
ab3a9b9 
If the source is not loaded, `player.play();` returns `undefined` on Firefox and Edge, which causes exception.
With this fix, player will not make exception and the fallback (`player.play()` execution after loading video) will be enabled.
@gkatsev gkatsev removed the needs: LGTM Needs one or more additional approvals label Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mister-ben mister-ben mister-ben approved these changes

@gkatsev gkatsev gkatsev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ayamamori @gkatsev @mister-ben @gecko655