-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
feat: Add normalizeAutoplay option to treat autoplay: true as autoplay: "play" #7190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document normalizeAutoplay
in https://github.com/videojs/video.js/blob/main/docs/guides/options.md
Also, worth looking into writing a test (probably in player.test.js because that's were some of this already exists) where we create a player based on a <video>
element with an autoplay
attribute but that has the attribute removed on init, I think this part is relatively straight forward. The trickier part, if testable, is whether adding a source will autoplay via our manual autoplay or via the video element autoplaying.
src/js/player.js
Outdated
@@ -3684,6 +3688,14 @@ class Player extends Component { | |||
} else if (!value) { | |||
this.options_.autoplay = false; | |||
|
|||
// normalize `true` as 'play' if need be | |||
} else if (value === true && this.options_.normalizeAutoplay) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check be combined into the if on line 3681 above? This way we aren't duplicated the lines below.
@gkatsev Pushed those changes, including a new test. Regarding your question about testing "whether adding a source will autoplay via our manual autoplay or via the video element autoplaying," isn't this sort of tested already by making sure |
No, that only verifies that <video autoplay id=vid>
</video> At this point, the video element is primed for autoplay. If you were to put a source in, it will start playing it. const player = videojs('vid', {
normalizeAutoplay: true
});
player.src('video.mp4');
// playback should be initiated via Video.js calling `play()`
// rather than the video element getting a source starting playback because it was already primed to do so It's probably tricky to get a test for this. Let me know whether this makes sense, happy to talk it about it some more. |
test/unit/player.test.js
Outdated
|
||
player.loadTech_('Html5'); | ||
|
||
assert.equal(player.options_.autoplay, true, 'autoplay option is set to true'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be player.autoplay()
instead to make it a bit more realistic of a test?
Co-authored-by: Gary Katsevman <git@gkatsev.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
…y: "play" (videojs#7190) This PR adds a new option to treat autoplay: true the same as autoplay: 'play'. In general we want video.js to handle as much of the autoplay logic itself as possible, since the browser's treatment of the video element's autoplay attribute is less predictable. For now the default option value is false, but it may be made the default behavior in a future major version.
Description
This PR adds a new option to treat
autoplay: true
the same asautoplay: 'play'
. In general we want video.js to handle as much of the autoplay logic itself as possible, since the browser's treatment of the video element'sautoplay
attribute is less predictable. For now the default option value isfalse
, but it may be made the default behavior in a future major version.