[V2] [iOS] Quick white screen (flash) after LaunchImage

Dexwell commented 6 years ago

Issue Description

Between the LaunchImage and display of the first RNN screen, there is a small window where the screen is white. In Debug mode it takes longer; this is where you see the green "Downloading JavaScript bundle" bar up top. The statusbar also goes dark here even though I set it to light using UIStatusBarStyle in Info.plist.

Existing solutions exist that require editing AppDelegate.m code related to the rootView. However when using RNN this is impossible, since we have to replace everything with a custom bootstrap method:

[ReactNativeNavigation bootstrap:jsCodeLocation launchOptions:launchOptions];

Is there any way you guys on the native side could make it so that the launch image and UIStatusBarStyle are not changed until the first RNN screen has loaded? This is some critical functionality impacting the UX of every app using RNN.

Steps to Reproduce / Code Snippets / Screenshots

n/a

Environment

React Native Navigation version: 2.0.2435
React Native version: 0.55.4
Platform(s) (iOS, Android, or both?): iOS
Device info (Simulator/Device? OS version? Debug/Release?): All

tiempham commented 6 years ago

I used layout: { backgroundColor: 'transparent', orientation: ['portrait', 'landscape'] } it's working.

Dexwell commented 6 years ago

@tiempham Not for me, which makes sense because the white screen means RN is loading the JavaScript. I think you misunderstand my issue.

@guyca @yogevbd Could you assign/look at this? We already have 4 +1s :)

chussum commented 6 years ago

v1 also have a same issue :(

danielgindi commented 6 years ago

Happens to me too. While it is loading the JS/React - it shows a white screen. If react-native-navigation would supply an intermediate splash screen that takes the Default image on iOS and then fades into the real screen when it's rendered - that will solve the issue.

Dexwell commented 6 years ago

Is this just not going to be looked at @yogevbd @guyca? Again, pretty big UX deal.

Dexwell commented 6 years ago

Those following this issue, perhaps duplicate this issue and refer it to get the devs' attention. I'd hate for this issue to block me from launching my app. @guyca @yogevbd

rduhard commented 6 years ago

The solution to this problem is to set the loadingView property of the RCTRootView. If you have a Launch image in your app, you'd want to pass in the launch screen view to be set as the RCTRootView.loadingView property so that RN will use that screen while it loads the JS. This removes the flash and looks like a seamless transition. The RNN library needs to support the ability to pass in a loadingView that can be set to the RCTRootView.loadingView when it's created and from there it is handled entirely by RN.

Dexwell commented 6 years ago

@rduhard So if I understand correctly this can't be solved until the RNN library adds support for a loadingView, correct?

rduhard commented 6 years ago

@Dexwell yes I believe so. We have a fork of V1 that we implemented this in currently but it would be ideal if they could add this necessary functionality into V2. They need to add a way for users of RNN to pass in a loadingView that would RNN would then subsequently set on the RCTRootView.loadingView property when it is created. In addition, they would need to set the frame of the loadingView to match that of the RCTRootView prior to setting it. This is how we implemented it in our fork.

andriofrizon commented 6 years ago

@rduhard Thanks for pointing out this solution. As far as I've understood this would solve the problem only on iOS right? How about android? It would be great to check your solution, could you share your fork?

I'm facing this problem both on iOS and Android atm.

rduhard commented 6 years ago

@Dexwell Another thing that needs to be considered is that the loadingView will be used on all RCTRootViews created so since RNN creates a new one for each component, there would need to be a way to differentiate the initial launch root view from any others you'd want a different loadingView or no view at all on. The solution for V2 will be quite different, looking at how they've changed the implementation of the controllers. The gist of the solution is we added a loadingView property to the RCCManager that we could set, and then in the RCCVIewControlller.commonInit function we set the loadingView on the RCTRootView but only if it's the initial screen that is being created since we did not need a loadingView within the app itself. This is for iOS only, I have not looked into the Android code at all.

Dexwell commented 6 years ago

It's pretty bizarre how the RNN devs seem unwilling to even acknowledge this issue. I'm afraid we'll have to hope someone experienced with native code comes in clutch and creates a PR that fixes this.

LRNZ09 commented 6 years ago

If you don't use an image, you can try this.

VicFrolov commented 6 years ago

+1

nikolaigeorgie commented 6 years ago

+1

ENT108 commented 6 years ago

+1

Dexwell commented 6 years ago

🤕@yogevbd @guyca

franciscomdelgado commented 6 years ago

+1

VicFrolov commented 6 years ago

Hopefully this issue gains some traction as the +1's are growing.

ENT108 commented 6 years ago

I fixed it with react-native-splash-screen.

Dexwell commented 6 years ago

Acknowledged! Thanks devs 🙌🏼

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe the issue is still relevant, please test on the latest Detox and report back. Thank you for your contributions.

Dexwell commented 5 years ago

Still relevant...

Garmega commented 5 years ago

+1. this issue is driving me insane

EDIT: I fixed it with react-native-splash-screen as well. this should really be built into rnn

evaldsurtans commented 5 years ago

+1

brianramirez commented 5 years ago

+1 This is a large UX issue and should be implemented to RNN.

henrikra commented 5 years ago

Does anyone have some idea where the code is that needs to be changed? I can look into that because I am facing the same problem. I would just want to be shown to right direction :)

danielgindi commented 5 years ago

Okay so I've analyzed the problem and it's problematic specifically on iOS. On Android, the Activity is controlled by the App, and it is only first displayed when RNN knows that it's ready. On iOS the system shows the UIWindow immediately after the splash (and loading in App's delegate code). You can only delay this with synchronous code, but RN/RNN loading is asynchronous, and it must have the main thread free to prepare UIKit stuff, so we can't block it.

So I started to write a solution for this - which is basically a view controller that parses the app's plist and fetches the correct launch screen storyboard/xib/image. After I'm almost done with that, I find out that there's a file named RNNSplashImage.m file in RNN's code, which does exactly that.

Now I'm investigating why the blanks still how if we have a splash screen automatically set up.

danielgindi commented 5 years ago

An update: It does use the splash-mock, and shows it up nicely. In fact most of the (fraction of a) time that we see the splash - it's actually that RNNSplashImage.

BUT when the app calls setRoot(... it goes straight to setRoot in native code, which replaces the UIWindow's root view controller with the new root view controller, immediately. It does not wait for its first render. Now we just have to dive into RN's internals and find out how to get notified on first render.

If anyone has experience in that area I could use a hand there.

danielgindi commented 5 years ago

Solved it! Will create a PR and let you know. Let's just hope it gets merged.

danielgindi commented 5 years ago

Update: Do not point your npm to the git branch. It will not work as it will be missing a pre-publish build step.

You can copy the podspec to your local project, and point that to the branch. Point your npm to 2.4.0 as that is the version that it is based on.

I will do my best to keep it in sync with master until it's merged in. Now we need a lot of votes on that PR (and testing your apps in production).

Dexwell commented 5 years ago

@danielgindi Thanks so much for working on this!

When I point my npm at your branch, my app launches with this error though:

However, this package itself specifies a main module field that could not be resolved (/Users/(...)/node_modules/react-native-navigation/lib/dist/index.js.

The dist folder doesn't exist?

danielgindi commented 5 years ago

Yes because there is a build stage before publishing to npm. I point my npm now to the official 2.4.0, and modify the podspec to point to thr branch in a postinstall script. The changes are only in the native code anyway.

Dexwell commented 5 years ago

@danielgindi Can you point me in the right direction as to how to set up the Podspec file?

danielgindi commented 5 years ago

Take the current Podspec, in the git url replace ”wix” with ”danielgindi”, and replace tag = ... with branch = “bugfix/ios_white_launch_screen”

danielgindi commented 5 years ago

In my project I have a postinstall script that is doing such modifications in the code/specs of several react packages. I think anyone with a project big enough ends up with such a mechanism, so this is just another “fix” you can do until it is merged.

Dexwell commented 5 years ago

That didn't work for me, but I ended up pulling in your code by setting up my Podfile like this

pod 'ReactNativeNavigation', git: 'https://github.com/danielgindi/react-native-navigation.git', branch: 'bugfix/ios_white_launch_screen'

The white flash persists because I setRoot after loading the initial root, looks like I have to change my approach there 😬

However, the status bar still looks buggy; it's light during launch, then changes to dark where the screen used to flash white, then becomes light again. Is it possible to update your PR so that it honors the chosen status bar style all the way through?

danielgindi commented 5 years ago

@Dexwell when compiling as an adhoc/testflight for the device - does the white screen still persist? I'm asking because I allowed it in the code to immediately replace the root for subsequent setRoot calls - assuming that everything's loaded. It's possible that I'll have to enable the "delay until render" for those too - but in order to be sure I need to know if it happens in releases too, not in React debug mode.

As for the status bar - I think I'll have to update RNNSplashImage to fetch the initial status bar color from the app's plist and return that in the ViewController's methods. Again - not sure about this, as I don't know where the colors are taken from in your app.

Let's try to clarify it! The stages are:

Real launch image, loaded by iOS
Fake launch image screen, loaded by RNN
Still fake launch image screen, react is loading (In debug mode you see the green bar at the top)
Still fake launch image screen, react has attached its view into RNN's generated view controller (green bar may say 100%)
RNN receives a slot on RN's UI thread and attached the new root instead of RNNSplashImage.

Now can you say what the status bar's color is at each stage? And what the source of it is (to your best guess)?

One last question: On Android, when use re-setRoot, what's the effect of it? Any animation? White screen? Because I can't see any animation structure for setRoot as opposed to push/pop/overlay...

Dexwell commented 5 years ago

@danielgindi Haven't set up TestFlight for it yet, but when launching on my device using the Release scheme it still flashes.

Let's see:

White. I have no Launch Screen File, so source is UIStatusBarStyleLightContent in Info.plist
Animates to black here.
Green bar overlaps status bar, so hard to say. Best guess still black.
Same as 3?

White, source probably the component's:

static get options() {
return {
statusBar: {
  style: 'light'
}
};
}

Re Android; it's an iOS only project for now.

danielgindi commented 5 years ago

Thanks! Then I'm going forward with enabling the fix for all setRoots. Adding status bar methods to RNNSplashImage also.

Dexwell commented 5 years ago

@danielgindi Latest commits fixed all issues; looks so smooth now. You're a legend! 🙌🏼🎉

danielgindi commented 5 years ago

@Dexwell The only blocker now for merging it is the tests. The e2e tests are failing due to using a library called detox which is supposed to wait for react to be ready before it expects UI elements to be on the screen, but it resolves that before react actually performs the layout stage, and then the tests fail. We need someone who has experience with detox to fix the tests...

guyca commented 5 years ago

@yogevbd waitForRender (#4369) should fix this issue, right?

danielgindi commented 5 years ago

@guyca Unless you are going to wait for the layout cycle - it's not going to solve it. Because the current implementation of "wait for render" on iOS - means waiting for React's DidAppear notification, which happens when the React Native view was added to the parent view. As the layout cycle is async and not happening immediately, you will still get a flicker for a fraction of a second.

I've debugged this a lot, and I don't really see a way around it. I have an implementation that's ready and tested in both debug & dist environments. There's just some weird incompatibility with `detox. It could be used together with @yogevbd's work, but it must be there in order to get rid of the white screen.

The e2e tests are currently passing 100% "on my machine" - but not on your CI machine. It's like it's ignoring the waitFor request.

תנו לזה קצת תשומת לב אה? שרפתי על זה כבר כמה שעות טובות.

danielgindi commented 5 years ago

@guyca The implementation you were referring to is both irrelevant, and Android only (while the current issue is with iOS. In Android it actually worked...) How can we make this get more attention?

guyca commented 5 years ago

@danielgindi This issue has already been prioritised internally and work has already started on it - unfortunately something came up and @yogevbd had to context switch to other tasks. I'm sure he'll get to this by next week. We also need this feature for the Wix app so you can be sure we'll get to this.

danielgindi commented 5 years ago

@guyca Great to hear that, thank you. I thought about a way to resolve the testing issue, or more of a workaround. Since detox does not know how to wait for real "ready" mode, We can (and should, anyway) make the new waiting mechanism depend on waitForRender flag of setRoot. It's not implemented in iOS anyway. And the current tests will stay as is - with waitForRender turned off. A separate test for waitForRender on setRoot will have to be created, to make sure that the UI is not attached before being really ready - actually expecting the errors that come up now.

The only gap is implementing the waitForRender configuration on the iOS side, and toggling the waiting mechanism according to that.

guyca commented 5 years ago

We intent waitForRender to be opt-in - in this case why should there be issues with detox?

danielgindi commented 5 years ago

@guyca That's what I'm saying

danielgindi commented 5 years ago

Note that the branch is now rebased on 2.6.0, and that it is now an opt-in feature, by adding waitForRender: true in the options for you root component.

An example:

Navigation.setRoot({
  root: { 
    stack: {
      children: [
        {
          component: {
            name: 'MyRootComponent'
            options:
              animations: {
                setRoot: {
                  waitForRender: true
                }
              }
            }
          }
        }
      ]
    }
  }
});

@Dexwell @LRNZ09 @henrikra

wix / react-native-navigation