Fix handling removal of transitioning views

[kkafar]

Summary:

Related PR in react-native-screens:

• fix(Android): going back on fabric with removeClippedSubviews software-mansion/react-native-screens#2495

Additional context:

• my detailed explanation of one of the issues
• Android Developer: ViewGroup.startViewTransition docs

Background

On Android view groups can be marked as "transitioning" with a ViewGroup.startViewTransition call. This effectively ensures, that in case a view group is marked with this call and its children are removed, they will be still drawn until endViewTransition is not called.

This mechanism is implemented in Android by keeping track of "transitioning" children in auxiliary mTransitioningViews array. Then when such "transitioning" child is removed, it is removed from children array but it's parent-child relationship is not cleared and it is still retained in the auxiliary array.

Having that established we can proceed with problem description.

Problem

Screen.Recording.2024-11-15.at.16.14.57.mov

Full code

import { NavigationContainer } from '@react-navigation/native';
import React from 'react';
import { createNativeStackNavigator } from '@react-navigation/native-stack';
import { enableScreens } from 'react-native-screens';
import {
  StyleSheet,
  Text,
  View,
  FlatList,
  Button,
  ViewProps,
  Image,
  FlatListProps,
  findNodeHandle,
} from 'react-native';

enableScreens(true);

function Item({ children, ...props }: ViewProps) {
  return (
    <View style={styles.item} {...props}>
      <Image source={require('../assets/trees.jpg')} style={styles.image} />
      <Text style={styles.text}>{children}</Text>
    </View>
  );
}

function Home({ navigation }: any) {
  return (
    <View style={styles.container}>
      <Button title="Go to List" onPress={() => navigation.navigate('List')} />
    </View>
  );
}

function ListScreenSimplified({secondVisible}: {secondVisible?: (visible: boolean) => void}) {
  const containerRef = React.useRef<View>(null);
  const innerViewRef = React.useRef<View>(null);
  const childViewRef = React.useRef<View>(null);

  React.useEffect(() => {
    if (containerRef.current != null) {
      const tag = findNodeHandle(containerRef.current);
      console.log(`Container has tag [${tag}]`);
    }
    if (innerViewRef.current != null) {
      const tag = findNodeHandle(innerViewRef.current);
      console.log(`InnerView has tag [${tag}]`);
    }
    if (childViewRef.current != null) {
      const tag = findNodeHandle(childViewRef.current);
      console.log(`ChildView has tag [${tag}]`);
    }
  }, [containerRef.current, innerViewRef.current, childViewRef.current]);

  return (
    <View
      ref={containerRef}
      style={{ flex: 1, backgroundColor: 'slateblue', overflow: 'hidden' }}
      removeClippedSubviews={false}>
      <View ref={innerViewRef} removeClippedSubviews style={{ height: '100%' }}>
        <View ref={childViewRef} style={{ backgroundColor: 'pink', width: '100%', height: 50 }} removeClippedSubviews={false}>
          {secondVisible && (<Button title='Hide second' onPress={() => secondVisible(false)} />)}
        </View>
      </View>
    </View>
  );
}

function ParentFlatlist(props: Partial<FlatListProps<number>>) {
  return (
    <FlatList
      data={Array.from({ length: 30 }).fill(0) as number[]}
      renderItem={({ index }) => {
        if (index === 10) {
          return <NestedFlatlist key={index} />;
        } else if (index === 15) {
          return <ExtraNestedFlatlist key={index} />;
        } else if (index === 20) {
          return <NestedFlatlist key={index} horizontal />;
        } else if (index === 25) {
          return <ExtraNestedFlatlist key={index} horizontal />;
        } else {
          return <Item key={index}>List item {index + 1}</Item>;
        }
      }}
      {...props}
    />
  );
}

function NestedFlatlist(props: Partial<FlatListProps<number>>) {
  return (
    <FlatList
      style={[styles.nestedList, props.style]}
      data={Array.from({ length: 10 }).fill(0) as number[]}
      renderItem={({ index }) => (
        <Item key={'nested' + index}>Nested list item {index + 1}</Item>
      )}
      {...props}
    />
  );
}

function ExtraNestedFlatlist(props: Partial<FlatListProps<number>>) {
  return (
    <FlatList
      style={styles.nestedList}
      data={Array.from({ length: 10 }).fill(0) as number[]}
      renderItem={({ index }) =>
        index === 4 ? (
          <NestedFlatlist key={index} style={{ backgroundColor: '#d24729' }} />
        ) : (
          <Item key={'nested' + index}>Nested list item {index + 1}</Item>
        )
      }
      {...props}
    />
  );
}

const Stack = createNativeStackNavigator();

export default function App(): React.JSX.Element {
  return (
    <NavigationContainer>
      <Stack.Navigator screenOptions={{ animation: 'slide_from_right' }}>
        <Stack.Screen name="Home" component={Home} />
        <Stack.Screen name="List" component={ListScreenSimplified}/>
      </Stack.Navigator>
    </NavigationContainer>
  );
}

export function AppSimple(): React.JSX.Element {
  const [secondVisible, setSecondVisible] = React.useState(false);

  return (
    <View style={{ flex: 1, backgroundColor: 'lightsalmon' }}>
      {!secondVisible && (
        <View style={{ flex: 1, backgroundColor: 'lightblue' }} >
          <Button title='Show second' onPress={() => setSecondVisible(true)} />
        </View>
      )}
      {secondVisible && (
        <ListScreenSimplified secondVisible={setSecondVisible} />
      )}
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    alignItems: 'center',
    justifyContent: 'center',
  },
  nestedList: {
    backgroundColor: '#FFA07A',
  },
  item: {
    flexDirection: 'row',
    alignItems: 'center',
    padding: 10,
    gap: 10,
  },
  text: {
    fontSize: 24,
    fontWeight: 'bold',
    color: 'black',
  },
  image: {
    width: 50,
    height: 50,
  },
});

Explanation (copied from here):

I've debugged this for a while now & I have good understanding of what's going on. This bug is caused by our usage of startViewTransition and its implications. We use it well, however React does not account for case that some view might be in transition. Error mechanism is as follows:

1. Let's have initially simple stack with two screens: "A, B". This is component rendered under "B":

    <View //<-- ContainerView (CV)
      removeClippedSubviews={false}
      style={{ flex: 1, backgroundColor: 'slateblue', overflow: 'hidden' }}>
      <View removeClippedSubviews style={{ height: '100%' }}> // <--- IntermediateView (IV)
        <View removeClippedSubviews={false} style={{ backgroundColor: 'pink', width: '100%', height: 50 }} /> // <--- ChildView (ChV)
      </View>
    </View>

2. We press the back button.
3. We're on Fabric, therefore subtree of B gets destroyed before B itself is unmounted -> in our commit hook we detect that the screen B will be unmounted & we mark every node under B as transitioning by calling startViewTransition.
4. React Mounting stage starts, view hierarchy is disassembled in bottom-up fashion (leafs first).
5. ReactViewGroupManager receives MountItem to detach ChV from IV.
6. A call to IV.removeView(ChV) is made, which effectively removes ChV from IV.children, HOWEVER it does not clear ChV.parent, meaning that after the call, ChV.parent == IV. This happens, due to view being marked as in-transition by our call to startViewTransition. If the view is not marked as in-transition this parent-child relationship is removed.
7. IV has removeClippedSubviews enabled, therefore a call to IV.removeViewWithSubviewsClippingEnabled(ChV) is made. This function does effectively two things:
   1. if the ChV has parent (interpretation: it has not yet been detached from parent), we compute it's index in IV.children (Android.ViewGroup's state) and remove it from the array,
   2. remove the ChV from mAllChildren array (this is state maintained by ReactViewGroup for purposes of implementing the "subview clipping" mechanism".

The crash happens in 7.1, because ChV has been removed from IV.children in step 6, but the parent-child relationship has not been broken up there. Under usual circumstances (this is my hypothesis now, yet unconfirmed) 7.1 does not execute, because ChV.parent is nulled in step no. 6.

Rationale for startViewTransition usage

Transitions. On Fabric, when some subtree is unmounted, views in the subtree are unmounted in bottom-up order. This leads to uncomfortable situation, where our components (react-native-screens), who want to drive & manage transitions are notified that their children will be removed after the subtrees mounted in screen subviews are already disassembled. If we start animation in this very moment we will have staggering effect of white flash (issue) (we animate just the screen with white background without it's children). This was not a problem on Paper, because the order of subtree disassembling was opposite - top-down. While we've managed to workaround this issue on Fabric using MountingTransactionObserving protocol on iOS and a commit hook on Android (we can inspect mutations in incoming transaction before it starts being applied) we still need to prevent view hierarchy from being disassembled in the middle of transition (on Paper this has also been less of an issue) - and this is where startViewTransition comes in. It allows us to draw views throughout transition after React Native removes them from HostTree model. On iOS we exchange subtree for its snapshot for transition time, however this approach isn't feasible on Android, because snapshots do not capture shadows.

Possible solutions

Android does not expose a method to verify whether a view is in transition (it has package visibility), therefore we need to retrieve this information with some workaround. I see two posibilities:

• first approach would be to override startViewTransition & endViewTransition in ReactViewGroup and keep the state on whether the view is transitioning there,
• second possible approach would be as follows: we can check for "transitioning" view by checking whether a view has parent but is not it's parent child (this should be reliable),

Having information on whether the view is in transition or not, we can prevent multiple removals of the same view in every call site (currently only in removeViewAt if parent.removeClippingSubviews == true).

Another option would be to do just as this PR does: having in mind this "transitioning" state we can pass a flag to removeViewWithSubviewClippingEnabled and prevent duplicated removal from parent if we already know that this has been requested.

I can also add override of this method:

  /*package*/ void removeViewWithSubviewClippingEnabled(View view) {
    this.removeViewWithSubviewClippingEnabled(view, false);
  }

to make this parameter optional.

Changelog:

[ANDROID] [FIXED] - Handle removal of in-transition views.

Test Plan:

WIP WIP

[tdn120]

This is interesting! It looks like there could certainly be issues with the current code largely ignoring transitions.

The current commit looks a little too simple, since this is the only call to removeViewWithSubviewClippingEnabled() in the library. So we'd effectively never be running the subview clipping logic on view removal.

The "first approach" seems like it should work, as long as the implementation is careful not to hold onto views after they're otherwise removed.

I'm not quite clear on how you envision the second approach - where/how would you be checking parent vs. child?

[kkafar]

@tdn120 thanks for taking a look.

I've incorporated your recent changes: 6a472c5, 0b22b95, 3d1a505 & f1d7016 and made the implementation more general.

We now track whether the view is transitioning or not by overriding the startViewTransition & endViewTransition methods. When removing a child from parent with removeClippedSubviews == true we now check whether the view was clipped: whether it is effectively detached from parent OR removal was requested during transition.

There is an alternative to holding the state - we could write isViewClipped(child) as follows:

return child.getParent() == null || this.indexOfChild(child) == -1;

^^ this could also detect whether the view is transitioning or not.

[kkafar]

I'm using Set<Integer> (holding only the view tag) instead of Set<View> to avoid any retain issues.

[javache]

Consider using androidx.collection.MutableIntSet instead to avoid Integer boxing.

[kkafar]

Thanks, I was not aware of this API! Hover it seems that it has been added few months ago in androidx.collection.MutableIntSet 1.4.0, while my development project (using recent react-native 0.76) uses version 1.1.0. I'm not sure where would I need to bump / add this dependency in RN to make use of it.

[javache]

Looks like this would need to be a new dependency in https://github.com/facebook/react-native/blob/main/packages/react-native/gradle/libs.versions.toml#L45 - let's add a comment to change this in the future.

[kkafar]

@javache I see you working on this code right now: #47937. Maybe you could take a look at this PR in spare moment?

[javache]

Hmm, this is really interesting, and may explain a crash we're seeing. I wonder why we were previously removing this subview twice - potentially messing up some state in ReactViewGroup.

What's your reasoning for removing this?

[kkafar]

@javache thanks for review. My logic was as follows:

before 0b22b95 you relied on handleRemoveView being called to perform side effects regarding child drawing order. Implementation of ReactViewgroup.removeView took care of handling these sideeffects and detaching child view from parent.

On the other hand ReactViewGroup.removeViewWithSubviewClippingEnabled had also two responsibilities:

1. detach child from parent if it has not been detached yet (notice the guard here!)
2. remove child from mAllChildren array, which also retained children already detached due to clipping.

Notice, that it had not performed handleRemoveView.

My theory is that in clipping view manager you had call to parent.removeView(child) to ensure sideeffects of handleRemoveView were performed and then you called parent.removeViewWithSubviewClippingEnabled(child) to remove child from mAllChildren array, relying on the guard from (1), which prevented Android crash potentially caused by removing twice the same child view.

After 0b22b95 side effects of handleRemoveView have been moved to onViewRemoved which is called on any attempt of child removal by the Android framework. Therefore we can remove the call to parent.removeView as all required side effects will be performed by the call to parent.removeViewWithSubviewClippingEnabled(child) - drawing order code, removal from mAllChildren and child detachment. I've left the check for nullish child, as I couldn't determine what was the logic behind it.

[javache]

Consider using androidx.collection.MutableIntSet instead to avoid Integer boxing.

[kkafar]

Thanks for review! I'll update the code in few hours

[facebook-github-bot]

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[tdn120]

Does this account for multiple views/transitions? Seems like we'd need to remove the id(s) instead, and then:
mIsTransitioning = mChildrenRemovedWhileTransitioning.isEmpty();

Also probably worth keeping mChildrenRemovedWhileTransitioning once we create it, we just don't want to create on init, it in cases where transitions aren't used at all.

[kkafar]

I've changed nulling the hashset to clearing it here: a7c7af3.

[kkafar]

@tdn120 I'm not sure though, what you mean by this:

Does this account for multiple views/transitions? Seems like we'd need to remove the id(s) instead [...]

Could you elaborate here?

The ReactViewGroup instance is responsible only of tracking its transitioning state & children. I wrote it so to align it with Android parent.startViewTransition(child) behaviour - it modifies only the parent behaviour with respect to this single child. The child is unaware of any changes.

[tdn120]

Just at a basic level, if you have a timeline of:

startViewTransition(viewA)
startViewTransition(viewB)
endViewTransition(viewA) 

At this point you would clear mIsTransitioning and mChildrenRemovedWhileTransitioning, so wouldn't viewB be out of sync until its transition ends?

[kkafar]

I was OOO by the end of last week. Back here now 👋🏻

I think I still don't understand the issue here.

Android docs explicitly state that a call to startViewTransition should be accompanied by call to endViewTransition. It is left up to the programmer to ensure this.

If you - as a API user - fail to fulfil this contract you'll get inconsistent (possibly buggy) behaviour and I believe there is not much we can do on React Native end to prevent that.

Regarding the example:

this.startViewTransition(viewA)
this.startViewTransition(viewB)
this.endViewTransition(viewA) 

...

Oh, it just got to me. Yeah, I think you're right and that is a very good catch. Thanks. I'll update the code in a bit.

[kkafar]

I should keep track of which children are transitioning and clear the state only if there are no more such children.

[kkafar]

@tdn120 I am thinking about various approaches, trying to avoid allocating additional hash set as much as possible. I would need it to store all the children for which startViewTransition has been called and then remove them from the set in endViewTransition. The working approach would look somehow like that:

  public boolean isChildTransitioning(View child) {
    return mTransitioningChildren != null && mTransitioningChildren.contains(child.getid());
  }

  public void onViewRemoved(View child) {
    ...

    if (isChildTransitioning(child)) {
      // This call would allocate
      ensureChildrenRemovedWhileTransitioning.add(child.getId());
    }

    ...
  }

  public void startViewTransition(View view) {
    super.startViewTransition(view);
    ensureTransitioningChildren().add(view.getId());
  }

  public void endViewTransition(View view) {
    super.endViewTransition(view);
    ensureTransitioningChildren().remove(view.getId());
    if (mChildrenRemovedWhileTransitioning != null) {
      mChildrenRemovedWhileTransitioning.remove(view.getId());
    }
  }

However, this requires 2 hash sets...

Only way to improve I see here is to rely in onViewRemoved on the fact, that if children is transitioning and it has been removed then its parent won't be nulled out.
Hence we could check it like this:

public void onViewRemoved(View child) {
    ...
    
    if (child.getParent() != null) {
        ensureChildrenRemovedWhileTransitioning.add(child.getId());
    }

    ...
}

Looking at Android code (link) this would be reliable, but we would be relaying on implementation detail. I feel like this behaviour is implicitly suggested in startViewTransition & endViewTransition docs, by stating that these remove child from parent state but noting about removing parent from child state, however this is not explicit.

Even if I'm overlooking some case this still would solve the initial issue, so we should be fine on this front.

Another possibility is to store the information on whether the view is transitioning or not directly on the child view itself, by using a tag as you suggested.

Lemme know which behavior you would agree for.

[tdn120]

Just for transparency, I'm also working on an alternative that might handle transitions as a side effect:
#47987

But still iterating, so I don't want to block this in the meantime.

[tdn120]

Latest version lgtm, but I'm going to be landing #47987. If you don't mind, I'll rebase this and update the PR so we can land it afterwards.

[facebook-github-bot]

@tdn120 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[tdn120]

Whoops, I can't update your PR. Rebased and made a couple small tweaks, you should be able to see the changes once it lands.

[facebook-github-bot]

@tdn120 merged this pull request in f402ed1.

[react-native-bot]

This pull request was successfully merged by @kkafar in f402ed1

^{When will my fix make it into a release? | How to file a pick request?}

[kkafar]

Thanks!