th3rdwave / react-native-safe-area-context

A flexible way to handle safe area insets in JS. Also works on Android and Web!
MIT License
2.18k stars 201 forks source link

Improvement suggestion - First render should always include safe area insets #226

Open iway1 opened 2 years ago

iway1 commented 2 years ago

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.

jacobp100 commented 2 years ago

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.

danilobuerger commented 2 years ago

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

danilobuerger commented 2 years ago

@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;
jacobp100 commented 2 years ago

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

jameswilddev commented 2 years ago

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.

neiker commented 2 years ago

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

egorkhmelev commented 2 years ago

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.

jacobp100 commented 2 years ago

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

egorkhmelev commented 2 years ago

@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!

jacobp100 commented 2 years ago

What happens if you set follow the instructions here for this

egorkhmelev commented 2 years ago

@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 commented 2 years ago

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.

jacobp100 commented 2 years ago

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.

egorkhmelev commented 2 years ago

@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?

jacobp100 commented 2 years ago

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

egorkhmelev commented 2 years ago

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.

jivanovic commented 2 years ago

@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 😢

Pedroor commented 2 years ago

@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.

tcank commented 2 years ago

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.

efstathiosntonas commented 2 years ago

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

tybro0103 commented 1 year ago

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 commented 1 year ago

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.

jacobp100 commented 1 year ago

Should be fixed now, no?

tybro0103 commented 1 year ago

@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.

jacobp100 commented 1 year ago

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

tybro0103 commented 1 year ago

@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.

jacobp100 commented 1 year ago

Apologies. Was going through all the open issues

jacobp100 commented 1 year ago

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

XantreDev commented 1 year ago

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

jacobp100 commented 1 year ago

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.

alwex commented 1 year ago

Here is a workaround I have implemented in my app: 1 - first render a flex view that fill up the entire screen that call the hook useSafeAreaInsets 2 - once the insets.top value is set (greated than 0), render the child components

Here is the full example:

const Screen: React.FC<Props> = ({
  children,
}) => {
  const {top, bottom, left, right} = useSafeAreaInsets()

  const hasCalculatedInsets =
   top > 0 || bottom > 0 || left > 0 || right > 0

  if (!hasCalculatedInsets) {
    return (
        <Flex flex={1} />
    )
  }

  return (
    <Flex flex={1}>
      <Flex height={top} />
      {children}
      <Flex height={bottom} />
    </Flex>
  )
}

note that I use style-system here, <Flex flex={1} /> is the same as passing a <View style={{ flex: 1 }} /> With this solution, the first frame will be empty, but it is not noticeable and I find it better than the jumping UI I had without it, it is even possible to render this empty screen before hiding the splash screen for a seamless experience. Now this workaround assume that the top inset is never equal to 0, @jacobp100 is is something you can confirm? Or is it a wrong assumption?

tybro0103 commented 1 year ago

@alwex

The problem outlined in this issue only happens when using <SafeAreaView /> and not when using useSafeAreaInsets(). In fact, others have mentioned switching to useSafeAreaInsets() as a workaround.

top can be 0 - an iPhone in landscape.

alwex commented 1 year ago

I confirm that the issue also exists with useSafeAreaInsets on android, but not on iOS. I realized that top can be 0 on landscape mode, I have updated the workaround with

  const hasCalculatedInsets =
   top > 0 || bottom > 0 || left > 0 || right > 0

which should cover the majority of cases.

jacobp100 commented 1 year ago

If anyone is reliably able to reproduce this and wants to take a look into fixing it, I'd be more than happy to give pointers and review PRs

tslater commented 1 year ago

If anyone is reliably able to reproduce this and wants to take a look into fixing it, I'd be more than happy to give pointers and review PRs

Is there a way to tag gpt-4 and to get its (his? her? their?) attention?

tslater commented 1 year ago

Is there a way to put a bounty on this issue? Anyone else willing to put up some $$?

alexandrius commented 1 year ago

@tslater maybe this will help https://github.com/4TWIGGERS/rn-iphone-helper

vanenshi commented 1 year ago

we currently using a hack for this problem that might help you fix the problem

  1. First this hook. in summary, This hook helps you combine the other styles like paddingTop with the insets (credit: https://github.com/infinitered, link inside code)
import { FlexStyle } from 'react-native';
import {
  Edge,
  EdgeInsets,
  useSafeAreaInsets,
} from 'react-native-safe-area-context';

export type ExtendedEdge = Edge | 'start' | 'end';

const propertySuffixMap = {
  top: 'Top',
  bottom: 'Bottom',
  left: 'Start',
  right: 'End',
  start: 'Start',
  end: 'End',
};

const edgeInsetMap: Record<string, keyof EdgeInsets> = {
  start: 'left',
  end: 'right',
};

/**
 * A hook that can be used to create a safe-area-aware style object that can be passed directly to a View.
 *
 * - [Documentation and Examples](https://github.com/infinitered/ignite/blob/master/docs/Utils-useSafeAreaInsetsStyle.md)
 */
export function useSafeAreaInsetsStyle(
  safeAreaEdges: ExtendedEdge[] = [],
  property: 'padding' | 'margin' = 'padding',
): Pick<
  FlexStyle,
  | 'marginBottom'
  | 'marginEnd'
  | 'marginStart'
  | 'marginTop'
  | 'paddingBottom'
  | 'paddingEnd'
  | 'paddingStart'
  | 'paddingTop'
> {
  const insets = useSafeAreaInsets();

  return safeAreaEdges.reduce((acc, e) => {
    return {
      ...acc,
      [`${property}${propertySuffixMap[e]}`]: insets[edgeInsetMap[e] ?? e],
    };
  }, {});
}
  1. Second, a helper component (credit: https://github.com/th3rdwave/react-native-safe-area-context/issues/219#issuecomment-1169245221)
    
    import { FC, ReactNode } from 'react';
    import {
    StyleProp,
    StyleSheet,
    View,
    ViewProps,
    ViewStyle,
    } from 'react-native';
    import { ExtendedEdge, useSafeAreaInsetsStyle } from './useSafeAreaInsetsStyle';

export type SAViewProps = { children: ReactNode; style?: StyleProp; edges?: ExtendedEdge[]; } & ViewProps;

/**

you can use the hook directly, but I prefer a component since it is easier to use

joeporpeglia commented 3 months ago

I'm also observing the jump on first render when using SafeAreaView, but only in dev builds. Maybe it's happening too fast to notice, but curious if anyone has reproduced this in a release build.