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

Jump to bottom

Add a fullscreen button to the video controls #24164

Closed
ferjm opened this issue Sep 9, 2019 · 10 comments · Fixed by #24179
Closed

Add a fullscreen button to the video controls #24164

ferjm opened this issue Sep 9, 2019 · 10 comments · Fixed by #24179
Labels
A-content/media C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor. L-css CSS is required L-javascript Javascript is required
Projects
Media playback

Comments

@ferjm
Copy link
Contributor

ferjm commented Sep 9, 2019

We need to add a new button to the media controls to make the video enter and exit fullscreen mode. The button should only be visible for video elements.

Relevant code:

Fullscreen API: https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API
@ferjm ferjm added E-less-complex Straightforward. Recommended for a new contributor. L-javascript Javascript is required L-css CSS is required A-content/media labels Sep 9, 2019
@highfive
Copy link

highfive commented Sep 9, 2019

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@ferjm ferjm added this to To do in Media playback Sep 9, 2019
@elaye
Copy link

elaye commented Sep 9, 2019

@highfive: assign me

@highfive highfive added the C-assigned There is someone working on resolving the issue label Sep 9, 2019
@highfive
Copy link

highfive commented Sep 9, 2019

Hey @elaye! Thanks for your interest in working on this issue. It's now assigned to you!

@elaye
Copy link

elaye commented Sep 10, 2019 •
edited

Hi @ferjm, I'm looking at this issue and I'm trying to play a video in Servo to see what the controls look like at the moment. I test with a simple html file and mp4 video with the following code:

<!DOCTYPE html>
<html>
  <body>
    test
    <video src="./video-small.mp4" type="video/mp4" controls autoplay></video>
  </body>
</html>

The video displays correctly and starts to play (it stops after a few frames but I assume it's another issue), however I can't see the controls.
I saw this comment that says that the shadow DOM must be enabled with the dom.shadowdom.enabled pref when launching servo, but it doesn't work either.

Is this still valid ? Do I need to do something else to be able to see the controls ?

I'm running Servo on MacOS 10.14.6 (Mojave) if that's of any use.

@ferjm
Copy link
Contributor Author

ferjm commented Sep 10, 2019

@elaye thanks for your interest in working on this issue.

I'm sorry I forgot to add this to the description. There is no need to set any preference anymore but the media controls are only rendered for block elements at the moment #24015. So adding "style = display: block" to your <video> element should make the controls appear.

(it stops after a few frames but I assume it's another issue)

Ugh... that shouldn't happen. Could you file another issue for this, please? Thanks.

@elaye elaye mentioned this issue Sep 11, 2019
Merged
3 tasks
@elaye
Copy link

elaye commented Sep 11, 2019 •
edited

Thank you for the additional information @ferjm. It does work with the display: block. I've made a pull request that adds a fullscreen button to the video controls. However I haven't added the ability to exit the fullscreen by clicking on the button again as I'm not sure what's the best way to do it.

Is there a property that I can read on the media that tells me if the video is fullscreen or do I need to handle the promise returned by requestFullscreen to set a local variable to keep track of the fullscreen state ?

@ferjm
Copy link
Contributor Author

ferjm commented Sep 11, 2019

However I haven't added the ability to exit the fullscreen by clicking on the button again as I'm not sure what's the best way to do it.

You can use the document.exitFullscreen() method.

Is there a property that I can read on the media that tells me if the video is fullscreen or do I need to handle the promise returned by requestFullscreen to set a local variable to keep track of the fullscreen state ?

There is no property specific for the media element, but you can check the document.fullscreenEnabled and document.fullscreenElement properties.

@elaye
Copy link

elaye commented Sep 11, 2019

Ok I've added the exit capability. I've noticed an odd behaviour with it though. When I enter fullscreen mode the window is maximized (at least on macOS) and the video is maximized inside the window. However when I exit the fullscreen mode the video recovers it's original size within the window but the window stays maximized.

@ferjm
Copy link
Contributor Author

ferjm commented Sep 12, 2019

However when I exit the fullscreen mode the video recovers it's original size within the window but the window stays maximized.

That seems to happen for any fullscreen element. I'll file a separated issue for this. Thanks

@ferjm
Copy link
Contributor Author

ferjm commented Sep 12, 2019

#24200

Media playback automation moved this from To do to Done Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/media C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor. L-css CSS is required L-javascript Javascript is required
Projects
Media playback
  
Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add fullscreen button to video controls elaye/servo
3 participants
@ferjm @elaye @highfive