-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 video to go complete fullscreen on Android #2325
Conversation
Hide the navbar on Android devices with onscreen navigation. Fixes #2324
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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.
Thank you for the contribution!
Unfortunately, the linter will complain about the style in this change. We use single quotes by convention. In the future, please run python build/all.py to run the linter and compiler yourself before submitting a PR.
In this case, the compiler will also complain because it doesn't know about this additional argument to requestFullscreen. It was only added in Chrome 71 about a month ago, and the "externs" the compiler uses to define the built-in APIs in the browser don't have this definition.
The latest version of the compiler has the updated definition, but we are unable to upgrade it right now because of other issues in our codebase. So this will be tricky to solve, and may require some kind of workaround.
Since this compiler is so complicated, I will update your change for you and test it before merging.
Thanks for bringing this to our attention!
This updates the fullscreen externs to: - Add a newer definition for requestFullscreen which includes options for the method - Remove prefixed methods which are now built into the compiler and are no longer necessary - Drop document.webkitSupportsFullscreen method, which does not seem to exist as defined, even on iOS (confusion with similar property on video element: https://apple.co/39TLC5I) This enables PR #2325 to resolve issue #2324 Change-Id: Ie7bea7e7d4fd59d6d801431e2ba996649e185dc1
Just pushed a workaround for the compiler issue in 51c3717. |
@joeyparrish, thank you. Didn't have the dev environment set up, but will do next time. Thank you for your help. |
Always happy to help! |
This updates the fullscreen externs to: - Add a newer definition for requestFullscreen which includes options for the method - Remove prefixed methods which are now built into the compiler and are no longer necessary - Drop document.webkitSupportsFullscreen method, which does not seem to exist as defined, even on iOS (confusion with similar property on video element: https://apple.co/39TLC5I) This enables PR #2325 to resolve issue #2324 Backported to v2.5.x Change-Id: Ie7bea7e7d4fd59d6d801431e2ba996649e185dc1
This PR has been cherry-picked for v2.5.8. |
Hide the navbar on Android devices with onscreen navigation.
Fixes #2324
Details about this fullscreen option can be found on Fullscreen API documentation:
https://developer.mozilla.org/en-US/docs/Web/API/FullscreenOptions/navigationUI