-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Support fullscreen video on Android #325
Support fullscreen video on Android #325
Conversation
b7455fa
to
65657ce
Compare
@dvicory This works great! Wish I had seen it earlier this week before I did almost identical work on my own fork. Your implementation is much cleaner though, so going to use yours and hope it gets merged soon. Nice work! Edit: After some further testing I ran into an issue trying to fullscreen videos that were captured in portrait orientation. More details below. |
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
65657ce
to
0c7c489
Compare
@Titozzz and @smathson sorry for the very late followup, but I've addressed the original issue of forcing orientation. Currently, I've made the best fullscreen experience only for KitKat or greater. I hope that's acceptable as getting it working well pre-KitKat turned out to be quite a challenge. Fullscreen video will still work there, just with the navigation and status bars. Right now it uses sticky immersive mode (for KitKat and above), doesn't force orientation, and works well if a user locks the screen or backgrounds the app. Let me know if I can address anything further. |
@dvicory Looks great to me. I won't have a chance to test this in our production app anytime soon but I'll try to run it through a test app tomorrow or early next week. |
@dvicory Any change you can get a look at the conflicts? I merged quite a lot of stuff recently |
0c7c489
to
3dc3f73
Compare
@Titozzz Rebased and resolved conflicts. Also I've tested on the following devices we have: Nexus 7 with Android 4.3 Couldn't find our phone with KitKat unfortunately. |
Great, thanks |
3dc3f73
to
d89a3db
Compare
d89a3db
to
becb4ad
Compare
did you checked how embedded youtube works in fullscreen? I've tried your PR, but there was an issues:
|
@Strate I experienced the same problem on my Nexus 6P with a custom ROM (Pixel Experience), but attributed it to the custom ROM... Other Nexus, Samsung, and Pixel phones have no problems. I would appreciate any help in investigating what's going wrong here. I don't know that this should be a blocker to merging? |
@Titozzz actually, this happens with other webview components (I've tried https://github.com/teamairship/react-native-android-fullscreen-webview-video). Looks like this is a youtube-related problem, because Vimeo works great. |
So you are saying we can merge this as is? |
@Titozzz I can't say this, because actually I am not an android developer, I just tried to find out youtube fullscreen problem, found this PR and tested it ) |
Manually applied the changes to test this out. This is for youtube and vimeo embed in a website. (Huawei Nova 3) For youtube embed, the fullscreen icon is not greyed out anymore. But when I tap on the icon, nothing happens. Seeing this As for vimeo embed, when I tap on the fullscreen icon, the screen becomes white colour. But the video is actually already full screen and playing below the white screen. |
Is there anything that I can do to help this PR to get merged? I was thinking of creating a repo with this branch and try to reproduce the bug. Would it help? |
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.
I'm trying to implement this feature on my app. So I get this modified file and manually applied the changes to test. Here's the tests result bellow:
Galaxy S8 - Android 9.0. - Fullscreen button working fine.
Lenovo Z2 Plus - Android 7.1.2. - Fullscreen button working fine.
Tested on 3 Moto X Play with Android 7.1.1. - Fullscreen button working fine.
Samsung Galaxy J7 - Android 6.0.1 - Fullscreen button working fine.
One of these Moto X Play is mine and the only strange issue that hapend on my cellphone is: The Fullscreen button appears enabled but nothing happens on tap at first try, so I uninstalled the app and run react-native run-android --deviceId=<MY_DEVICE_ID_HERE> again and it works fine.
Unfortunately, I'm not able to analyze and suggest any changes on the Java Code.
I hope I helped, and I'm waiting this solution gets fully merged soon.
Thanks :D
I tested it on my Galaxy Tab S2 with Android 7.0 and in fullscreen, my screen gets black, if my rotation is portrait (sound works and video is clickable), in landscape mode, it works. |
If anyone can find the reasons for the last few bugs it would be great as it already looks like it's working on many devices |
can we ship this as experimental? would this break other features? |
I could. I can always revert if something gets broken but I guess it should only affect people using full screen and since it's broken right now.. |
That would be great and easier for me to test it! @dvicory |
@karvulf Okay, I made a branch with the TypeScript built. You should be able to add it like so: |
Thank you very much! It works @dvicory |
Nope, you already rock 😍 |
# [5.9.0](v5.8.2...v5.9.0) (2019-05-16) ### Bug Fixes * **Android:** geolocation access ([#562](#562)) ([409b9ae](409b9ae)) ### Features * **fullscreen videos:** Support fullscreen video on Android ([#325](#325)) ([d72c2ae](d72c2ae)) * **onScroll:** Add `onScroll` callback for iOS & Android ([#516](#516)) ([e4c8dab](e4c8dab))
🎉 This PR is included in version 5.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@allcontributors[bot] please add @dvicory for code doc |
@MalcolmScruggs I've put up a pull request to add @dvicory! 🎉 |
Nice job fellows! |
Any ideas about getting notified when enter/exit fullscreen? I'm seeking for a way on Android to unlock the orientations when the webview enters fullscreen mode to enable auto-rotate feature, and lock it again when exits. WKWebView handles this case well on iOS side, so it's a bit inconsistent on this. Temporarily my workaround is changing the Java code to emit exit and enter events. I don't know if it's a good idea to add another callback for Android : /. |
Fix missing `allowsFullscreenVideo` prop after react-native-webview#325 got merged.
Fix missing `allowsFullscreenVideo` prop after #325 got merged.
Fix missing `allowsFullscreenVideo` prop after #325 got merged.
…ative-webview#325) * Extract WebChromeClient from an anonymous class * Support fullscreen videos on Android Forces landscape mode while playing. * Use sticky immersive mode for fullscreen videos No longer forces landscape mode as that is a problem for portrait videos - allow the user to rotate as necessary. Only supports KitKat or greater, and falls back to leaving the status and navigation bars visible for lower than KitKat. This is the easiest way to prevent issues with resizing the video during playback. Also implement a lifecyle event listener which means if a user backgrounds the app or locks the screen with the video fullscreen, the UI visibility is re-applied. * Add allowsFullscreenVideo prop to control whether videos can be fullscreen on Android Luckily, we're able to change the WebChromeClient on demand in response to prop changes without seeming to do any harm. If you switch to disallow fullscreen, it will attempt to close the currently fullscreened video (if there is one) so users aren't stuck. I did notice a bug that if you go from fullscreen allowed, to fullscreen disallowed, the fullscreen button will remain on the video. Tapping the button will have no effect.
# [5.9.0](react-native-webview/react-native-webview@v5.8.2...v5.9.0) (2019-05-16) ### Bug Fixes * **Android:** geolocation access ([react-native-webview#562](react-native-webview#562)) ([409b9ae](react-native-webview@409b9ae)) ### Features * **fullscreen videos:** Support fullscreen video on Android ([react-native-webview#325](react-native-webview#325)) ([d72c2ae](react-native-webview@d72c2ae)) * **onScroll:** Add `onScroll` callback for iOS & Android ([react-native-webview#516](react-native-webview#516)) ([e4c8dab](react-native-webview@e4c8dab))
Fix missing `allowsFullscreenVideo` prop after react-native-webview#325 got merged.
# [5.9.0](react-native-webview/react-native-webview@v5.8.2...v5.9.0) (2019-05-16) ### Bug Fixes * **Android:** geolocation access ([#562](react-native-webview/react-native-webview#562)) ([409b9ae](react-native-webview/react-native-webview@409b9ae)) ### Features * **fullscreen videos:** Support fullscreen video on Android ([#325](react-native-webview/react-native-webview#325)) ([d72c2ae](react-native-webview/react-native-webview@d72c2ae)) * **onScroll:** Add `onScroll` callback for iOS & Android ([#516](react-native-webview/react-native-webview#516)) ([e4c8dab](react-native-webview/react-native-webview@e4c8dab))
Fix missing `allowsFullscreenVideo` prop after react-native-webview/react-native-webview#325 got merged.
Fixes #294.
Let me know if we want to add anything as part of this.