Fix ReanimatedCommitHook cloning

[bartlomiejbloniarz]

Summary

This PR changes the way the ShadowTree is updated in the commit hook. Previously we would always clone the updated nodes, leading to RN also cloning the remaining nodes (some of them have never been mounted, as we are in the commit hook).

When RN clones uncommited nodes, it doesn't clone the state from the source node, but instead uses last mounted state, meaning that some of the updates will be lost.

To fix that, we don't clone unmounted ShadowNodes but instead change them in place. To recognize if a node was mounted we use the getHasBeenPromoted method.

This change needs to be well tested as modifying nodes in place has to be done carefully, since it's not really the intended way.

fixes #6659

Test plan

Check behaviour of the repro from #6659, ChessboardExample, BokehExample

[chrfalch]

Awesome work, @bartlomiejbloniarz - thanks for looking into this!! Tell me if this is ready to be tested and I can help!

[bartlomiejbloniarz]

@chrfalch I think it's ready to be tested now. Thanks!

[j-piasecki]

LGTM, but I'm still unsure whether getHasBeenPromoted is a strong enough condition.

[bartlomiejbloniarz]

@j-piasecki Yeah, I'm also worried about that. I think that if there will be no issues during testing and this gets merged, we will release a RC version.

[mhoran]

While this patch resolves the modal issue for me on iOS, it seems to have cause a significant regression in useAnimatedStyle for my use case, causing the keyboard to get stuck in an infinite open and close loop.

[bartlomiejbloniarz]

@mhoran It breaks on both iOS and Android?

[Vali-98]

Tested this on Android - this fixes the issue where modals are rendered off-screen or have 0 height/width causing them to be invisible while useAnimatedStyle exists elsewhere outside the Modal.

However, this does not fix cases where Entering animations of an Animated.View inside a Modal seems to have 0 height/width/x/y, despite onLayout reporting the heights and widths properly.
eg:

<Modal>
    <Animated.View entering={...}  >
     // children inside are rendered properly, but the width/height/x/y of the parent Animated.View is 0
    </Animated.View>
</Modal>

One would assume these issues are somehow interlinked as they both are Modal related and cause zero'd out layout values.

[mhoran]

@mhoran It breaks on both iOS and Android?

Yes, in slightly different ways.

On iOS, I see two issues. First, issues with TextInput onFocus and onBlur events triggering an animation. This gets stuck in a loop, opening and then immediately closing the keyboard.

The second issue is an interaction with ScrollView and TouchableHighlight. The ScrollView cannot be scrolled; it gets stuck.

Simulator.Screen.Recording.-.iPad.mini.A17.Pro.-.2024-12-03.at.17.34.48.mp4

On Android, I only see the ScrollView issue, and less often; but it is still reproducible.

I found that this seems to be some interaction with react-native-drawer-layout. The issues only seem to occur when the drawer is open when the app starts. I suspect this is because the drawer is still seen as "in front" of the ScrollView due to the underlying commit hook issue. If I remove the TouchableHighlight I can scroll the ScrollView, but the keyboard issue persists. Removing the drawer layout resolves all issues.

I have created a reproducer here: https://github.com/mhoran/react-native-scratch/tree/reanimated-commit-hook-issue (reanimated-commit-hook-issue branch). Note this is using a local checkout of react-native-reanimated and expects it to be located next to the repo. Change package.json as necessary. It uses npm as the package manager and leverages symlinks.

[gaearon]

Is it possible to fix the root issue instead? It sounds like this is going against immutable tree updates — which is worrying to me because it is a fundamental constraint of the new RN architecture. It is worrying when Reanimated as a library keeps diverging from how RN works instead of getting closer to it.

Looking at #6659 (comment):

But whenever we clone a ShadowNode that has not been mounted, YogaLayoutableShadowNode re-clones all of its children. When cloning, RN uses the last mounted state, meaning that it actually overrides the height assigned by RNScreens, with wrong value.

This sounds like the proper fix should either be in RNScreens (does it mutate stuff it shouldn't?) or in how it interacts with Yoga. Can the fix be investigated there instead?

[gaearon]

It would be great to find a person on the React Native team who can help brainstorm the right integration layer between these libraries and ensure that they actually fit into the architecture. I presume it would be whoever added the commit hook functionality.

[gaearon]

cc @sammy-SC maybe you have thoughts on the right way to handle this

[gaearon]

Basically, the point I'm trying to make is that immutable tree updates is the entire premise of the Fabric architecture. It's a fundamental constraint. Every technical decision about how RN works will be relying on this constraint — both existing decisions and future decisions. So, in my understanding, it is not appropriate for a library to break this constraint and mutate these nodes — or worse, fight over who gets to mutate these nodes. If one library starts undermining this constraint, it isn't a good reason for another library to give in and undermine it even further. Instead, the concern needs to be brought upstream and a resolution within the architecture needs to be found. Reanimated is used very widely so if it starts violating the fundamental constraints of Fabric, this makes it hard for RN itself to change its implementation and take advantage of those constraints. Since changes that were previously unobservable would now become observable. I think this is very important and I want to make sure that this point of view is heard (you probably hear a lot of pragmatic "just fix it now somehow" so I want to balance that out a bit). Thanks.

[chrfalch]

I agree with @gaearon as well. From Expo's side this is also something we feel very strongly about getting right - these libraries (REA/RNScreens) is as you say a fundamental part of almost all RN apps out there and will affect so many other libraries as well (React Navigation and a lot of other libs).

I don't think the problem is mutating stuff that shouldn't be mutated - If I remember correctly the issue started as an optimization to make sure Reanimated didn't clone shadow nodes too often. The problem was that when running animations the code ended up cloning single shadow nodes multiple times in each pass - causing a performance degradation (correct me if I'm wrong, @bartlomiejbloniarz).

A PR (#6214) was merged that optimized this to avoid "over-cloning" - but it seemed that the PR also introduced some side effects causing some layout issues that was mentioned in one of the first issues (#6659).

A suggested way forward would be to remove the optimizations to create a baseline and verify that all the reported issues are working correct. Then we can measure and investigate which optimizations that should be made.

As it is now, I also feel a bit insecure about what Dan also pointed out - the root cause of this problem (in the new architecture).

[bartlomiejbloniarz]

@chrfalch Reverting #6214 wouldn't actually solve the problem. I mentioned in #6659 that this issue was also present in the old algorithm. Funnily enough it didn't show up in the Debug mode there, because we were updating ShadowNodes in-place, as an optimization. The new algorithm actually stopped in-place updates, and that's why those issues manifested.

I agree that the root cause of this issue is:

But whenever we clone a ShadowNode that has not been mounted, YogaLayoutableShadowNode re-clones all of its children. When cloning, RN uses the last mounted state, meaning that it actually overrides the height assigned by RNScreens, with wrong value.

I am not sure when this behavior was introduced to RN, so that's something I need to examine.

@gaearon let me explain in more detail what we are doing here, as we are not exactly breaking the ShadowTree immutability, but as I was debugging the problems that this PR introduces, I am getting more convinced that our approach needs radical rethinking. Basically:

• our animations are applied by simply commiting a new ShadowTree every frame
• those updates are not seen by the RN renderer, as the new ShadowNodes are not pushed to JS
• so, if RN performs a render when an animation is running, it will commit a ShadowTree, that doesn't contain our animation updates, resulting in a flicker
• to mitigate that, we use the CommitHook api. Basically, the shadowTreeWillCommit method is called whenever a new tree is about to be commited (before the layout is calculated). This method allows us to inspect the new tree, and return a new one, that will be commited instead.
• so we have a big registry of all the updates, that we reapply on every non-reanimated commit

This PR is only about the algorithm that we use to modify the tree that we get in the shadowTreeWillCommit method. Before this PR (#6776), we would clone every ShadowNode that we wanted to animate (and also the entire path from there to the root). With my changes we instead check if a node has been commited and if not, then we modify it in place. So we are not mutating the mounted ShadowTree. We are just intercepting the tree that's about to be commited (using the dedicated CommitHook api) and mutating it before it's actually promoted to be the new ShadowTree. The motivation behind this change was that, when we cloned those un-commited ShadowNodes the whole tree would be re-cloned and we would lose State updates (and things like the Modal height in #6659 would break).

But again, this approach comes with its own problems, that came up in testing, and it's very likely that this needs to be rethinked.

[gaearon]

Makes sense, thanks for explaining! Seems important that the team finds some approach that integrates nicely with RN itself. Does RN.Animated avoid these problems somehow?

[chrfalch]

@chrfalch Reverting #6214 wouldn't actually solve the problem. I mentioned in #6659 that this issue was also present in the old algorithm. Funnily enough it didn't show up in the Debug mode there, because we were updating ShadowNodes in-place, as an optimization. The new algorithm actually stopped in-place updates, and that's why those issues manifested.

You explained it very well - thanks @bartlomiejbloniarz (also for looking into this is such a throughout way)

[bartlomiejbloniarz]

@gaearon

Animated doesn't really go through the same pipeline, since it either applies transforms directly to views, when using nativeDriver or it re-renders the tree on every frame from JS.

[bartlomiejbloniarz]

@mhoran Could you test this version of this PR on your app?

[chrfalch]

@bartlomiejbloniarz - sorry for the long wait - but I've now spent some time testing this issue and can verify that it now seems to be working correct in Debug/Release builds using the reproduction project - both with/without the last commit you've added. Great work!

[mhoran]

@mhoran Could you test this version of this PR on your app?

On iOS, the issues I reported with the previous version of this patch look to be resolved with the latest patch. I still see a regression on new arch if padding is changed in the same useAnimatedStyle block as a transform (the screen flickers), but perhaps that is unrelated to the commit hook issue.

Things are looking much better on Android as well. This seems to resolve issues I was having with TouchableOpacity not knowing whether it was pressed. I'm now able to navigate to screens that were previously inaccessible due to that bug. I'm getting a hang with animated padding, but that may also be unrelated to the commit hook issue, as it is not affected by this patch.

Great work!

[mhoran]

One small issue remains with modals on both Android and iOS. On Android, I've seen the modal animate almost entirely off-screen to the top left corner before jumping to the correct position. On iOS, modals sometimes animate in from the bottom right instead of bottom center. Unfortunately I've found this incredibly difficult to reproduce, but there still seems to be some sort of issue with modals in particular.

[samducker]

This issue seems to affect useAnimatedKeyboard also I am having performance issues with react-native-gifted-chat (which uses useAnimatedKeyboard and reanimated) since I moved to the new architecture. When I open the keyboard there is noticeable heavy lag similar to the issue with modals like gorham/bottom-sheet.

[mhoran]

This issue seems to affect useAnimatedKeyboard also I am having performance issues with react-native-gifted-chat (which uses useAnimatedKeyboard and reanimated) since I moved to the new architecture. When I open the keyboard there is noticeable heavy lag similar to the issue with modals like gorham/bottom-sheet.

How are you animating the view? If you have padding (or margin or height) in an animated style, it will cause flickering. This is a regression in new arch, but I'm not sure this issue is the root cause.

Animated padding (margin and height) had poor performance on old arch (see #5685), but in new arch, even having a static padding, margin or heading value in an aimated style block will cause flickering.

I worked around this by spotting my animated styles in two (one for the transform, one for padding), and ensuring that the padding style updates as infrequently as possible.

[samducker]

Hey @mhoran yeah my bad I think you're right as I just applied the new PR as a patch to the latest RC and it is not resolving my issues with gorhom/bottom-sheet lagging and lack of space at the bottom + the keyboard issue I mentioned

I made this assumption based on the other lagging issues related to this PR and reading this issue in gorhom bottom sheet gorhom/react-native-bottom-sheet#2046

It is also not resolving my keyboard issues in gifted chat either, the code is from gifted chat and is here https://github.com/FaridSafi/react-native-gifted-chat/blob/master/src/GiftedChat.tsx

[bartlomiejbloniarz]

@mhoran Do you see the issue with modal animation only with this PR or is it something that also happens on main?

[mhoran]

@mhoran Do you see the issue with modal animation only with this PR or is it something that also happens on main?

On Android, modals always animate to the top left corner and remain off screen. With this PR, they only animate to the top left corner sometimes, and always wind up in the correct place. The animation is just wrong, sometimes.

On iOS, this PR seems to introduce a regression, but it's very hard to tell. I only see modal issues on iOS when the window has been resized to less than full screen using Stage Manager. The modals display in odd places, and do not show entirely on screen. As such, it's very difficult to tell where it is animating from, since the full modal is not visible. It could be that this issue exists on main or it could be a regression, and reproducing is very tricky.

[latekvo]

When testing with the modal bug repro reported here: #6659, the invalid modal rendering is still visible during the modal entrance animation.
This issue seems to only occur on a physical device.

Test code

import {useState} from 'react';
import {Button, Modal, SafeAreaView, StatusBar, View} from 'react-native';
import Animated, {useAnimatedStyle} from 'react-native-reanimated';

export default function App() {
  const [visible, setVisible] = useState(false);

  const animatedStyle = useAnimatedStyle(() => ({
    transform: [{translateX: 0}],
  }));

  return (
    <SafeAreaView
      style={{
        flex: 1,
        backgroundColor: '#fff',
        padding: 16,
      }}>
      <StatusBar />
      <View
        style={{
          paddingVertical: 16,
        }}
      />
      <Button
        title="Open Modal"
        onPress={() => {
          setVisible(true);
        }}
      />
      <Animated.View style={animatedStyle} />
      <Modal visible={visible} animationType="slide">
        <View
          style={{
            padding: 32,
            backgroundColor: 'pink',
          }}/>
      </Modal>
    </SafeAreaView>
  );
}

On the following video, the entrance animation is 8 frames long.
During the first 3 frames of the animation - there's a pink square moving on the bottom-left corner of the screen, before being covered up by the modal.

trimmed_repro.mp4

[mhoran]

As mentioned in react-navigation/react-navigation@9831292#commitcomment-150759476, this patch no longer worked for me on iOS once I upgraded react-native-drawer-layout to 4.1.1. I see issues with ScrollViews and cannot open the keyboard, as if dirtyLayout() was not added.