-
Notifications
You must be signed in to change notification settings - Fork 24.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
Add support for Android TV devices #16500
Add support for Android TV devices #16500
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
cc @dlowder-salesforce |
Apple TV supports |
We will try to add support for |
Just linking this here: #16596 The other PR makes it possible to run apps on post 6.0 TV devices as since 6.0 it is required for the overlay permission to be set in the settings that are missing on Google's TV version of Android (the overlay settings of course, not all the settings) |
My PR should make this no longer relevant: @krzysztofciombor can you update your description to include info about #16596 |
c547763
to
8d89d89
Compare
Any idea on how long it will be before this gets merged in? I would love to use it. |
Hey @spunkedy! I have reviewed this PR submitted first in our fork here software-mansion-labs#1 so I hope this will also get approved by current maintainers. It is now blocked though on #16596 which is required for TV devices running Android 6 and above. If you have time you can pull this PR in, test it out and report here. This kind of input will def help maintainers while considering this PR. |
8d89d89
to
0c361d8
Compare
@facebook-github-bot label Android Generated by 🚫 dangerJS |
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.
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@krzysztofciombor thanks a lot for this PR. It will help a lot. this seem to be working fine. |
Hi everyone, do we have an ETA on when this pull request will be merged and available on master? |
This causes a crash during internal test running. Does it look legit?
|
@krzysztofciombor I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
0c361d8
to
099b841
Compare
Generated by 🚫 dangerJS |
@shergin Yes, the issue here was that @mandhon Sorry for the delayed response. So the thing is this is actually similar to a native Android TV behavior. This differs from Apple TV behavior, but it would be difficult to unify those behaviors and still achieve native look-and-feel. We are planning on a follow-up PR which will enable more granular control over focus engine for AndroidTV, but that's out of scope for this PR. (see: https://developer.android.com/training/tv/start/navigation.html#modify-d-pad-nav for reference) |
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.
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
(See my comment in the code.)
@@ -201,6 +203,53 @@ public boolean onTouchEvent(MotionEvent ev) { | |||
return true; | |||
} | |||
|
|||
@Override |
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.
Obviously, we would love to have first party support of Android TV in RN! Krzysztof, thank you so much for your effort! ❤️
However, we discussed this approach internally at Facebook and we mostly concern about this place. Given that fact that Android TV is still pretty marginal platform, having AndroidTV-specific code in this base class looks... excessive. We are worried about performance and unpredictable/undesirable implications.
Is there a way to restructure this somehow to remove explicit mentioning of TV from the most basic and important classes?
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.
Hey! Thanks for your time spent on reviewing this
Before we move on with making any changes, I’d like to list a two options and learn what your opinions are. But at first I’d like to emphasize two things:
a) Even though this PR is primarily for Android TV support most of the changes done here apply not only to TV devices but also to most of the phones with accessibility features turned on and to devices that are more commonly used with hardware keyboards (e.g. tablets). From the developer perspective a TV remote is treated as it was a hardware keyboard with a limited set of keys (just arrow keys, return and play/pause buttons). This is also the case for accessibility gestures that allows people with low vision to navigate through the app by swiping down/left/right/up which triggers arrow key press events. This has not yet been supported in RN apps on Android. See this for more info about what can be handled in dispatchKeyEvent
: https://developer.android.com/reference/android/view/KeyEvent.html
b) Unlike with iOS on Android we have limited options when it comes to detecting TV environment. On iOS this can be done in compile time as Apple TV devices gets separate binaries, whereas on Android we can only do runtime checks. On modern Android TV devices we can perhaps rely on LEANBACK_LAUNCHER intent filter and provide a separate root activity that would be configured differently but then this won’t work on Fire Stick TV devices that are quite popular.
Here are two options that I think we should consider:
- As I don’t share your concern regarding performance I understand Android TV references in Root View may look a bit off. The reason I am not concerned about perf here is that the events are only going to be generated when hardware keyboard buttons are being pressed (as mentioned this includes tv remotes, real keyboards or D-PADs attached to Android tablets or phones, and accessibility gestures). These type of events should not be more frequent than “text change” evens we already dispatch for TextInput component or hardware back button press we handle in ReactActivity class. My suggestion would be therefore to rename ReactAndroidTVRootViewHelper to something that is more generic and not referencing TV (e.g. ReactAndroidHWInputDeviceHelper or maybe ReactAndroidKeyboardNavigationHelper). Also to get rid of most of the “TV” reference from that helper class comments and method names as they really are not specific to TV devices.
- Extract HW input related methods to a separate root view class that would also extend ReactRootView. It is then possible to use a custom main activity subclass that would use that other root view instead of the default one. For that step we’d need to add additional setup instructions to the docs for people who want their app to work on Android TV and other devices with hardware input capabilities.
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.
cc @mdvacca
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.
Hey @kmagiera thank you for your effort!
I like the option 1) renaming this with ReactAndroidHWInputDeviceHelper, my concern with this approach is this line: https://github.com/facebook/react-native/pull/16500/files#diff-c308f7d4634ee2c75c99e773916f5e53R57
This means that for every key we will search for the focusedView and that is expensive. Maybe we could execute the findFocusedView(mReactRootView) only when the keyEvent is one of the events we handle?
@mdvacca can you please take a look at this discussion here: #16500 (comment) |
@@ -12,6 +12,7 @@ | |||
'use strict'; | |||
|
|||
const React = require('React'); |
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.
React is never used, lets remove it
Libraries/Utilities/Platform.ios.js
Outdated
@@ -28,6 +28,9 @@ const Platform = { | |||
const constants = NativeModules.PlatformConstants; | |||
return constants ? constants.interfaceIdiom === 'tv' : false; | |||
}, | |||
get isTV() { |
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.
Can we add a comment to isTVOS describing that it is deprecated and it will be replaced by isTV().
Also can you move its implementation to isTV(), that way it is easier to deprecate in the future
@@ -143,6 +151,16 @@ public boolean onKeyUp(int keyCode, KeyEvent event) { | |||
return false; | |||
} | |||
|
|||
public boolean onKeyLongPress(int keyCode, KeyEvent event) { | |||
if (getReactNativeHost().hasInstance() && getReactNativeHost().getUseDeveloperSupport()) { | |||
if (keyCode == KeyEvent.KEYCODE_MEDIA_FAST_FORWARD) { |
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.
NIT: can you unify these two IFs conditions?
int eventKeyCode = ev.getKeyCode(); | ||
int eventKeyAction = ev.getAction(); | ||
View targetView = findFocusedView(mReactRootView); | ||
if (targetView != null) { |
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.
Let early exit of the method if targetView == null || eventKeyAction != KeyEvent.ACTION_UP:
if (targetView == null || eventKeyAction != KeyEvent.ACTION_UP) { return; } ...
public void handleKeyEvent(KeyEvent ev, RCTDeviceEventEmitter emitter) { | ||
int eventKeyCode = ev.getKeyCode(); | ||
int eventKeyAction = ev.getAction(); | ||
View targetView = findFocusedView(mReactRootView); |
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.
The findFocusedView method transverses the entire viewHierarchy for every keyEvent on the ReactRootView. We need to do this only for TV
@@ -201,6 +203,53 @@ public boolean onTouchEvent(MotionEvent ev) { | |||
return true; | |||
} | |||
|
|||
@Override |
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.
Hey @kmagiera thank you for your effort!
I like the option 1) renaming this with ReactAndroidHWInputDeviceHelper, my concern with this approach is this line: https://github.com/facebook/react-native/pull/16500/files#diff-c308f7d4634ee2c75c99e773916f5e53R57
This means that for every key we will search for the focusedView and that is expensive. Maybe we could execute the findFocusedView(mReactRootView) only when the keyEvent is one of the events we handle?
This allows the app to be launched on Android TV devices as well
Contains code for handling KeyEvents related to Android TV navigation
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.
Added few minor comments inline
@@ -127,6 +127,14 @@ public void onActivityResult(int requestCode, int resultCode, Intent data) { | |||
} | |||
} | |||
|
|||
public boolean onKeyDown(int keyCode, KeyEvent event) { | |||
if (keyCode == KeyEvent.KEYCODE_MEDIA_FAST_FORWARD) { | |||
event.startTracking(); |
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.
should we only do this when getReactNativeHost().getUseDeveloperSupport()
is true?
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.
Hm, this check is currently done in onKeyLongPress
(https://github.com/facebook/react-native/pull/16500/files#diff-f3ee27db155d4af4368a3ff1e60a765cR156).
Do you think it should be moved to here, or do you think it's worth keeping it in both places?
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.
add it in both, we don't need to startTracking
unless we are in dev mode
super.onFocusChanged(gainFocus, direction, previouslyFocusedRect); | ||
return; | ||
} | ||
ReactContext reactContext = mReactInstanceManager.getCurrentReactContext(); |
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.
Maybe instead of doing all of this we could change visibility of ReactRootView.sendEvent to "package" and then pass ReactRootView reference to AndroidHWInputDeviceHelper so that we can call sendEvent
there directly
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.
Wouldn't that create a tight coupling between ReactRootView
and AndroidHWInputDeviceHelper
? What if someone would want to reuse the helper in some other place, which theoretically should be possible now since we've made it more generic?
@mdvacca We've addressed all your code review comments. |
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 working on this @krzysztofciombor.
The code updates look good to me.
CC: @ayc1
Looks like there are several conflicts - I'll take a stab at addressing these after the PR imports. |
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.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Just to let you know, anything you may have added to this PR after the bot imported the diff following my comment yesterday is not going to be included when the change is synced back to GitHub. |
So this PR did not land for a few reasons. There were some conflicts that needed to be addressed, which I handled manually after the PR was converted to a diff on our internal Phabricator instance. Once I did that, the diff started going through our test suite. It looks like an internal test may have prevented it from landing, but it's hard to tell now because updating the external PR after the import had already happened caused the diff to abort the land process. I'll re-import again and address conflicts if needed. Please do not add any more commits to the PR, otherwise we might need to go through the entire cycle again. |
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.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@hramos Could you please tell us what is the status of this pr now? |
@sunnylqm this has been merged into master 28 days ago: b7bb2e5 and made it to 0.55 release that went out just today: https://github.com/facebook/react-native/releases/tag/v0.55.0 |
Summary: <!-- Thank you for sending the PR! We appreciate you spending the time to work on these changes. Help us understand your motivation by explaining why you decided to make this change. You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html Happy contributing! --> * To be on par with Apple TV support, this makes it possible to run React Native apps on Android TV devices (See also: https://react-native.canny.io/feature-requests/p/android-tv-support) * These changes also make it possible to navigate through the app using D-PAD buttons that are present on some mobile devices * Since these changes affect, among others, `ReactRootView.java` and `Touchable.js` code and are closely related to Apple TV implementation, it makes sense for them to be included in the core - React native apps can be launched on Android TV devices and properly render their content - Navigation is possible using left, right, top, bottom arrows from the remote (or D-PAD) - Touchable components can handle D-PAD center button press events and correctly fire their `onPress` handlers - Touchable components will receive `onPressIn` and `onPressOut` events and can react to focus/blur changes appropriately (just like on Apple TV) - `Platform` constants allow to check if the react-native app is running on TV (`Platform.isTV`) - `ScrollView`s behave correctly (same as native implementation) when switching to view outside bounds – that is, the container would scroll such that the newly focused element is fully visible - Native "clicking" sounds are played when moving between focusable elements - Play/Pause click event is send to `TVEventHandler` - Rewind and FastForward events are send to `TVEventHandler` - Back button behaves as a normal Android back button - Diagonal buttons work correctly on Android TV, e.g. if there is no button directly to the right from the focused one, but there is one to the right but a bit higher/lower it will grab focus - Dev menu can be accessed by long pressing fast forward button A demo showing RNTester app running on Android TV device (Amazon Fire TV Stick) can be found here: [![RNAndroidTVDemo](http://img.youtube.com/vi/EzIQErHhY20/0.jpg)](http://www.youtube.com/watch?v=EzIQErHhY20) - `TextInput` will not work on Android TV devices. There's an issue with native `ReactEditText` implementation that prevents it from receiving focus. This makes it impossible to navigate to `TextInput`. This will be fixed next, but will be included in a separate Pull Request - ~Overlay permissions cannot be granted on Android TV devices running Android version >= 6.0 This is because the overlay permission can only be granted by firing an Intent to open settings page (`ACTION_MANAGE_OVERLAY_PERMISSION`). Since this page does not exist on TV devices the permission cannot be requested. This will make the app crash when trying to open dev menu (⌘+M) or displaying a redbox error. Note: This does not affect devices running Android version < 6.0 (for example Amazon Fire TV Stick)~ This is now fixed by: facebook/react-native#16596 * Launch the RNTester app on Android TV device. * Ensure it launches without a crash * Ensure basic navigation is possible * Ensure Touchable components can receive select events * Ensure the changes do not break current Android and iOS mobile devices functionality. * Ensure the changes do not break current Apple TV functionality. [RNAndroidTVDemo video](http://img.youtube.com/vi/EzIQErHhY20/0.jpg) * Added `ReactAndroidTVViewManager` that handles TV `KeyEvent`s and dispatches events to JS - This is the core that enables basic navigation functionality on Android TV devices * Following the above change we copy `TVEventHandler.ios.js` into `TVEventHandler.android.js` to enable JS to pick up those native navigation events and dispatch them further to subscribed views. (Note: We do not have a native `TVNavigationEventEmitter` implementation on Android, thus this file is slightly modified, e.g. it does pass `null` to `NativeEventEmitter` constructor) * Added `uiMode` to `AndroidInfoModule`. (**Note**: This required changing `extends BaseJavaModule` to `extends ReactContextBaseJavaModule` to be able to use `getSystemService` which requires `Context` instance! * Added `isTV` constants to both `Platform.ios.js` (keeping the deprecated `isTVOS` as well) and `Platform.android.js` * Changed condition check on `Touchable.js` to use the newly added `isTV` flag to properly handle TV navigation events on Android as well * Added `LEANBACK_LAUNCHER` to `RNTester` `intent-filter` so that it is possible to launch it on Android TV devices. * See also a PR to `react-native-website` repo with updated docs for Android TV: facebook/react-native-website#59 - [ ] Fix `TextInput` components handling by allowing them to be focused and making a proper navigation between them (and/or other components) possible. One thing to note here that the default behavior to immediately open software keyboard when focused on `TextInput` field will need to be adjusted on Android TV as well) - [x] Fix overlay permissions issue by changing the way redbox/dev menu are displayed (see: facebook/react-native#16596) - [ ] Adjust placement of TV-related files (e.g. the `TVEventHandler.js` file is placed inside `AppleTV` directory which is not accurate, since it does handle Android TV events as well) Previous discussion: software-mansion-labs/react-native#1 <!-- Help reviewers and the release process by writing your own release notes **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.** CATEGORY [----------] TYPE [ CLI ] [-------------] LOCATION [ DOCS ] [ BREAKING ] [-------------] [ GENERAl ] [ BUGFIX ] [-{Component}-] [ INTERNAL ] [ ENHANCEMENT ] [ {File} ] [ IOS ] [ FEATURE ] [ {Directory} ] |-----------| [ ANDROID ] [ MINOR ] [ {Framework} ] - | {Message} | [----------] [-------------] [-------------] |-----------| [CATEGORY] [TYPE] [LOCATION] - MESSAGE EXAMPLES: [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see --> [ANDROID] [FEATURE] [TV] - Added support for Android TV devices Closes facebook/react-native#16500 Differential Revision: D6536847 Pulled By: hramos fbshipit-source-id: 17bbb11e8583b97f195ced5fd9762f8902fb8a3d
What should I do if I want to fire the focus event manually on the Touchable components? |
Was this ever fixed? It does not work for me and I cannot find any PR... EDIT: It is possible to wrap inputs in Touchable elements and do |
The issue is still there. Did the touchable method work for you? |
For anyone still looking for a solution on handling the focus for TextInput, here is my implementation. I would rather have the onPress assign the ref to focus rather than onFocus. Instead, onFocus, I am applying an animation to up the scale of the input and when pressed, it focuses and pops up the keyboard. Code:
|
@mohamedshuaau you should be aware that Android TV is no longer supported in this repo -- support has moved to https://github.com/react-native-tvos/react-native-tvos/ . |
Add support for Android TV devices
Motivation
ReactRootView.java
andTouchable.js
code and are closely related to Apple TV implementation, it makes sense for them to be included in the coreSupported Android TV features
onPress
handlersonPressIn
andonPressOut
events and can react to focus/blur changes appropriately (just like on Apple TV)Platform
constants allow to check if the react-native app is running on TV (Platform.isTV
)ScrollView
s behave correctly (same as native implementation) when switching to view outside bounds – that is, the container would scroll such that the newly focused element is fully visibleTVEventHandler
TVEventHandler
A demo showing RNTester app running on Android TV device (Amazon Fire TV Stick) can be found here:
Caveats
TextInput
will not work on Android TV devices. There's an issue with nativeReactEditText
implementation that prevents it from receiving focus. This makes it impossible to navigate toTextInput
.This will be fixed next, but will be included in a separate Pull Request
Overlay permissions cannot be granted on Android TV devices running Android version >= 6.0This is because the overlay permission can only be granted by firing an Intent to open settings page (
ACTION_MANAGE_OVERLAY_PERMISSION
). Since this page does not exist on TV devices the permission cannot be requested. This will make the app crash when trying to open dev menu (⌘+M) or displaying a redbox error.Note: This does not affect devices running Android version < 6.0 (for example Amazon Fire TV Stick)
This is now fixed by: [ANDROID] [ENHANCEMENT] [DevSupport] Use currentActivity to display redbox, loading view and dev menu #16596
Test Plan
RNAndroidTVDemo video
Changes
ReactAndroidTVViewManager
that handles TVKeyEvent
s and dispatches events to JS - This is the core that enables basic navigation functionality on Android TV devicesTVEventHandler.ios.js
intoTVEventHandler.android.js
to enable JS to pick up those native navigation events and dispatch them further to subscribed views. (Note: We do not have a nativeTVNavigationEventEmitter
implementation on Android, thus this file is slightly modified, e.g. it does passnull
toNativeEventEmitter
constructor)uiMode
toAndroidInfoModule
. (Note: This required changingextends BaseJavaModule
toextends ReactContextBaseJavaModule
to be able to usegetSystemService
which requiresContext
instance!isTV
constants to bothPlatform.ios.js
(keeping the deprecatedisTVOS
as well) andPlatform.android.js
Touchable.js
to use the newly addedisTV
flag to properly handle TV navigation events on Android as wellLEANBACK_LAUNCHER
toRNTester
intent-filter
so that it is possible to launch it on Android TV devices.react-native-website
repo with updated docs for Android TV: Add docs for building for Android TV and unify them with Apple TV docs react-native-website#59Future work
TextInput
components handling by allowing them to be focused and making a proper navigation between them (and/or other components) possible. One thing to note here that the default behavior to immediately open software keyboard when focused onTextInput
field will need to be adjusted on Android TV as well)TVEventHandler.js
file is placed insideAppleTV
directory which is not accurate, since it does handle Android TV events as well)Previous discussion: software-mansion-labs#1
Release Notes
[ANDROID] [FEATURE] [TV] - Added support for Android TV devices