react-native-safe-area-context icon indicating copy to clipboard operation
react-native-safe-area-context copied to clipboard

Improvement suggestion - First render should always include safe area insets

Open iway1 opened this issue 3 years ago • 41 comments

I've had a lot of problems using this library in situations where there are components using the safe area view or components rendering at the same time as safe area view have to rerender quickly. Not sure if this has to do with the fact I'm using react-navigation.

The SafeAreaView just does not apply insets on the initial render sometimes. I run into issues pretty frequently where my layout flickers because the children of the safe area view component are first laid out without the safe area insets, and then they flicker to the correct position where they should have been on first render.

In multiple places in my application, I've had to outright remove <SafeAreaView edges={['top']}> because the layout flickers and replace it with useSafeAreaInsets. That works fine a lot of the time, but there are situations where it's not an option (Such as in a TabBar component used with React Navigation). And in those cases, I'm left to try hacky things.

For example, I have a new user profile screen that queries the profile data. On the initial render, there is a loading indicator rather than the profile itself. When the profile is fetched it is displayed. In the demo version of the app, the profile fetching happens immediately, and therefor the screen rerenders immediately. I have a bottom TabBar that uses a <SafeAreaView edges={['bottom']}>. On the initial render, the tab bar doesn't have any insets at all!

This only seems to happen when the component is immediately rerendering in this case (but in other cases it just happens for no reason). If I make the fetch user profile function sleep momentarily with a timeout with 0 ms, the flickering doesn't happen. It's a hack to deal with the fact that this library allows renders without insets.

I just feel like the safe area view should never allow for a situation where it can render without the insets. If it were able to always render with insets, layout flickering would be impossible.

I'm really not sure how the insets aren't getting applied on first render, but they aren't. It seems like they should be based on the code for SafeAreaView.

iway1 avatar Dec 13 '21 21:12 iway1

Could you give a bit more information about your setup?

SafeAreaContext takes an initialSafeAreaInsets prop, which you can set to initialWindowMetrics.insets (which is the insets when the app launched).

I did notice a few things that might be interfering with some setups, but would be good to hear what your setup is so we can do the right fix.

jacobp100 avatar Dec 17 '21 09:12 jacobp100

@iway1 is this fixed for you with https://github.com/th3rdwave/react-native-safe-area-context/pull/229 ?

danilobuerger avatar Dec 22 '21 08:12 danilobuerger

@jacobp100 @janicduplessis Another thing I noticed which might be related to this: Whenever adding a SafeAreaView on iOS the children will get the same size set as the SafeAreaView on the first batch layout as its not attached to the window hierarchy yet and doesn't have access to the proper safeAreaInsets. As soon as didMoveToWindow happens, the shadow view gets the safe area insets and then on the second layout the children will get the inseted (proper) size. Sometimes it seems (only on device) that between the first and second layout the whole thing gets rendered thus appearing on screen without any insets. A potential fix (see below) might be setting the display = YGDisplayNone in the shadow view until it receives its first safe area insets. However, I am unsure if that makes sense, so I wanted your opinion before opening a PR with it. With the diff below the children of SafeAreaView will not get their frame set until it has received the first insets.

diff --git a/node_modules/react-native-safe-area-context/ios/SafeAreaView/RNCSafeAreaShadowView.m b/node_modules/react-native-safe-area-context/ios/SafeAreaView/RNCSafeAreaShadowView.m
index c4afdae..056879a 100644
--- a/node_modules/react-native-safe-area-context/ios/SafeAreaView/RNCSafeAreaShadowView.m
+++ b/node_modules/react-native-safe-area-context/ios/SafeAreaView/RNCSafeAreaShadowView.m
@@ -35,6 +35,7 @@ - (instancetype)init
       _paddingMetaProps[ii] = YGValueUndefined;
       _marginMetaProps[ii] = YGValueUndefined;
     }
+    self.display = YGDisplayNone;
   }
   return self;
 }
@@ -95,6 +96,8 @@ - (void)updateInsets
     return;
   }
 
+  self.display = YGDisplayFlex;
+
   UIEdgeInsets insets = _localData.insets;
   RNCSafeAreaViewMode mode = _localData.mode;
   RNCSafeAreaViewEdges edges = _localData.edges;

danilobuerger avatar Dec 22 '21 08:12 danilobuerger

I wanted to rework getting the sage area insets directly from the provider rather than relying on UIKit to propagate it to subviews (see https://github.com/th3rdwave/react-native-safe-area-context/issues/92). I think the fix for that would also fix the issue you just mentioned

jacobp100 avatar Dec 22 '21 10:12 jacobp100

I'm seeing similar behavior. I think most apps have a single SafeAreaView for the entire application so this effect is hard to notice, but this one I'm currently working on needs a bit more of a customized approach and I'm noticing a 1-frame hitch in which the safe area insets are not applied any time a new SafeAreaView is mounted.

jameswilddev avatar Dec 24 '21 14:12 jameswilddev

We recently migrated from react-native-safe-area-view and the solution there was to use forceInset but I found is not possible on react-native-safe-area-context and using edges is usually ignored while transitioning to a new screen

Also related: https://github.com/th3rdwave/react-native-safe-area-context/issues/219

neiker avatar Feb 04 '22 11:02 neiker

This is pretty unpleasant bug which makes nearly impossible to use SafeAreaView, which is suggested by this library as a main way to use this library. Personally I see this flicker every time I force reload the app.

egorkhmelev avatar Mar 15 '22 13:03 egorkhmelev

We need more information about what your setup is - really with a minimal repro - to fix the problem if it's a bug, or tell you where you're going wrong if it's not

jacobp100 avatar Mar 15 '22 13:03 jacobp100

@jacobp100 thank you for you comment. Simple repo here: https://github.com/egorkhmelev/safe-area-bug

What I've noticed, the more complex screen structure the easier to reproduce the issue, with simple structure its almost unnoticeable, however if you record simulator screen and reload multiple times, then analyse video file frame by frame it can be spotted even though it was unnoticeable by the eye. I have easier times to spot it on real device versus simulator with the same project (sample and own). I've tried two different structures: simple and more complex (as per example for native stack in lib repo), reproducible on both.

Even though useSafeAreaInset() hook doesn't lead to the same issue on the test project, it does on real project with complex structure even if all screens is wrapped with withProvider helper (wrapped in SafeAreaProvider)

Below are two sets of two frames, screenshot was made from recorded video file.

SafeAreaView for both top & bottom view:

Screenshot 2022-03-17 at 16 40 01Screenshot 2022-03-17 at 16 40 06

useSafeAreaInsets() for top view and SafeAreaView for bottom view:

Screenshot 2022-03-17 at 16 43 33Screenshot 2022-03-17 at 16 43 37

Would be great to hear your thoughts what is wrong in setup/configuration of the project and/or lib and how this issue can be fixed. Thank you!

egorkhmelev avatar Mar 17 '22 17:03 egorkhmelev

What happens if you set follow the instructions here for this

jacobp100 avatar Mar 17 '22 17:03 jacobp100

@jacobp100 Change added and committed. Bug is still reproducible both on test project and own project for SafeAreaView case. Cannot reproduce for test & own project with useSafeAreaInset() hook instead of SafeAreaView with initialMetrics configured.

egorkhmelev avatar Mar 17 '22 18:03 egorkhmelev

Further test showed that the only configuration which avoids this issue with react-navigation and native stack is to use useSafeAreaInset() hook instead of SafeAreaView with initialWindowMetrics for root SafeAreaProvider and also with withProvider wrapper (without initial metrics) for each screen.

Maybe it will be useful for other folks here as a workaround for now.

egorkhmelev avatar Mar 17 '22 19:03 egorkhmelev

The hook has a slightly delayed response to changes compared to SafeAreaView. SAV is done purely natively, so we skip the bridge. But if something else has a delay, sometimes it’s good to match that so there’s less flicker.

jacobp100 avatar Mar 17 '22 19:03 jacobp100

@jacobp100 not sure I understand what you mean exactly. I do understand the difference of SAV vs hook (and bridge) and that's why I am really curious to make SAV works correctly without flicker in our app. Is there anything we can do to make it work correctly? Or this needs to be fixed in the lib?

egorkhmelev avatar Mar 18 '22 09:03 egorkhmelev

To be honest, you should only use the hook when you absolutely have to - when it updates, it lags behind SAV. I think the problem for you is your navigation library is using it

jacobp100 avatar Mar 18 '22 09:03 jacobp100

It doesn't make any sense because hooks works without issue actually (as I mentioned above) for the first render and SAV is flickering, but it should be other way around per your response and lib docs?

I've updated repo to remove completely navigation and hooks, so the only thing is used for insets is SAV and bug is still reproducible and can be noticed with eyes. Doesn't matter if I use initialMetrics or not.

egorkhmelev avatar Mar 18 '22 14:03 egorkhmelev

@egorkhmelev did you manage to find any other solution then using the useSafeAreaInsets hook? 😁 I am currently doing this as a temporary workaround since the flickering represents a bad UX imo.

I have all of the latest versions but the problem is still there 😢

jivanovic avatar Jun 12 '22 12:06 jivanovic

@jivanovic I was having the same problem using version 4.2.4, I updated the lib to version 4.3.1 and noticed that the problem had been resolved.

Pedroor avatar Jun 15 '22 21:06 Pedroor

We have the same issue, first render of SafeAreaView doesn't have the right paddings until next render. Now we have one screen that is slow loading its content, and we can see the flickering, at least in iOS. We tried with version 4.3.1 and the flickering is still present. Using a custom View with paddings set from useSafeAreaInsets values, we have the correct spacing since first render and there is no more flickering.

tcank avatar Jun 21 '22 20:06 tcank

Same here with 4.3.3 (video is at 0.25x speed)

https://user-images.githubusercontent.com/717975/187733798-48cec2a1-2d06-43fb-aa14-f8a64962a14c.mov

efstathiosntonas avatar Aug 31 '22 16:08 efstathiosntonas

The problem here doesn't have anything to do with initial values, or device rotation, or the insets changing. It's about mounting a new instance of <SafeAreaView />. The new instance will render with values of 0 and then immediately re-render with the correct values. And to of course complicate things, it does this sometimes, not always.

For example, if you make your own <NavBar /> which cares about the safe area, and each of your screens renders its own instance of <NavBar />, every screen change will mount a new instance of <NavBar /> and thus <SafeAreaView /> as well. So just switching to a new screen will cause this jarring flash; no rotation or initial app launch or anything like that involved.

My solution to this (because I'm using classical components and not functional) for now is to store the inset values in redux, and have my components set padding according to those values. This does lose the benefit of the native change that <SafeAreaView /> can do upon rotation, but the jarring flash is a much worse problem.

// render this in the Root component
class InsetsMonitor extends React.PureComponent {
  constructor(props) {
    super(props);
    this.onInsets = this.onInsets.bind(this);
  }
  onInsets(insets) {
    setTimeout(() => {
      this.props.dispatch(MyActionCreators.setInsets(insets))
    }, 0);
  }
  render() {
    return (
      <SafeAreaInsetsContext.Consumer>
        {this.onInsets}
      </SafeAreaInsetsContext.Consumer>
    );
  }
}
export default connect()(InsetsMonitor);

It'd be really nice to have a simple callback for retrieving the values in plain JavaScript. The only way to get access to them that I found was by rendering a component, which forces you to produce a side effect during render (thus the setTimeout(,0)).

I stumbled on this from this StackOverflow post. Despite the OP being about something slightly different, some of the folks stumbling on that thread had this problem and linked back to this lib.

This should be labeled as bug.

I'm using v4.4.1, and not using any navigation libs.

tybro0103 avatar Dec 16 '22 17:12 tybro0103

Steps to reproduce:

npx create-expo-app rnsac-bug-repro
cd rnsac-bug-repro
npx expo install react-native-safe-area-context

Replace contents of App.js with:

import React from 'react';
import { StatusBar } from 'expo-status-bar';
import { StyleSheet, Text, View, Button } from 'react-native';
import { SafeAreaProvider, SafeAreaView, initialWindowMetrics } from 'react-native-safe-area-context';

const tabs = ['Press these', 'buttons enough', 'and you will see', 'flashing content'];

class Header extends React.PureComponent {
  render() {
    return (
      <SafeAreaView style={{width: '100%'}} edges={['top', 'left', 'right']}>
        <Text style={{fontSize: 34}}>{this.props.title}</Text>
      </SafeAreaView>
    );
  }
}

class CurrentScreen extends React.PureComponent {
  constructor(props) {
    super(props);
    this.state = {
      tab: tabs[0],
    };
  }
  onPressTab(tab) {
    this.setState({tab});
  }
  render() {
    return (
      <View style={{flex: 1, width: '100%'}}>
        {/* the key forces the component to remount, making for simpler example */}
        <Header title={this.state.tab} key={this.state.tab} />
        <View>
          {tabs.map((tab) => (
            <Button onPress={this.onPressTab.bind(this, tab)} title={tab} key={tab}>{tab}</Button>
          ))}
        </View>
      </View>
    );
  }
}

export default function App() {
  return (
    <SafeAreaProvider style={styles.container} initialMetrics={initialWindowMetrics}>
      <CurrentScreen />
    </SafeAreaProvider>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    backgroundColor: '#fff',
    alignItems: 'center',
    justifyContent: 'center',
  },
});
npx expo run:ios --device
# choose a real device, not simulator

Changing tabs produces the problem about 20% of the time for me. I only see the problem when running on device... simulator is probably too slow.

tybro0103 avatar Dec 16 '22 17:12 tybro0103

Should be fixed now, no?

jacobp100 avatar Jan 19 '23 16:01 jacobp100

@jacobp100 No, it is not fixed.

I laid out a very simple reproduction in my last comment. Using those steps it did not work with v4.4.1 at the time. I just tried the same steps again with the latest v4.5.0, and the same problem is still happening. There's nothing in the changelog or commit history for v4.5.0 to suggest this was fixed.

Again this won't be noticeable in the simulator, but is very much an issue when running on real device.

tybro0103 avatar Jan 19 '23 17:01 tybro0103

https://github.com/th3rdwave/react-native-safe-area-context#optimization - does this fix your issue?

jacobp100 avatar Jan 19 '23 17:01 jacobp100

@jacobp100 No, it does not.

There was no reason to believe it would because this problem has nothing to do with app launch or device rotation. It's a problem at mount time of <SafeAreaView />, especially at times when one instance is unmounted and a new instance replaces it. I explained this in my first comment.

I can imagine a scenario where having the initialWindowMetrics could fix the issue, but if and only if the user is currently holding the device in the same orientation as they were when they launched the app... but having this only work for some orientations and not others isn't really a solution.

But despite all that, I went ahead and tried it out, and the problem still persists, regardless of orientation. I've updated my steps to reproduce to include this initialWindowMetrics bit.... these steps to reproduce are really the holy grail of steps to reproduce and really exactly what you need. Perform the steps and you will see.

It did just occur to me that I'm not sure if I mentioned this problem is on iOS devices specifically. I haven't tried Android.

tybro0103 avatar Jan 19 '23 17:01 tybro0103

Apologies. Was going through all the open issues

jacobp100 avatar Jan 19 '23 18:01 jacobp100

For anyone that might take this on, I wonder if it’s caused by us using didMoveToWindow

https://github.com/th3rdwave/react-native-safe-area-context/blob/c2d527f1f0d3ec073b1921ccae0c35ef1cf84e3c/ios/RNCSafeAreaView.m#L50

When this happens, it updates some RN state internally, and that might be slower than we’re expecting, causing the flicker. It might be ok to use willMove(toShperview:), which is called slightly sooner

Failing that, we might have to give each provider an ID, store each provider in a dictionary, then pass the provider ID via React context. That way we might be able to get the insets before these UIKit methods are called

jacobp100 avatar Jan 19 '23 18:01 jacobp100

Why SafeAreaProvider cannot use JSI for tooking real value from native side synchronously?

XantreDev avatar Mar 15 '23 16:03 XantreDev

As far as I'm aware, there's no mechanism for React to do synchronous updates just yet. That's the part that's stopping us.

jacobp100 avatar Mar 15 '23 19:03 jacobp100