Android - Fix Overlay for Marshmallow 23+ #11316

jpshelley · 2016-12-06T05:29:33Z

Overview

Currently any React Native apps that target API 23 or greater will crash on the first initial debug/dev build due to the overlay permission.

Sadly there isn't a concrete "request permission" baked into the Marshmallow permission system.
However, we can launch the overlay screen without starting the react app and once its turned on start the app.

Addresses / Changes

targetSdkVersion 23 lead crash #10454 - targetSdkVersion 23 lead crash / App crash for targeting 23+
Add the overlay permission information #10479 - Add the overlay permission information / Larger discussion around targeting API 23+
Intent to Overlay permission goes directly to the app in question, rather then the general full listing of applications. This allows a developer who is not familiar with the system to easily toggle the overlay without getting confused.

Test plan (required)

Ran UIExplorer App on fresh install with Target 23

cd react-native
./gradlew :Examples:UIExplorer:android:app:installDebug
./packager/packager.sh

Notice the app doesn't crash as it use to
Turn on the overlay toggle for the app
Push the back button (this navigates back to UIExplorerApp)
Watch the UIExplorerApp fetch the JS Bundle!

Make sure tests pass on both Travis and Circle CI.

facebook-github-bot · 2016-12-06T05:29:37Z

By analyzing the blame information on this pull request, we identified @foghina and @cmcewen to be potential reviewers.

satya164 · 2016-12-06T09:46:39Z

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

@@ -29,6 +30,10 @@
 * class doesn't implement {@link ReactApplication}.
 */
 public class ReactActivityDelegate {
+
+  private final int REQUEST_OVERLAY_CODE = 1111;


nit: maybe a more random number like 3798264?

And probably change the name to REQUEST_OVERLAY_PERMISSION_CODE?

I can surely change the value, but I don't think there is any benefit of it being random. It doesn't need to be unique to other application just your own. So if your actvitiy ends up having multiple request codes its best to have them well defined, in order, and organized.
ie:

private final int MY_FIRST_CODE = 1111; private final int MY_SECOND_CODE = 2222;

Also it doesn't need to be 4 digits in length, just a common pattern I've noticed in the community.

I'll update the name though, seems reasonable.

It doesn't need to be unique to other application just your own

It can conflict with third party modules / and even own code. Since this code is part of the RN library, not my application.

We don't follow this pattern anywhere else inside RN either. So I'd prefer a more random number.

@satya164 The number must be between 0 and 65535 I believe
http://stackoverflow.com/a/37812367/1784299

I don't think Random is the correct approach because then it is "random" for a developer when that clashes. It should be predefined so a developer knows early on why they can't use 1 for example as their request code.

I don't think its that big of a deal to be honest but want to get it "right" the first time.

For instance, google uses: https://github.com/google/iosched/blob/e8c61e7e23f74aa6786696dad22e5136b423a334/android/src/main/java/com/google/samples/apps/iosched/ui/BaseActivity.java#L81

because then it is "random" for a developer when that clashes

The developer is not going look into the library to see what code is used. This code is not part of the app. We can never prevent clash, but this number is too easy to clash with third party lib/user's code.

@SandroMachado that example is a sample app, I don't think libraries can follow the same convention as an app anyways.

RN code in general doesn't follow this convention. So I don't see what advantage it gives by following at a single place, unless all other code is changed to follow the convention.

Anyway, I don't think this is important enough to bikeshed on. Please do the changes @SandroMachado mentioned and I'll merge this.

@satya164 Okay two thoughts:

We can use a random number as you suggest.

I'm okay with this, but we always have the potential of clashes then.

A can change the clash in their own app if they caused the clash

However if the clash came from another library/dependency then they would have to change or this project would have to change.

Or we can not hardcode it and allow the user to pass in whatever RequestCodethey want.

This could be through the Delegate constructor.

That way they always have control over it and can change it if needed so as not to clash.

We would just add an assertion after the number is passed in to make sure its a valid number

This isn't very common in the Android Community but from a Developer perspective I don't see why it wouldn't work.

satya164 · 2016-12-06T09:50:44Z

LGTM. Just one nit and I'll merge

SandroMachado · 2016-12-06T11:38:40Z

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

-        Intent serviceIntent = new Intent(Settings.ACTION_MANAGE_OVERLAY_PERMISSION);
-        getContext().startActivity(serviceIntent);
+        needsOverlayPermission = true;
+        Intent serviceIntent = new Intent(Settings.ACTION_MANAGE_OVERLAY_PERMISSION, Uri.parse("package:" + getContext().getPackageName()));
        FLog.w(ReactConstants.TAG, REDBOX_PERMISSION_MESSAGE);
        Toast.makeText(getContext(), REDBOX_PERMISSION_MESSAGE, Toast.LENGTH_LONG).show();


Probably we can remove this now, WDYT?

I think its still worth showing to the developer though so they understand what they need to do. Especially since we distribute to quality assurance it would help them understand the situation.

Yup, makes sense! 👍 Please ignore.

SandroMachado · 2016-12-06T11:40:02Z

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

@@ -79,17 +84,19 @@ public ReactInstanceManager getReactInstanceManager() {
  }

  protected void onCreate(Bundle savedInstanceState) {
+    boolean needsOverlayPermission = false;
    if (getReactNativeHost().getUseDeveloperSupport() && Build.VERSION.SDK_INT >= 23) {


... >= Build.VERSION_CODES.M?

SandroMachado · 2016-12-06T11:43:39Z

Also don't forget to update the disclaimer from here: https://github.com/facebook/react-native/blob/v0.39.0/docs/IntegrationWithExistingApps.md

If your app is targeting the Android api level 23 or greater, make sure you have, for the development build, the overlay permission enabled. You can check it with Settings.canDrawOverlays(this);. This is required because, if your app produces an error in the react native component, the error view is displayed above all the other windows. Due to the new permissions system, introduced in the api level 23, the user needs to approve it.

And probably you can squash the two commits. WDYT?

Great work! 👍

jpshelley · 2016-12-06T19:06:47Z

How should we update the disclaimer @SandroMachado? I think its wise to keep "a disclaimer" but maybe change the words? Any thoughts on what would be best to say?

satya164 · 2016-12-06T19:22:57Z

I believe this doesn't affect "integrate with existing apps", and only affects apps using ReactActivity. In that case we shouldn't change the documentation.

SandroMachado · 2016-12-06T19:31:17Z

@satya164 You are right, for integrating is not need to change because probably they will not create a ReactActivity. My bad.

satya164 · 2016-12-06T19:38:17Z

@facebook-github-bot shipit

jpshelley · 2016-12-06T22:22:47Z

@satya164 not sure if the bot is supposed to take a long time, but Github did have a service degradation earlier that may have caused a hiccup in the command.

satya164 · 2016-12-06T23:02:49Z

@facebook-github-bot shipit

mkonicek · 2016-12-07T14:19:18Z

How is this permission different from the SYSTEM_ALERT_WINDOW?
https://github.com/facebook/react-native/blob/master/local-cli/templates/HelloWorld/android/app/src/main/AndroidManifest.xml#L7

jpshelley · 2016-12-07T15:49:39Z

@mkonicek As mentioned in the PR description and linked issues, that permission died in API 23 with the Marshmallow permission update. This is to benefit those that target API 23+

satya164 · 2016-12-07T15:59:53Z

@facebook-github-bot shipit

Summary: Currently any React Native apps that target API 23 or greater will crash on the first initial debug/dev build due to the overlay permission. Sadly there isn't a concrete "request permission" baked into the Marshmallow permission system. However, we can launch the overlay screen without starting the react app and once its turned on start the app. - facebook#10454 - targetSdkVersion 23 lead crash / App crash for targeting 23+ - facebook#10479 - Add the overlay permission information / Larger discussion around targeting API 23+ - Intent to Overlay permission goes directly to the app in question, rather then the general full listing of applications. This allows a developer who is not familiar with the system to easily toggle the overlay without getting confused. **Test plan (required)** * Ran UIExplorer App on fresh install with Target 23 ``` cd react-native ./gradlew :Examples:UI Closes facebook#11316 Differential Revision: D4286351 fbshipit-source-id: 024e97c08c40ee23646dd153794fcde7127b2308

jpshelley · 2016-12-08T23:15:11Z

@satya164 Not sure if the PR is supposed to be closed but I think the commit made it to master if anything needs to happen on this PR d21aa92

Summary: Currently any React Native apps that target API 23 or greater will crash on the first initial debug/dev build due to the overlay permission. Sadly there isn't a concrete "request permission" baked into the Marshmallow permission system. However, we can launch the overlay screen without starting the react app and once its turned on start the app. - #10454 - targetSdkVersion 23 lead crash / App crash for targeting 23+ - #10479 - Add the overlay permission information / Larger discussion around targeting API 23+ - Intent to Overlay permission goes directly to the app in question, rather then the general full listing of applications. This allows a developer who is not familiar with the system to easily toggle the overlay without getting confused. **Test plan (required)** * Ran UIExplorer App on fresh install with Target 23 ``` cd react-native ./gradlew :Examples:UI Closes #11316 Differential Revision: D4286351 fbshipit-source-id: 024e97c08c40ee23646dd153794fcde7127b2308

Summary: Currently any React Native apps that target API 23 or greater will crash on the first initial debug/dev build due to the overlay permission. Sadly there isn't a concrete "request permission" baked into the Marshmallow permission system. However, we can launch the overlay screen without starting the react app and once its turned on start the app. - facebook#10454 - targetSdkVersion 23 lead crash / App crash for targeting 23+ - facebook#10479 - Add the overlay permission information / Larger discussion around targeting API 23+ - Intent to Overlay permission goes directly to the app in question, rather then the general full listing of applications. This allows a developer who is not familiar with the system to easily toggle the overlay without getting confused. **Test plan (required)** * Ran UIExplorer App on fresh install with Target 23 ``` cd react-native ./gradlew :Examples:UI Closes facebook#11316 Differential Revision: D4286351 fbshipit-source-id: 024e97c08c40ee23646dd153794fcde7127b2308

jpshelley added 2 commits December 5, 2016 23:03

Use activity for result to stop crashes

102f5a2

fix weird auto-formatting quirks

bd75397

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Dec 6, 2016

jpshelley mentioned this pull request Dec 6, 2016

Add the overlay permission information #10479

Closed

satya164 reviewed Dec 6, 2016

View reviewed changes

SandroMachado reviewed Dec 6, 2016

View reviewed changes

satya164 added GH Review: needs-revision and removed GH Review: review-needed labels Dec 6, 2016

update comments

20c67e7

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Dec 6, 2016

satya164 closed this Dec 8, 2016

jpshelley deleted the android-FixOverlayForMarshmallow branch December 16, 2016 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android - Fix Overlay for Marshmallow 23+ #11316

Android - Fix Overlay for Marshmallow 23+ #11316

jpshelley commented Dec 6, 2016 •

edited

Loading

facebook-github-bot commented Dec 6, 2016

satya164 Dec 6, 2016

SandroMachado Dec 6, 2016

jpshelley Dec 6, 2016

satya164 Dec 6, 2016 •

edited

Loading

jpshelley Dec 6, 2016

SandroMachado Dec 6, 2016

satya164 Dec 6, 2016 •

edited

Loading

satya164 Dec 6, 2016

jpshelley Dec 6, 2016

satya164 commented Dec 6, 2016

SandroMachado Dec 6, 2016

jpshelley Dec 6, 2016

SandroMachado Dec 6, 2016

SandroMachado Dec 6, 2016

SandroMachado commented Dec 6, 2016 •

edited

Loading

jpshelley commented Dec 6, 2016

satya164 commented Dec 6, 2016

SandroMachado commented Dec 6, 2016

satya164 commented Dec 6, 2016

jpshelley commented Dec 6, 2016

satya164 commented Dec 6, 2016

mkonicek commented Dec 7, 2016

jpshelley commented Dec 7, 2016

satya164 commented Dec 7, 2016

jpshelley commented Dec 8, 2016

Android - Fix Overlay for Marshmallow 23+ #11316

Android - Fix Overlay for Marshmallow 23+ #11316

Conversation

jpshelley commented Dec 6, 2016 • edited Loading

Overview

Addresses / Changes

facebook-github-bot commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satya164 Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satya164 Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satya164 commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandroMachado commented Dec 6, 2016 • edited Loading

jpshelley commented Dec 6, 2016

satya164 commented Dec 6, 2016

SandroMachado commented Dec 6, 2016

satya164 commented Dec 6, 2016

jpshelley commented Dec 6, 2016

satya164 commented Dec 6, 2016

mkonicek commented Dec 7, 2016

jpshelley commented Dec 7, 2016

satya164 commented Dec 7, 2016

jpshelley commented Dec 8, 2016

jpshelley commented Dec 6, 2016 •

edited

Loading

satya164 Dec 6, 2016 •

edited

Loading

satya164 Dec 6, 2016 •

edited

Loading

SandroMachado commented Dec 6, 2016 •

edited

Loading