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

Allow 'playsinline' player option #4325

Merged
merged 9 commits into from
May 10, 2017

Conversation

alex-barstow
Copy link
Contributor

@alex-barstow alex-barstow commented May 4, 2017

Description

Video.js players can accept a number of standard <video> element options (autoplay, muted, loop, etc), but not currently playsinline, which is now part of the HTML spec. We should add it to the list of <video> attributes that can be provided to the player as options.

Specific Changes proposed

Update the Html5 and Player classes with getter and setter methods for playsinline and make a few other additions to ensure that the playsinline option works the same way as the other <video> element options.

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

@alex-barstow
Copy link
Contributor Author

I assume this will need a sister PR in master as well

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. Just some changes in the doc comments. playsinline is now generic signal to tell the vendor to play the video in-line. iOS is just the main (only?) example of such a vendor.

src/js/player.js Outdated
* Get or set the playsinline attribute.
*
* @param {boolean} [value]
* - true means that we should play inline in iOS Safari
Copy link
Member

Choose a reason for hiding this comment

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

I would change these two say set the playsinline attribute or unset it. And mention above that this probably only applies on ios right now.

@@ -1235,6 +1235,20 @@ Html5.resetMediaElement = function(el) {
'autoplay',

/**
* Get the value of `playsinline` from the media element. `playsinline` indicates
* that the media should play inline in iOS Safari.
Copy link
Member

Choose a reason for hiding this comment

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

iOS is an example of a browser that will only play inline if the attribute is present.

@@ -1518,6 +1532,19 @@ Html5.resetMediaElement = function(el) {
'autoplay',

/**
* Set the value of `playsinline` on the media element. `playsinline` indicates
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@gkatsev gkatsev added the 5.x label May 8, 2017
@gkatsev
Copy link
Member

gkatsev commented May 8, 2017

Also, yes, a PR against master would be good. Can be done once this PR is approved, though.

@gkatsev
Copy link
Member

gkatsev commented May 9, 2017

@alex-barstow While testing, I found that if I have a video embed with playsinline attribute, calling player.playsinline() after the player loads doesn't reflect that value.

Also, we should add an @abstract method in tech.js for this.

*
* @see [Spec]{@link https://html.spec.whatwg.org/#attr-video-playsinline}
*/
Html5.prototype.playsinline = function() {
Copy link
Member

Choose a reason for hiding this comment

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

you should move these up and use the class syntax

@@ -1553,6 +1574,30 @@ Html5.resetMediaElement = function(el) {
};
});

// This setter is declared outside of the loop because there is no 'playsinline'
Copy link
Member

Choose a reason for hiding this comment

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

We can just describe this in the PR instead, don't think it's necessary, especially if we move it into the class syntax.

@@ -2381,6 +2382,29 @@ class Player extends Component {
}

/**
* Set or unset the playsinline attribute.
* Playsinline tells the browser that non-fullscreen playback is preferred.
*
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a @see here as well?

@imbcmdth
Copy link
Member

imbcmdth commented May 9, 2017

@gkatsev @alex-barstow Should we have any testing like https://github.com/videojs/video.js/blob/master/test/unit/player.test.js#L94 where we make sure autoplay is propagated from the video element?

@gkatsev
Copy link
Member

gkatsev commented May 9, 2017

@imbcmdth Perhaps adding it would be useful, but the tests are already testing that if attributes are present they persist when videojs wraps the video element.
I guess we might want to test the setter as well.

@gkatsev
Copy link
Member

gkatsev commented May 10, 2017

@alex-barstow would you be able to make the sister PR against master for this as well?

@gkatsev gkatsev merged commit 946f84b into videojs:5.x May 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants