[LayoutAnimations] Fix exiting animations inside a Native Stack Navigator #3865

jwajgelt · 2022-12-14T10:31:44Z

Summary

Currently, removing a screen with exiting layout animations running cause views to not be properly removed from the native hierarchy, as well as leaking memory in Layout Animations' AnimationManager.

This is caused by the ancestors of exiting views being removed from the native hierarchy by the React-Native-Screens implementation, while our implementation assumes they're still mounted in the native hierarchy.

Test plan

In the Example app, go to the "Nested NativeStacks with layout" screen and try toggling and navigating around while the exiting animation on the second screen of the nested stack is running.

jwajgelt · 2022-12-14T10:37:00Z

...ager/latest/com/swmansion/reanimated/layoutReanimation/ReanimatedNativeHierarchyManager.java

+      if (indicesToRemove != null) {
+        for (int index : indicesToRemove) {
+          View child = viewGroupManager.getChildAt(viewGroup, index);
+          ((ReaLayoutAnimator) mReaLayoutAnimator).cancelAnimationsInSubviews(child);
+        }
+      }


The fix for Android is to cancel all the animations in the transaction that removes the Screen.
This will cause all the exiting views to unregister their ancestors before being unmounted.

android/src/main/java/com/swmansion/reanimated/layoutReanimation/AnimationsManager.java

jwajgelt · 2022-12-14T10:43:27Z

ios/LayoutReanimation/REAAnimationsManager.m

+  NSNumber *parentTag = _exitingParentTags[child.reactTag];
+  [_exitingParentTags removeObjectForKey:child.reactTag];
+  while (parentTag != nil && ![parent isKindOfClass:[RCTRootView class]]) {
    UIView *view = parent;
+    NSNumber *viewTag = parentTag;
+    parentTag = _exitingParentTags[viewTag];
+    UIView *viewByTag = [self viewForTag:viewTag];
    parent = view.superview;
+
+    if (view == nil) {
+      if (viewByTag == nil) {
+        // the view was already removed from both native and RN hierarchies
+        // we can safely forget that it had any animated children
+        [_ancestorsToRemove removeObject:viewTag];
+        [_exitingSubviewsCountMap removeObjectForKey:viewTag];
+        [_exitingParentTags removeObjectForKey:viewTag];
+        continue;
+      }
+      // the child was dettached from view, but view is still
+      // in the native and RN hierarchy
+      view = viewByTag;
+    }
+
    if (view.reactTag == nil) {
+      // we skip over views with no tag when registering parent tags,
+      // so we shouldn't go to the parent of viewTag yet
+      parentTag = viewTag;
      continue;
    }
+


On iOS, we can't simply cancel animations in the removed screen like on Android, since by the time manageChildren is called on the removed screen, it's already been removed from the native hierarchy.

Instead, when unregistering ancestors when the animations end, we use the tags of ancestors instead of view.superview refs to find the parents of the exiting views.

kmagiera · 2022-12-23T13:13:58Z

I think this approach is ok. I'm only wondering if the changes that check for RNScreen are necessary for this to work. If I understand this correctly, it seem like these are separate things, and the changes that reference screens are there to prevent exiting views from animating when the whole screen is unmounted. If that's the case I think it'd be better to split these changes into separate PRs. Also blocking exiting animations in such a case may be in conflict with the work we do on SET (cc @piaskowyk )

jwajgelt · 2023-01-04T15:40:41Z

I think this approach is ok. I'm only wondering if the changes that check for RNScreen are necessary for this to work. If I understand this correctly, it seem like these are separate things, and the changes that reference screens are there to prevent exiting views from animating when the whole screen is unmounted. If that's the case I think it'd be better to split these changes into separate PRs. Also blocking exiting animations in such a case may be in conflict with the work we do on SET (cc @piaskowyk )

On iOS, cancelling animations in subviews of a RNSScreenStackView fixes an issue when unmounting an entire stack with exiting animations inside the currently mounted screen keeping the entire stack alive for the duration of the animation (since in that case, Screens don't unmount the views themselves.)

…_native_stack_fix

Example/package.json

...ager/latest/com/swmansion/reanimated/layoutReanimation/ReanimatedNativeHierarchyManager.java

android/src/main/java/com/swmansion/reanimated/layoutReanimation/AnimationsManager.java

ios/LayoutReanimation/REAUIManager.mm

piaskowyk · 2023-01-12T08:49:59Z

Could you also resolve merge conflicts?

piaskowyk · 2023-01-12T15:44:00Z

Could you also check how it works on native stack and js stack?

jwajgelt · 2023-01-18T14:58:41Z

@piaskowyk

Could you also check how it works on native stack and js stack?

It does, though this shouldn't really change the behaviour with js stack.

…_native_stack_fix

Example/ios/ReanimatedExample.xcodeproj/project.pbxproj

...ager/latest/com/swmansion/reanimated/layoutReanimation/ReanimatedNativeHierarchyManager.java

android/src/main/java/com/swmansion/reanimated/layoutReanimation/AnimationsManager.java

ios/LayoutReanimation/REAAnimationsManager.m

piaskowyk · 2023-01-20T16:13:28Z

This is the last iteration and then we could merge it

…ator (software-mansion#3865)  ## Summary Currently, removing a screen with `exiting` layout animations running cause views to not be properly removed from the native hierarchy, as well as leaking memory in Layout Animations' `AnimationManager`. This is caused by the ancestors of exiting views being removed from the native hierarchy by the React-Native-Screens implementation, while our implementation assumes they're still mounted in the native hierarchy. ## Test plan  In the Example app, go to the "Nested NativeStacks with layout" screen and try toggling and navigating around while the `exiting` animation on the second screen of the nested stack is running.

jwajgelt added 5 commits December 14, 2022 11:33

native stack in Example app

f794095

add example breaking native stack

22baeee

fix exiting in native stack on Android

a3fe2a0

cancel animations in native stack ios

d1fa903

fix cleaning up ancestors of exiting on ios

109ded6

jwajgelt force-pushed the @jwajgelt/layout_anim_native_stack_fix branch from b594651 to 109ded6 Compare December 14, 2022 10:33

jwajgelt commented Dec 14, 2022

View reviewed changes

android/src/main/java/com/swmansion/reanimated/layoutReanimation/AnimationsManager.java Show resolved Hide resolved

jwajgelt commented Dec 14, 2022

View reviewed changes

jwajgelt self-assigned this Dec 16, 2022

jwajgelt added the Area: Layout Animations label Dec 16, 2022

jwajgelt requested review from tomekzaw and piaskowyk December 16, 2022 08:29

fix callbacks sometimes not being called when removing views on Android

14d09e7

try removing the dependency on RNScreens for layout animations

3c4d0a6

jwajgelt force-pushed the @jwajgelt/layout_anim_native_stack_fix branch from 416db71 to 3c4d0a6 Compare January 9, 2023 08:49

jwajgelt marked this pull request as ready for review January 9, 2023 08:50

Merge remote-tracking branch 'origin/main' into @jwajgelt/layout_anim…

84b7840

…_native_stack_fix

jwajgelt changed the title ~~[WIP][LayoutAnimations] Fix exiting animations inside a Native Stack Navigator~~ [LayoutAnimations] Fix exiting animations inside a Native Stack Navigator Jan 11, 2023

piaskowyk reviewed Jan 12, 2023

View reviewed changes

jwajgelt added 2 commits January 13, 2023 08:43

Merge branch 'main' into @jwajgelt/layout_anim_native_stack_fix

54b0728

fix formatting

e985442

jwajgelt force-pushed the @jwajgelt/layout_anim_native_stack_fix branch from cb6042a to e985442 Compare January 13, 2023 08:44

jwajgelt added 2 commits January 18, 2023 15:59

Merge remote-tracking branch 'origin/main' into @jwajgelt/layout_anim…

8cedbed

…_native_stack_fix

revert version changes

3120c4b

jwajgelt added 2 commits January 18, 2023 16:32

Update Weave.tsx

2fb0557

Update Weave.tsx

f209514

piaskowyk requested changes Jan 20, 2023

View reviewed changes

jwajgelt added 2 commits January 23, 2023 08:46

code review fixes

eb931ce

more code review fixes

f2140b4

piaskowyk approved these changes Jan 23, 2023

View reviewed changes

piaskowyk merged commit e6e239a into main Jan 23, 2023

piaskowyk deleted the @jwajgelt/layout_anim_native_stack_fix branch January 23, 2023 08:30

zhelezkov mentioned this pull request Jan 27, 2023

[ios][bug] Huge issue with LayoutAnimation - Single Animated.View with layout prop is enough to break un-mounting and de-allocation of any View which has been popped/swiped-back/closed/destroyed via navigation #3124

Closed

3 tasks

jwajgelt mentioned this pull request Feb 20, 2023

Layout animation display when component unmount #4063

Closed

KiwiKilian mentioned this pull request Mar 31, 2023

BarCodeScanner keeps scanning after unmount on iOS expo/expo#21868

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LayoutAnimations] Fix exiting animations inside a Native Stack Navigator #3865

[LayoutAnimations] Fix exiting animations inside a Native Stack Navigator #3865

jwajgelt commented Dec 14, 2022

jwajgelt Dec 14, 2022

jwajgelt Dec 14, 2022

kmagiera commented Dec 23, 2022

jwajgelt commented Jan 4, 2023

piaskowyk commented Jan 12, 2023

piaskowyk commented Jan 12, 2023

jwajgelt commented Jan 18, 2023

piaskowyk commented Jan 20, 2023

[LayoutAnimations] Fix exiting animations inside a Native Stack Navigator #3865

[LayoutAnimations] Fix exiting animations inside a Native Stack Navigator #3865

Conversation

jwajgelt commented Dec 14, 2022

Summary

Test plan

jwajgelt Dec 14, 2022

Choose a reason for hiding this comment

jwajgelt Dec 14, 2022

Choose a reason for hiding this comment

kmagiera commented Dec 23, 2022

jwajgelt commented Jan 4, 2023

piaskowyk commented Jan 12, 2023

piaskowyk commented Jan 12, 2023

jwajgelt commented Jan 18, 2023

piaskowyk commented Jan 20, 2023