-
-
Notifications
You must be signed in to change notification settings - Fork 507
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
Android + React-Navigation: Slow back animation/transitions #1020
Comments
Is it the same issue as #1017? If so, I think we should close this issue to keep the discussion in one place. |
I’m not 100% if it is the same as this happens whether or not there are are nested flat/scroll lists. This also happens for me only on back/pop actions (push works fine). Basically, the moment the view is added back to the tree is very noticeable, and it interrupts the JS animations used by react-navigation. You can clearly see when the this happens; animation starts, black background, view shows, animation freezes, then animation continues. A simple fix I could think of (if it makes sense), would be to have the screen added to the tree first, and then do the navigation once it’s confirmed that it has been added. It may slow the initial action but at least he UI would not stutter. Basically, the animation needs to wait until the view is added to the tree rather than doing it all at once. |
@cristianoccazinsp can you share a video of the behavior you described and a simple repo with a reproduction of it? The attach of below views is currently triggered by the start of the screen transition, you can see the code here: https://github.com/react-navigation/react-navigation/blob/9b2105692d77fb58715e97a0c375bdb53f5f3542/packages/stack/src/views/Stack/CardStack.tsx#L529. If you manage to come up with an idea how to postpone the transition, we will for sure consider merging such change. |
Here's a small video: https://drive.google.com/file/d/1YwEYNeDug8rV0i5tXxMqzZsc-PZcH57Y/view?usp=sharing I've on purpose used the heaviest screen (this is not normally this bad) as the base screen, and another heavy screen (camera) as the pushed screen. The pushed screen loads lazily after animations, so it does not stutter or break the transition. However, when going back, the transition freezes. Using this same screen as base, any other screen that is pushed / popped will have the same issue. Keep in mind I'm not using any nested flatlist/scroll view in the first test ( I do in the second one, which animates pretty well), just a regular scroll view with many, many children. From the iOS debugger, it shows about 8k views on screen. Note that this also happens on iOS, but it is much less noticeable (the animation is definitely slower and stutters a tiny bit at the end) |
Here's another one. This time on a much slower device, it is clear what happens between the pop operation, screen re-attach, and animation. The black background (the app's default splash screen rendered behind everything) should never be visible, no matter how slow the device is. If anything, the transition should be delayed until the screen is properly attached (I think the push operation behaves like this? The screen being hidden only hides after the animation completes). Could this somehow be a setting? Delay between transitions or something. I guess that even if a tiny delay is added, it will give time to the view to be attached and then the animation should happen smoothly, but I don't know how I could even test this by changing the source code on the fly. Screen.Recording.2021-07-21.at.11.15.03.mov |
@WoLewicki I believe I have found a fix (for react-navigation rather than this lib). I've tested the following changes, and the pop animation no longer stutters. Basically, we need two changes. First, we should consider in this line https://github.com/react-navigation/react-navigation/blob/9b2105692d77fb58715e97a0c375bdb53f5f3542/packages/stack/src/views/Stack/CardStack.tsx#L536 if there's a screen being closed, so we add an extra value to the + const closingRoutes = closingRouteKeys.length;
const outputValue =
index === self.length - 1
? STATE_ON_TOP // the screen is on top after the transition
- : index >= self.length - activeScreensLimit
+ : index >= self.length - activeScreensLimit - closingRoutes
? STATE_TRANSITIONING_OR_BELOW_TOP // the screen should stay active after the transition, it is not on top but is in activeLimit
: STATE_INACTIVE; // the screen should be active only during the transition, it is at the edge of activeLimit
isScreenActive = sceneForActivity
? sceneForActivity.progress.current.interpolate({
inputRange: [0, 1 - EPSILON, 1],
outputRange: [1, 1, outputValue],
extrapolate: 'clamp',
})
: STATE_TRANSITIONING_OR_BELOW_TOP; Lastly, we update this line here: https://github.com/react-navigation/react-navigation/blob/9b2105692d77fb58715e97a0c375bdb53f5f3542/packages/stack/src/views/Stack/Card.tsx#L140 - this.animate({ closing });
+ if (closing) {
+ setTimeout(() => {
+ this.animate({ closing });
+ });
+ } else {
+ this.animate({ closing });
+ } This pretty much delays the animation until the next JS loop, giving time to all the view processing to take place before animating the close operation. I don't know about consequences this may have overall to the rest of the react-navigation stack library, but IMHO the changes are pretty basic and harmless. The following video shows how the animation no longer stutters. Note that the screen is still slow, you can see a noticeable delay right after hitting the back button, but this is expected and is the delay we would otherwise see during the animation (as react-native-screens is most likely blocking the bridge/UI thread in order to re-attach the view tree). Under regular use, there should be no delay. https://drive.google.com/file/d/1nQsPh1n5ELrMZY_8xoKp0oZx_UexkJMI/view?usp=sharing @satya164 is this something you would consider implementing? |
@cristianoccazinsp we are investigating this issue, thanks for the reproduction and potential fix! One thing I am afraid of is that adding |
Do you have any usage of what could break? I mean animations already take time and this is just an extra cycle to give time to things to update, so it really shouldn’t affect anything. Could it perhaps be a setting? |
@cristianoccazinsp can you check if PR linked here: #1017 (comment) fixes the slow animation? |
@WoLewicki I will give it a shot. I just realized you guys are also migrating to kotlin so I need to fully install this branch as opposed to just replacing the files locally :( |
@WoLewicki the changes in that PR greatly improved the issue for me. Not only that, but it fixed another extremely ugly issue where the app's background (activity background) is visible for half a second when a screen is replaced without animation, or right at the end of the animation. Tested on a Pixel 5. Issue 1, activity background visible during some transitions fully fixed. Before and after video showing a Before PR: (notice the ugly screen flash right after replace) bg-before.mp4After PR: (notice no flashing of the app's background) bg-after.mp4However, under some circumstances, the animations may still freeze (e.g., when the camera preview pauses due to the surface going away, although this may be an issue with RNCamera). However, the change is a great improvement as it fixes 1 issue and more or less improves the laggy transitions. See screen push animation, it freezes for half a second right before the animation completes (it didn't happen without the PR). However, this only seems to happen on the screen that has the camera component and not in any other. slow.camera.mp4I think the final fixes should really come from react-navigation with changes similar to what I proposed above to fully prevent the bridge from blocking while the animations are happening. But these changes are really good, please consider reviewing and merging asap :), it's much better than the current behaviour. |
I think the problem with camera is not in the scope of this issue, and may be a general problem with fragments or a bug in |
@WoLewicki I have the same issue only with Google Pixel physical devices, any update for this, please? |
We haven't investigated it further as it seemed like not in scope of |
@WoLewicki the issue is back. I cannot really find the version this started to be an issue again as I've just noticed this but it has been there for a while now. The issue happens when popping screens. Do you want me to open a new issue? Current screens version: 3.13.1
See the following. Note that I've used a screen that has lots of views and subviews and is inefficient on purpose, but the freeze happens exactly in the same two very obvious moments, just faster on standard screens. screen-20220328-172151_2.mp4 |
Digging further, the freezing happens even with 3.7.0 which is where the issue was initially fixed. Looks like either Hermes or something in Android 12 and/or the androidx library has messed up something. Sadly, can't go back to non-hermes. Digging even further, works great on a Pixel 2 with Android 11, but works awful like above in the Pixel 5 Android 12, everything with the exact same app. |
Maybe on this exact phone, there is more code dispatched on the |
Tried on a pixel 6 with android 12 as well and no issues, must be this pixel 5 🥲 |
This lag at the end of the transition is noticed when moving from Camera screen to other screen.
This is because shutting of the Camera on blur. const unsubscribeBlur = navigation.addListener("blur", () => { setIsScreenReady(false); }); // to stop the camera when leaving screen But now, how to make the camera working back is another issue. |
Description
I've recently been testing with
enableScreens(true);
and it seems to work great for simple screens. However, for screens with many views (e.g., a very large list rendered in a flatlist), going back to it from another screen results in very slow animations. Basically, you can see the black background (or w/e bg your app has), it freezes half a second, and the animation continues.This does not happen when screens is not used, I'm guessing because all the views are there already. The initial view is lazy loaded when pushed, so this is not an issue when opening the screen, but it is awfully sluggish when another screen is popped.
I could use
detachPreviousScreen: true
on the pushed screen, but then that would make that screen very slow instead.Is using the native stack navigator the only way to improve this? Why even with the native driver the animations are so bad?
Screenshots
Steps To Reproduce
Note: I'm using the regular StackNavigator (
createStackNavigator
), and not the native one, but I'm also using the native driver.Expected behavior
Animations should not be blocked by screens being added to the tree.
Actual behavior
Slow animations
Snack or minimal code example
react-navigation config
Package versions
The text was updated successfully, but these errors were encountered: