[ANDROID] [ENHANCEMENT] [DevSupport] Use currentActivity to display redbox, loading view and dev menu #16596

kmagiera · 2017-10-30T10:25:50Z

This change aims at replacing SYSTEM_ALERT_WINDOW/OVERLAY API being used for rendering dev support related views on Android (redbox, dev menu, green loading view) with API that does not require any special permission. The permission is still used for displaying perf monitor, although it is no longer requested at app startup but only when perf monitor gets enabled.

This change should not affect the way react native apps work in production environment as in release mode all dev support functionality is disabled.

Motivation

There are two main reasons why requiring SYSTEM_ALERT/OVERLAY permission for displaying basic dev related windows is problematic:

On Android >=6 devices it is required that overlay permission is granted in device settings for apps being side loaded (not installed via play store which is usually the case for apps being developed). Although this setting is not available on some Android devices including Google's stock Android TV version. On such devices App cannot be granted rights to draw in system alert window which cases the app to crash (instead of showing a redbox or dev menu dialog)
Some Android device vendors have issues with implementation of Settings.canDrawOverlays that always return false (I've seen it on Xiaomi Redmi 4A with Android 6.1). This issue because of the following code in ReactActivityDelegate.java#L90, results in the overlay permission settings screen popping up every time the app is launched even though the permission has been perviously granted which is extremely annoying. Since this change only require overlay permission for displaying perf monitor we no longer ask for it on startup but only when user switches perf monitor ON.

Test Plan

Test need to be performed on pre Android 6 and post Android 6 devices.

Run app with devserver off -> should result in redbox
Start packager with --reset-cache flag for the loading bar to be visible for some longer period of time. Then restart the app and see the loading bar show up
While the app is running, open dev menu, navigate to "dev settings", test "reload"
Modify JS app such that the app crashes, see it display redbox properly. Check if "reload" button works well from the redbox
Verify that "Show Perf Monitor" option works as expected. On Android >=6 re-install the app to see it ask for overlay permission at the moment when perf monitor option gets selected.

Release Notes

SYSTEM_ALERT_WINDOW permission will no longer be required on Android to display Redbox

Breaking

This change can break things for framework users who provide custom implementation of DevSupportManager interface on Android:

Who does this affect: Owners of apps that use custom implementation of DevSupportManager interface on Android.
How to migrate: Update create method of your DevSupportManager's factory to take ReactInstanceManagerDevHelper type as a second argument instead of ReactInstanceDevCommandsHandler. The interface ReactInstanceDevCommandsHandler has been renamed to ReactInstanceManagerDevHelper but kept all the methods the same (new method got added). If you were calling one of three methods from ReactInstanceDevCommandsHandler interface (onReloadWithJSDebugger, onJSBundleLoadedFromServer and toggleElementInspector) you can call exact same methods directly on ReactInstanceManagerDevHelper instance that is being provided in exchange for ReactInstanceManagerDevHelper .
Why make this breaking change:
This PR adds a new method to ReactInstanceManagerDevHelper called getCurrentActivity. In which case the prev name can no longer be justified. The activity is required for some of the DevSupportManager methods in order to start new dialogs and popups so that overlay permission isn't necessary.
Severity (number of people affected x effort):
Relatively small (perhaps Fb internally is using DevSupportManager abstraction to provide an alternative implementation but since it isn't documented I doubt anyone else uses it). Effort it very low as it boils down to updating uses of interface ReactInstanceDevCommandsHandler with ReactInstanceManagerDevHelper (all the methods in ReactInstanceDevCommandsHandler stays the same)

pull-bot · 2017-10-30T10:26:54Z

	Warnings
⚠️	📋 Release Notes - This PR may have incorrectly formatted Release Notes.

@facebook-github-bot label Needs more information

@facebook-github-bot label Core Team

Generated by 🚫 dangerJS

janicduplessis

Out of curiosity is there a reason why we can't use the same technique to get rid of system overlay in perf overlay too?

janicduplessis · 2017-10-31T04:31:42Z

ReactAndroid/src/main/java/com/facebook/react/devsupport/ReactInstanceDevCommandsHandler.java

+   * Return activity react views are mounted in. This will be used by DevSupportManager to launch
+   * dialogs (such as dev settings dialog or a red box)
+   */
+  @Nullable Activity getCurrentActivity();


Would there be another way to pass around the activity? It feels kind of out of place on this interface.

I agree, on the other hand new interface would complicate this part of the codebase even more which I don't think is necessary considering it doesn't do much.

Which of these two options would you prefer:

Rename this interface so that this method fits better

This interface implementation just passes all the calls to private methods of ReactInstanceManager. I could make those methods public and instead of passing ReactInstanceDevCommandsHandler ref to DevSupportManagerFactory I could pass ref to ReactInstanceManager

PS: I prefer the latter

Yea if it doesn't require too many changes I think number 2 would be nice

@janicduplessis updated this PR with this change. Note that now it is more likely that this code will break some internal fb stuff where they perhaps use their own implementation of devsupportmanager

kmagiera · 2017-11-02T14:46:01Z

@janicduplessis we could use this technique, but it makes stuff display in the current activity window. I figured it is fine for redbox and dev menu because when they appear you can't navigate to a different activity. This isn't true for perf overlay that only covers part of the screen and in theory you can open/close activities while you watch the perf overlay. This is a usecase for apps that use some sort of native navigation (homemade or one by wix or airbnb)

kmagiera · 2017-11-03T09:48:47Z

@janicduplessis As my latest changes may break stuff for ppl who provide alternative impl of DevSupportManager I added a section to the PRs description with migration guide etc.

facebook-github-bot

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ayc1

Thanks for working on this Krzysztof! This is amazing. I'll import and test this out

ayc1 · 2017-11-15T19:39:24Z

ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java

+
+      // We check if activity is attached to window by checking if decor view is attached
+      final View decorView = mCurrentActivity.getWindow().getDecorView();
+      if (decorView.getWindowToken() == null) {


Have you tried using https://developer.android.com/reference/android/support/v4/view/ViewCompat.html#isAttachedToWindow(android.view.View) ?

Ah, nice. Didn't know about ViewCompat. Will update that. Thanks for suggesting!

facebook-github-bot

@achen1 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

kmagiera · 2017-11-21T17:19:25Z

Hey @achen1 or @ayc1 aby update on this?

ayc1 · 2017-11-29T19:20:48Z

hey @kmagiera sorry I didn't have a chance to look at this lately. there has been a few changes to these files recently. do you mind rebasing and fixing the merge conflicts? after that, I'll pull it in to fix the internal build failures due to the removal of ReactInstanceDevCommandsHandler.

…instead of ReactInstanceDevCommandsHandler

…ecor view

…n to verify if view is attached

kmagiera · 2017-12-04T08:14:50Z

Hey @ayc1. Sorry for the delay. Just rebased this PR on top of the most recent changes from master and have run the test plan once again to make sure everything works as expected. Let me know if I can help with anything more here

facebook-github-bot

@achen1 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ayc1 · 2017-12-06T01:38:11Z

ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java

 import com.facebook.common.logging.FLog;
 import com.facebook.debug.holder.PrinterHolder;
 import com.facebook.debug.tags.ReactDebugOverlayTags;
 import com.facebook.infer.annotation.Assertions;
 import com.facebook.react.R;
+import com.facebook.react.ReactInstanceManager;


this creates a circular dependency

//java/com/facebook/catalyst/js/react-native-github/ReactAndroid/src/main/java/com/facebook/react:react -> //java/com/facebook/catalyst/js/react-native-github/ReactAndroid/src/main/java/com/facebook/react/devsupport:devsupport -> //java/com/facebook/catalyst/js/react-native-github/ReactAndroid/src/main/java/com/facebook/react:react

maybe you should leave the interface in react/devsupport?

kmagiera · 2017-12-06T07:16:43Z

Ah, ok, I’ll update that today

…etween main react package and devsupport package

kmagiera · 2017-12-07T09:48:27Z

@ayc1 I restored interface to workaround circular deps issue. I had to rename it though as with getCurrentActivity method it no longer sound reasonable to call it ReactInstanceDevCommandsHandler, the new name is ReactInstanceManagerDevHelper.

I also updated "Breaking" section in PR description to account for that change.

facebook-github-bot

@achen1 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ayc1 · 2017-12-15T23:21:13Z

@kmagiera this is causing the green dev loading view to appear behind the activity when you reload JS on old Android versions like API 17 and 19 but not on 23. Can you take a look at this regression?

kmagiera · 2017-12-21T10:37:12Z

Hey @ayc1, sure I will take a look

kmagiera · 2017-12-21T12:35:37Z

@ayc1 I created #17305 to fix this issue on Kitkat and below

Summary: This PR fixes the problem with dev loading view on KitKat and older Android devices after #16596 Install RNTester app on Android API 19 or lower device. See green loading view show up under status bar. Do the same with full screen theme set and see it show up correctly at the top of the screen. Verify the green loading bar displays correctly on devices with API > 19 too. This fixes an issue introduced in #16596 [ANDROID][MINOR][DevSupport] - Fix green dev loading bar on Android Kitkat and below Closes #17305 Differential Revision: D6621077 Pulled By: achen1 fbshipit-source-id: 3b4216af535d7db5c96d137f20004fe2651b1dc9

Summary: This PR fixes the problem with dev loading view on KitKat and older Android devices after facebook#16596 Install RNTester app on Android API 19 or lower device. See green loading view show up under status bar. Do the same with full screen theme set and see it show up correctly at the top of the screen. Verify the green loading bar displays correctly on devices with API > 19 too. This fixes an issue introduced in facebook#16596 [ANDROID][MINOR][DevSupport] - Fix green dev loading bar on Android Kitkat and below Closes facebook#17305 Differential Revision: D6621077 Pulled By: achen1 fbshipit-source-id: 3b4216af535d7db5c96d137f20004fe2651b1dc9

Summary:  * 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: #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: #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#1  [ANDROID] [FEATURE] [TV] - Added support for Android TV devices Closes #16500 Differential Revision: D6536847 Pulled By: hramos fbshipit-source-id: 17bbb11e8583b97f195ced5fd9762f8902fb8a3d

Summary:  * 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  [ANDROID] [FEATURE] [TV] - Added support for Android TV devices Closes facebook/react-native#16500 Differential Revision: D6536847 Pulled By: hramos fbshipit-source-id: 17bbb11e8583b97f195ced5fd9762f8902fb8a3d

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 30, 2017

kmagiera mentioned this pull request Oct 30, 2017

Add support for Android TV devices #16500

Closed

3 tasks

janicduplessis reviewed Oct 31, 2017

View reviewed changes

facebook-github-bot reviewed Nov 7, 2017

View reviewed changes

kmagiera force-pushed the overlayperm branch 2 times, most recently from 62367f6 to 60a19bf Compare November 15, 2017 15:12

ayc1 reviewed Nov 15, 2017

View reviewed changes

facebook-github-bot reviewed Nov 15, 2017

View reviewed changes

kmagiera added 5 commits December 4, 2017 08:16

Use current activity to display redbox, loading view and dev menu.

5df2c2a

Prevent perm dialog launch loop if user declines permission

1429402

Use ref to ReactInstanceManager directly in dev support related code …

d0b8e31

…instead of ReactInstanceDevCommandsHandler

Use PopupWindow instead of adding loading view directly to window's d…

9ee193a

…ecor view

Use ViewCompat.isAttachedToWindow instead of calling to getWindowToke…

d7fc98c

…n to verify if view is attached

kmagiera force-pushed the overlayperm branch from 51a76e1 to d7fc98c Compare December 4, 2017 08:12

facebook-github-bot reviewed Dec 6, 2017

View reviewed changes

ayc1 suggested changes Dec 6, 2017

View reviewed changes

Extract instanceManager methods to interface to avoid circular deps b…

4723beb

…etween main react package and devsupport package

facebook-github-bot reviewed Dec 7, 2017

View reviewed changes

facebook-github-bot closed this in d19afc7 Dec 8, 2017

kmagiera mentioned this pull request Dec 21, 2017

Add top offset for react loading view on Kitkat #17305

Closed

ide mentioned this pull request Dec 22, 2017

[0.52] Commits to cherry-pick into stable release #17315

Closed

friederbluemle mentioned this pull request Feb 3, 2021

Update integration-with-existing-apps docs facebook/react-native-website#2473

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ANDROID] [ENHANCEMENT] [DevSupport] Use currentActivity to display redbox, loading view and dev menu #16596

[ANDROID] [ENHANCEMENT] [DevSupport] Use currentActivity to display redbox, loading view and dev menu #16596

kmagiera commented Oct 30, 2017 •

edited

Loading

pull-bot commented Oct 30, 2017 •

edited

Loading

janicduplessis left a comment

janicduplessis Oct 31, 2017

kmagiera Nov 2, 2017

janicduplessis Nov 2, 2017

kmagiera Nov 3, 2017

kmagiera commented Nov 2, 2017

kmagiera commented Nov 3, 2017

facebook-github-bot left a comment

ayc1 left a comment

ayc1 Nov 15, 2017

kmagiera Nov 16, 2017

facebook-github-bot left a comment

kmagiera commented Nov 21, 2017

ayc1 commented Nov 29, 2017

kmagiera commented Dec 4, 2017

facebook-github-bot left a comment

ayc1 Dec 6, 2017

kmagiera commented Dec 6, 2017

kmagiera commented Dec 7, 2017 •

edited

Loading

facebook-github-bot left a comment

ayc1 commented Dec 15, 2017

kmagiera commented Dec 21, 2017

kmagiera commented Dec 21, 2017

[ANDROID] [ENHANCEMENT] [DevSupport] Use currentActivity to display redbox, loading view and dev menu #16596

[ANDROID] [ENHANCEMENT] [DevSupport] Use currentActivity to display redbox, loading view and dev menu #16596

Conversation

kmagiera commented Oct 30, 2017 • edited Loading

Motivation

Test Plan

Release Notes

Breaking

pull-bot commented Oct 30, 2017 • edited Loading

janicduplessis left a comment

Choose a reason for hiding this comment

janicduplessis Oct 31, 2017

Choose a reason for hiding this comment

kmagiera Nov 2, 2017

Choose a reason for hiding this comment

janicduplessis Nov 2, 2017

Choose a reason for hiding this comment

kmagiera Nov 3, 2017

Choose a reason for hiding this comment

kmagiera commented Nov 2, 2017

kmagiera commented Nov 3, 2017

facebook-github-bot left a comment

Choose a reason for hiding this comment

ayc1 left a comment

Choose a reason for hiding this comment

ayc1 Nov 15, 2017

Choose a reason for hiding this comment

kmagiera Nov 16, 2017

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

kmagiera commented Nov 21, 2017

ayc1 commented Nov 29, 2017

kmagiera commented Dec 4, 2017

facebook-github-bot left a comment

Choose a reason for hiding this comment

ayc1 Dec 6, 2017

Choose a reason for hiding this comment

kmagiera commented Dec 6, 2017

kmagiera commented Dec 7, 2017 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

ayc1 commented Dec 15, 2017

kmagiera commented Dec 21, 2017

kmagiera commented Dec 21, 2017

kmagiera commented Oct 30, 2017 •

edited

Loading

pull-bot commented Oct 30, 2017 •

edited

Loading

kmagiera commented Dec 7, 2017 •

edited

Loading