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 video to go complete fullscreen on Android #2325

Closed
wants to merge 1 commit into from
Closed

Allow video to go complete fullscreen on Android #2325

wants to merge 1 commit into from

Conversation

zarmhast
Copy link
Contributor

@zarmhast zarmhast commented Jan 4, 2020

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

Hide the navbar on Android devices with onscreen navigation.

Fixes #2324
@googlebot
Copy link

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@zarmhast
Copy link
Contributor Author

zarmhast commented Jan 4, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Member

@joeyparrish joeyparrish left a 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!

shaka-bot pushed a commit that referenced this pull request Jan 6, 2020
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
@joeyparrish
Copy link
Member

Just pushed a workaround for the compiler issue in 51c3717.

@joeyparrish
Copy link
Member

I pushed your change manually in 3dc0b17 with style fixes in ee91f4a. I expected github to notice and close the PR, but for some reason, it did not.

Thanks again!

@joeyparrish joeyparrish closed this Jan 6, 2020
@zarmhast zarmhast deleted the patch-1 branch January 7, 2020 03:02
@zarmhast
Copy link
Contributor Author

zarmhast commented Jan 7, 2020

@joeyparrish, thank you. Didn't have the dev environment set up, but will do next time. Thank you for your help.

@joeyparrish
Copy link
Member

Always happy to help!

joeyparrish added a commit that referenced this pull request Jan 15, 2020
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
@joeyparrish
Copy link
Member

This PR has been cherry-picked for v2.5.8.

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incomplete fullscreen on Android
3 participants