feat: manual autoplay #5209

brandonocasey · 2018-05-24T20:21:11Z

Due to some of the policy changes around autoplay we are going to add a new option. The name of the option and the parameters that the option can be can change in this pr, but I think the general functionality is going to remain the same. Basically you have three paths

Try to autoplay using play and if that doesn't work do nothing, mostly the same as using the autoplay attribute right now.
Try to mute and then autoplay using play, this will work in most cases
Try to autoplay using play and if that doesnt work try to mute and then autoplay, which we know will work.

Questions:

Should we remove option 2 and just recommending using autoplay in that case?
Should we default to using option 3?
Should we just use the autoplay attribute and accept a string?

Coming Soon

Tests
Documentation updates

gkatsev · 2018-05-24T20:40:21Z

src/js/player.js

+    // - truthy value: will just call play
+    // - any: Will call play and if the promise fails, it will try to mute
+    //        and then play
+    if (this.options_.manualAutoplay) {


I still think that maybe autoplay is best and have these options:

muted - will mute the video and then play

manual - will call play manually

any - (or maybe another name?) try to play unmuted and then mute if it fails

any other value do current behavior

then the autoplay getter and setter can also handle this whenever it gets called and the autoplay getter can know whether this option was set and which option.

then this line will just turn to something like this.autoplay(this.options_.autoplay) or something.

I realize that this overloads the values for the autoplay option, but it really is the best name and we can always deprecate the old values and go forward with just the new stuff.

also, we could theoretically, have the setter return the play promise in that way if called later on

OK we are going to go this route, let me re-work this pr

marguinbc · 2018-05-25T13:41:18Z

Late to the comment party but I feel like the 'new autoplay' should have states/settings that correspond to the browser options in those that allow users to choose autoplay options. Something like:

never (results in click-to-play)
always (same as any - tries w/ sound, then muted)
muted (same as muted above)

I'm not sure about manual because I feel always and muted would call play() manually anyway but I see no harm in keeping it and having it return the play promise

manual (calls play() and returns play promise?)

And any other value defaults to current behavior...

misteroneill

Code looks good. Could probably use some JSDoc and documentation updates, though.

misteroneill · 2018-05-25T13:44:15Z

@marguinbc always doesn't make sense, though, because we can never guarantee that autoplay will work. I don't see why having a never option would make sense - that's just the default (no autoplay of any type).

Also, we can't return the play promise with these because it happens in a loadstart listener.

marguinbc · 2018-05-25T14:01:10Z

I suppose any is a better name than always then as they are basically doing the same thing and always implies we can always autoplay, vs any implies "any autoplay that will work".

I see never as being a way to override the autoplay attribute or rather explicitly set it to none as @gkatsev's comment earlier said

any other value do current behavior

Which implies none/default = current behavior and would be dependent on whether the autoplay attribute is set or not.

Fair point on the promise... should have looked a the code.

gkatsev · 2018-05-28T14:12:07Z

docs/guides/faq.md

+Do to recent changes in `autoplay` behavior we no longer recommend using the `autoplay` attribute
+on the video element. It's still supported by video.js but many browsers, including Chrome, are changing their
+`autoplay` attribute behavior.
+For more info on that see: https://developers.google.com/web/updates/2017/09/autoplay-policy-changes


better to link to our blogpost :)

gkatsev · 2018-05-28T14:12:40Z

docs/guides/options.md

-> **Note:** As of iOS 10, Apple offers `autoplay` support in Safari. For details, refer to ["New <video> Policies for iOS"][ios-10-updates].
+#### More info on autoplay support and changes:
+* As of iOS 10, Apple offers `autoplay` support in Safari. For details, refer to ["New <video> Policies for iOS"][ios-10-updates].
+* The autoplay policy in Chrome is changing: https://developers.google.com/web/updates/2017/09/autoplay-policy-changes


blogpost again

gkatsev · 2018-05-28T14:13:42Z

src/js/player.js

@@ -379,6 +379,13 @@ class Player extends Component {
    tag.controls = false;
    tag.removeAttribute('controls');

+    // the attribute overrides the option
+    if (tag.hasAttribute('autoplay')) {
+      this.options_.autoplay = true;


this is so that the following case is covered?

<video autoplay data-setup='{"autoplay": "play"}'>

yeah, basically we would autoplay: true here and not autoplay: play

gkatsev · 2018-05-28T14:14:32Z

src/js/player.js

    // Grab tech-specific options from player options and add source and parent element to use.
    const techOptions = {
      source,
      'nativeControlsForTouch': this.options_.nativeControlsForTouch,
      'playerId': this.id(),
      'techId': `${this.id()}_${titleTechName}_api`,
-      'autoplay': this.options_.autoplay,
+      autoplay,


we should move this up next to source, so, the object is consistent

gkatsev · 2018-05-28T14:16:02Z

src/js/player.js

+   * @param {boolean|string} [value]
+   *        - true: autoplay using the browser behavior
+   *        - false: do not autoplay
+   *        - See `manualAutoplay_` for valid string values


best to enumerate the options here instead of in manualAutoplay_

gkatsev · 2018-05-28T14:18:22Z

src/js/player.js

+    // a setAutoplay call. This option will be passed
+    // in the constructor for the tech
+    if (this.tech_) {
+      this.techCall_('setAutoplay', techAutoplay);


I think we only want to do this if autoplay is boolean otherwise, it will add the autoplay attribute in the html5 tech which we don't want.

I think I did this because in the case where autoplay is currently true on the tech, we want to set it to false, maybe that doesn't remove the attribute like I think that it should, I will test this out as well.

Seems like the attribute will be removed by setting the property to false. I think we do want this.

gkatsev · 2018-05-28T14:20:03Z

src/js/player.js

+      this.options_.autoplay = value;
+      // any other value sets autoplay to true
+    } else {
+      this.options_.autoplay = true;


this is probably fine, alternatively, we can cast the value into a boolean and use that instead.

Let me see what the video element does, I think any value on autoplay is treated as true.

OK so it seems that the browsers cast it to a bool, so I will change that here.

gkatsev · 2018-05-28T14:20:29Z

src/js/player.js

+      this.options_.autoplay = true;
+    }
+
+    techAutoplay = techAutoplay || value;


should this be techAutoplay || this.options_.autoplay?

gkatsev · 2018-05-28T14:21:15Z

test/unit/player.test.js

@@ -1904,3 +1904,253 @@ QUnit.test('disposing a tech that dit NOT set a poster, should keep the poster',

  player.dispose();
 });
+
+QUnit.module('Player autoplay', {


maybe better to move this into a separate file?

yeah I will do that.

gkatsev · 2018-05-28T14:22:38Z

src/js/player.js

+    let techAutoplay;
+
+    // if the value is a valid string set it to that
+    if (typeof value === 'string' && (/(any|play|muted)/).test(value)) {
      this.options_.autoplay = value;


should we change how we cache this value? i.e., switch to using this.cache_.autoplay instead and keep the options autoplay what it was initially?
We can also keep updating the option but use the cache until a breaking change

I think that autoplay is actually an option, and every time we have to switch techs we have to pass it to the tech as such. I see the cache as more of a cache of the techs value on our end so maybe we should cache something like techAutoplay_?. I think autoplay as a whole is more of an option, but I do see the value in being able to see what the tech has autoplay set to.

basically, I think we should keep this.options_ set to whatever is the initial set of options passed it and use other places to cache transient or instance properties. However, I don't think this is necessarily something for this PR.

misteroneill

Code looks solid to me. Some documentation cleanup, though.

misteroneill · 2018-05-30T18:52:53Z

docs/guides/faq.md

-```html
-<video autoplay controls class="video-js">
-```
+Do to recent changes in `autoplay` behavior we no longer recommend using the `autoplay` attribute


Do -> Due

I don't think the first autoplay should use backticks. It's not referencing the attribute or any code, but the behavior.

misteroneill · 2018-05-30T18:55:37Z

docs/guides/faq.md

-<video autoplay controls class="video-js">
-```
+Do to recent changes in `autoplay` behavior we no longer recommend using the `autoplay` attribute
+on the video element. It's still supported by video.js but many browsers, including Chrome, are changing their


video should probably have backticks as it refers to the element.

video.js should be capitalized.

but should be preceded by a comma.

misteroneill · 2018-05-30T18:56:22Z

docs/guides/faq.md

-
-player.autoplay(true);
-```
+Instead we recommend using the `autoplay` option rather than the `autoplay` attribute. For more information on using that.


The period at the end should be a comma.

misteroneill · 2018-05-30T18:56:48Z

docs/guides/faq.md

-player.autoplay(true);
-```
+Instead we recommend using the `autoplay` option rather than the `autoplay` attribute. For more information on using that.
+See the [autoplay option][autoplay-option] in the videojs options guide.


See should be see

videojs should be Video.js

misteroneill · 2018-05-30T18:58:15Z

docs/guides/faq.md

-```html
-<video muted autoplay playsinline>
-```
+We do not recommend doing this manually using attributes on the video element. Instead you should pass the


video should probably have backticks as it refers to the element.

Instead should be followed by a comma.

misteroneill · 2018-05-30T18:59:09Z

docs/guides/faq.md


-Will make an inline, muted, autoplaying video on an iPhone with iOS10.
+> NOTE: At this point the autoplay attribute and option are NOT a guarantee that your video will autoplay.


At this point should be followed by a comma.

misteroneill · 2018-05-30T19:00:14Z

docs/guides/options.md

@@ -53,11 +53,34 @@ Each of these options is also available as a [standard `<video>` element attribu

 ### `autoplay`

-> Type: `boolean`
+> Type: `boolean|string`
+> NOTE: At this point the autoplay attribute and option are NOT a guarantee that your video will autoplay.


At this point should be followed by a comma.

misteroneill · 2018-05-30T19:01:08Z

docs/guides/options.md

+> NOTE2: If there is an attribute on the media element the option will be ignored.
+> NOTE3: You cannot pass a string value in the attribute, you must pass it in the videojs options
+
+Instead of using the `autoplay` attribute you should pass an `autoplay` option to videojs. The following values


I think videojs should have backticks here as it refers to the function rather than the library.

mister-ben

Knowing whether autoplay succeeded and whether muting was necessary will be important for some use cases (analytics, UX changes). Should there be an event or a property with this info for convenience?

mister-ben · 2018-06-04T09:47:35Z

docs/guides/faq.md

-var player = videojs('my-video', {
-  autoplay: true
-});
+For more information on the autoplay changes see our blog post: https://blog.videojs.com/autoplay-best-practices-with-video-js/


I think this section flows better with this line at the end.

mister-ben · 2018-06-04T09:48:41Z

docs/guides/faq.md

-player.autoplay(true);
-```
+Instead we recommend using the `autoplay` option rather than the `autoplay` attribute, for more information on using that.
+see the [autoplay option][autoplay-option] in the Video.js options guide.


no . after that

mister-ben · 2018-06-04T09:52:01Z

src/js/player.js

-      this.techCall_('setAutoplay', value);
+    // getter usage
+    if (value === undefined) {
+      return this.options_.autoplay;


This returns undefined rather than false if not configured.

brandonocasey · 2018-06-04T15:13:25Z

hmmm yeah it could be a good idea to have something like manual-autoplay-success and manual-autoplay-failure. Thoughts @gkatsev ?

brandonocasey · 2018-06-04T18:04:45Z

On thinking about this some more, I think we may want to do this for play in general. We might want to have play-fulfilled, play-rejected, and play-pending events. Naming is based on the states specified in the promise specification.

misteroneill · 2018-06-04T18:08:50Z

I think that idea has merit for sure. Maybe a property as well, e.g. lastPlayState() that returns a pseudo-symbol like videojs.[FULFILLED|REJECTED|PENDING]? Thinking out loud here.

gkatsev · 2018-06-04T18:12:05Z

src/js/player.js

+    let techAutoplay;
+
+    // if the value is a valid string set it to that
+    if (typeof value === 'string' && (/(any|play|muted)/).test(value)) {
      this.options_.autoplay = value;


basically, I think we should keep this.options_ set to whatever is the initial set of options passed it and use other places to cache transient or instance properties. However, I don't think this is necessarily something for this PR.

gkatsev · 2018-06-04T18:15:27Z

src/js/player.js

+    let techAutoplay;
+
+    // if the value is a valid string set it to that
+    if (typeof value === 'string' && (/(any|play|muted)/).test(value)) {


should we call this.manualAutoplay_ in here? Use case would be something like:

var player = videojs('vid'); player.autoplay('any');

I'd expect the player to autoplay in this case. This would also match current behavior (in browsers that support autoplay). Main risk is that we'd want to be able to do something like the following too

var player = videojs('vid', { autoplay: 'any' ); player.autoplay(false);

and have it not autoplay.

Also, should we have a string state "off" and deprecate boolean values for it?

Maybe just do this in a followup PR?

Should player.autoplay('any') also return a promise?

I think we probably just want events rather than a promise

gkatsev · 2018-06-04T18:20:34Z

Autoplay events probably make sense. We probably can have just two events autoplay-success and autoplay-failure and then have extra data on the event object or the data object for events (secondary arg) provide info on autoplay configuration.

I'm hesitant to map the play promise into events unless we have an actual use-case. Most of what we talked about it has been theoretical so far. This is especially since we have moved towards recommending people call play themselves and thus they will generally have a Promise available to them directly. We probably want to resurrect #3927 and actually implement it so that player.promise always returns a promise.

brandonocasey · 2018-06-04T20:12:08Z

See #5227

mister-ben · 2018-06-05T21:09:18Z

src/js/player.js

+      // if we are muted or not set to any here there is nothing we can do
+      if (type !== 'any' || this.muted()) {
+        this.trigger('autoplay-failure');
+        return;


If we've muted the player and the autoplay still fails, shouldn't we unmute again so a subsequent play triggered by a human has sound?

This sounds reasonable to me.

gkatsev · 2018-06-06T21:53:00Z

src/js/player.js

+    }
+
+    playPromise.then(() => {
+      this.trigger('autoplay-success');


should we add the autoplay value to the event or are we expecting users to call player.autoplay() to get the current value?

I think we can add the autoplay value to the event

gkatsev · 2018-06-06T21:53:09Z

src/js/player.js

+      // if we are muted or not set to any here there is nothing we can do
+      if (type !== 'any' || this.muted()) {
+        this.trigger('autoplay-failure');
+        return;


This sounds reasonable to me.

This PR extends the `autoplay` to the player with a few options that should hopefully make working with browsers that disable unmuted autoplay by default easier. The current boolean option will match current behavior and any unknown option will be treated as it does now. The new options are the string values `muted`, `play`, and `any`. - `muted` will mute the element and call `play()` on `loadstart`, - `play` will call `play()` on `loadstart()`, this is similar to the `autoplay` attribute - `any` will call `play()` on `loadstart()` but if it fails it will try muting the video and calling `play()` again.

feat: manual autoplay

b6b8a23

gkatsev reviewed May 24, 2018

View reviewed changes

unit tests, and option change

b3db5d9

misteroneill reviewed May 25, 2018

View reviewed changes

major refactor

a99a1de

brandonocasey force-pushed the feat/manual-autoplay branch from 4440097 to a99a1de Compare May 25, 2018 17:55

gkatsev reviewed May 28, 2018

View reviewed changes

brandonocasey added 6 commits May 29, 2018 11:24

move autoplay next to source

c5c3bae

link to blog post

5c3cde9

code review

e242be6

minor logic and comment tweaks

1670508

move autoplay tests to their own file

ea4af02

add a new test for bool casting

c8d2dba

brandonocasey force-pushed the feat/manual-autoplay branch from 4c28774 to c8d2dba Compare May 30, 2018 18:54

misteroneill approved these changes May 30, 2018

View reviewed changes

code review

9a7a3bf

misteroneill approved these changes May 30, 2018

View reviewed changes

mister-ben reviewed Jun 4, 2018

View reviewed changes

code review

794c984

gkatsev reviewed Jun 4, 2018

View reviewed changes

code review

4479826

events

e74c1fd

brandonocasey force-pushed the feat/manual-autoplay branch from 3565859 to e74c1fd Compare June 4, 2018 20:33

test refactor

971971e

mister-ben reviewed Jun 5, 2018

View reviewed changes

gkatsev reviewed Jun 6, 2018

View reviewed changes

pass autoplay value with event, retore muted

b97d718

gkatsev approved these changes Jun 15, 2018

View reviewed changes

gkatsev added the confirmed label Jun 15, 2018

gkatsev merged commit e8e4fe2 into master Jun 21, 2018

gkatsev deleted the feat/manual-autoplay branch June 21, 2018 18:32

gkatsev mentioned this pull request Jun 21, 2018

Autoplay (muted or unmuted) doesn't work in Chrome with latest video.js #5233

Closed


		Will make an inline, muted, autoplaying video on an iPhone with iOS10.
		> NOTE: At this point the autoplay attribute and option are NOT a guarantee that your video will autoplay.

feat: manual autoplay #5209

feat: manual autoplay #5209

Conversation

brandonocasey commented May 24, 2018 • edited by gkatsev Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marguinbc commented May 25, 2018

misteroneill left a comment

Choose a reason for hiding this comment

misteroneill commented May 25, 2018

marguinbc commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mister-ben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Jun 4, 2018

brandonocasey commented Jun 4, 2018 • edited Loading

misteroneill commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Jun 4, 2018

brandonocasey commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented May 24, 2018 •

edited by gkatsev

Loading

brandonocasey May 29, 2018 •

edited

Loading

brandonocasey May 29, 2018 •

edited

Loading

brandonocasey commented Jun 4, 2018 •

edited

Loading