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
Add fullscreen button to video controls #24179
Add fullscreen button to video controls #24179
Conversation
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @ferjm (or someone else) soon. |
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.
Thanks! Looking good :)
I replied to your questions on the issue. I think we can make the toggleFullscreen
method not only enter but also exit fullscreen mode with the API properties and method that I mentioned there.
Ok, I added the ability to exit fullscreen mode to the |
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.
Thanks! Only a couple of minor comments left.
resources/media-controls.js
Outdated
@@ -368,6 +387,16 @@ | |||
this.media.muted = !this.media.muted; | |||
} | |||
|
|||
toggleFullscreen() { | |||
const fullscreenEnabled = document.fullscreenEnabled && document.fullscreenElement; |
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 need to check if document.fullscreenElement
is the media element.
</span> | ||
<button id="volume-switch"></button> | ||
<input id="volume-level" type="range" value="100" min="0" max="100" step="1"></input> | ||
${isAudioOnly ? "" : '<button id="fullscreen-switch" class="fullscreen"></button>'} |
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.
It's really not a big deal, but it would be very nice if we could change the icon depending on whether we are out (icon with arrows pointing out) or in (icon with arrows pointing in) of fullscreen mode. Or use a neutral icon instead.
Thank you! @bors-servo r+ |
📌 Commit 9cbdfd6 has been approved by |
…ferjm Add fullscreen button to video controls This PR adds a fullscreen button to the video controls r? @ferjm --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24164 <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24179) <!-- Reviewable:end -->
☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster |
This PR adds a fullscreen button to the video controls
r? @ferjm
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is