Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V2 seems to have performance issues #82

Closed
panda0603 opened this issue May 25, 2020 · 18 comments
Closed

V2 seems to have performance issues #82

panda0603 opened this issue May 25, 2020 · 18 comments

Comments

@panda0603
Copy link

panda0603 commented May 25, 2020

greetings,

just upgraded to v2.0.0 from v0.7.3.

I am using react-navigation V5 at the moment with latest version.

upon upgrading to 2.0.0, stack navigation animations lags / stutters on some screens(maybe because of native code changes in 2.0.0?).

Confirmed that this only happens on the latest version of react-native-safe-area-context,

since v0.7.3 works fine. Also tried v1.0.2, works fine as well.

besides react navigation, I am using safeAreaViews in my screens from react-native-safe-area-context as well.

Due to the problem, at the moment I upgraded to 1.0.2 from 0.7.3.

@janicduplessis
Copy link
Collaborator

Hi @panda0603,

We changed the implementation of SafeAreaView from JS to Native in 2.0.0 so it is probably where the issue comes from. Does the perf issue happen on a specific platform (iOS / Android)?

@ahce
Copy link

ahce commented May 26, 2020

Hi!
I created this repo to reproduce the issues.

  • iOS: flicker after navigate
  • Android: screen cut after navigate

@jacobp100
Copy link
Collaborator

Thanks for the repo! Are you able to attach videos that demonstrate the flicker and screen cut? Just so we're definitely talking about the same thing

Has anyone tried this with a native stack navigator from react-native-screens?

@janicduplessis
Copy link
Collaborator

I’m using native stack with v2 and haven’t noticed issues.

@jacobp100
Copy link
Collaborator

If you’re mixing the hooks and the native view, the native view will update before the hooks

I’ve got some comments about other stuff I’ve noticed too, but will add them to my margin PR

@ahce
Copy link

ahce commented May 26, 2020

I updated the repository, with this var you can switch between default and native stack.
The issue occurs with default stack.

@janicduplessis
Copy link
Collaborator

@panda0603 Can you try 2.0.3? I might have found the issue.

@panda0603
Copy link
Author

panda0603 commented May 27, 2020

@janicduplessis
Thanks for the lookup.
Problem still persists after 2.0.3 on both android and ios.
I think problem lies not only in safeAreaView, but also while in interaction with react navigation navigate animation.

When I add wrap a component with safeAreaView, that wrapped component seems to have delay upon showing(added gif, notice how to bottom menubar pops up).

So I removed safeAreaView, the delay of the menubar is gone but the navigation lag, stuttering effect is still there (gif doesn't seem to show it cause of low frame rate). Hope this helps.

ezgif com-video-to-gif

@satya164
Copy link
Collaborator

If I understand correctly, the problem is, that when animating a screen, the safe area will keep changing, which will cause tons of updates during the animation.

@jacobp100
Copy link
Collaborator

So this is something I noticed too. I had to change that behaviour in my margins PR (#85) - maybe you could give that branch a try

@janicduplessis
Copy link
Collaborator

janicduplessis commented May 27, 2020

I see, I wonder if we could prevent it from updating during animations, or just prevent it from updating too quickly. Some sort of throttling, I also noticed that when using it inside a scrollview it would be very janky because it would try to update all the time.

@jacobp100 The part I don't like about this solution is the SafeAreaView no longer applies padding only to edges that touch the safe area automatically. Ideally I'd like to preserve this behavior.

Looks like iOS has some built in way of dealing with animations and safe area but since react-native doesn't use UIView animations it won't work. https://stackoverflow.com/questions/46670921/ios-11-safe-areas-vs-uiview-animation.

To integrate properly with animations we'd need to know the final position of the view that is being animated and have the insets calculated based on that. Sadly react-native Animated does not have any infra to support this.

So in summary the options are

  1. Throttling: Probably a bad idea since it will cause a weird update at the end of the animation, for this to work the safe area would need to be calculated based on where the animation is ending, which is not possible.
  2. Use insets from the parent view controller and use edges prop to control which edges the inset is applied to: Requires more manual work but is probably the more reliable solution. I think in most cases it is possible to know which edges the SafeAreaView needs to apply to.
  3. Use UIView animations / add infra in RN Animated: Might be the ideal solution but probably requires too much work.

@janicduplessis
Copy link
Collaborator

Thinking about it more and I think going for 2. is the right call, this is also how SafeAreaView behaved in 1.0.

@janicduplessis
Copy link
Collaborator

Using view controller / fragments as the view to base insets on will also integrate nicely with RN Screens and iOS 13 modals.

@jacobp100
Copy link
Collaborator

jacobp100 commented May 27, 2020

2 is the behaviour in the margins pr. I'm still thinking what's best too - but I'll add a few notes

For iOS's default implementation
  • Matches the behaviour of contentInsetAdjustmentBehavior on ScrollViews
For option 2
  • Matches v1
  • If we allow the user to set edges, it would be counter-intuitive that they sometimes aren't applied even when set
  • We'd need better documentation on this behaviour, and there's not too much documentation of the finer details (stuff inside scrollviews)
  • We'll probably never iOS's implementation on other platforms
  • We can support changing the margins

@janicduplessis
Copy link
Collaborator

janicduplessis commented May 28, 2020

Ok so I went for what I think is a pretty good compromise in term of usability and perf. SafeAreaView is now relative to the nearest SafeAreaProvider like it was in v1.0. This way it is possible to control where the inset is calculated from and avoid it being inside a view that is animated. It also still works well with native stack where you can have a provider inside each screen since it is not animated using RN and handles insets update properly.

Released as 3.0.0-beta.1 if you can test it that would be great.

Will document this behavior properly.

@ahce
Copy link

ahce commented May 28, 2020

Hi! I tested v3.0.2:

  • Works fine on Android
  • Improved on iOS, but the flickering continues

Repo --> here

@jacobp100
Copy link
Collaborator

jacobp100 commented May 29, 2020

The flickering could be some code using the native safe area view, and some code getting it from the hooks. I have a few places in my so where I have to use the hook and I get a flicker switching between portrait and landscape

Also v3 is working a treat. Getting the safe areas from the provider view is smart!

@janicduplessis
Copy link
Collaborator

Gonna close this as the issue seems mostly fixed, gonna open an issue with the repro @ahce provided to investigate further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants