Make animated components use different tags for events

[szydlovsky]

Summary

Turns out the usual component tag (used for Layout Animations, Shared Element Transitions and Animated Styles) won't always work for events if the component with nested event emitters is made animated. Thus I am splitting these two into different tag variables.

Test plan

You can check out the following code (logs in console):

Code

import React from 'react';
import Animated, { useAnimatedScrollHandler } from 'react-native-reanimated';
import { StyleSheet, View, Text } from 'react-native';
import { FlashList } from '@shopify/flash-list';

const AnimatedFlashList = Animated.createAnimatedComponent(FlashList);

const data = new Array(45);

export default function EmptyApp() {
  const scrollHandler = useAnimatedScrollHandler(
    {
      onBeginDrag: () => {
        'worklet';
        console.log('ON BEGIN DRAG');
      },
      onScroll: () => {
        'worklet';
        console.log('ON SCROLL');
      },
      onMomentumEnd: () => {
        'worklet';
        console.log('ON END');
      },
    },
    []
  );

  const renderFlashListItem = () => {
    return <View style={styles.itemFlash} />;
  };

  const renderFlatListItem = () => {
    return <View style={styles.itemFlat} />;
  };

  return (
    <View style={styles.container}>
      <View style={styles.listContainer}>
        <Text>Animated FlashList</Text>
        <AnimatedFlashList
          onScroll={scrollHandler}
          renderItem={renderFlashListItem}
          data={data}
          estimatedItemSize={40}
        />
      </View>
      <View style={styles.listContainer}>
        <Text>Animated FlatList</Text>
        <Animated.FlatList
          onScroll={scrollHandler}
          renderItem={renderFlatListItem}
          data={data}
        />
      </View>
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    flexDirection: 'row',
  },
  listContainer: {
    flex: 1,
    flexDirection: 'column',
  },
  itemFlash: {
    width: 20,
    height: 20,
    backgroundColor: 'red',
    margin: 10,
  },
  itemFlat: {
    width: 20,
    height: 20,
    backgroundColor: 'green',
    margin: 10,
  },
});

Current tests work on:

• Paper
• Fabric
• Web

[gronxb]

I haven't checked detail, but gorhom/react-native-bottom-sheet was behaving strangely after applying that patch.

[szydlovsky]

@gronxb if you were able to provide a small example where the react-native-bottom-sheet behaves strangely I’d be able to check it out and make sure it works as well

[gronxb]

I've been busy and haven't prepared a repro, I'll do it as soon as I can.

When calling Pressable onPress from within the bottom sheet, the bottom sheet is forced down.

[szydlovsky]

@gronxb I'm sorry to bother you but we are about to go through with reviewing this PR - do you maybe have time to provide the example of gorhom/react-native-bottom-sheet working strangely with this patch?

[tjzel]

While it's fine for me that we might have two tags, first for events and the second for other operations, I think that we should focus on the "why" here.

1. How comes that we actually need two different tags? Do all operations that require _componentViewTag work properly when _eventViewTag is correct? Or is it a "switch-like" situation?

2. Why didn't we need it before? Which change exactly forces us to make the split here? Or maybe it was always like this and only now we have detected it?

3. What about useScrollViewOffset here? Is it always able to get the tag it actually requires?

We definitely need to try to answer those questions.

[gronxb]

@szydlovsky

Sorry for the delay, here's the reproduction, I think it's the backdrop, if you press anywhere on the bottom sheet, it forces it to go down.

If I don't apply that patch, it only goes down normally when I press backdrop.

It seems like something conflicts and thinks backdrop is pressed even if you press anywhere.

https://github.com/gronxb/reanimated-repro/blob/main/App.tsx

• Apply Patch

2024-05-07.12.02.04.mov

• No Patch

2024-05-07.12.09.12.mov

[szydlovsky]

@tjzel I think all of the questions have answers:

1. How comes that we actually need two different tags? Do all operations that require _componentViewTag work properly when _eventViewTag is correct? Or is it a "switch-like" situation?

The _componentViewTag is the tag of the top-level component, used in all SET's, animated styles and animated props. The _eventViewTag is a tag of the event-emitting component, sometimes is the top-level one, sometimes might be nested or might even not exist (and it won't affect the other tag). We need it for all event handlers. So there are separate scenarios for each.

2. Why didn't we need it before? Which change exactly forces us to make the split here? Or maybe it was always like this and only now we have detected it?

We had it before, but the implementation wasn't well readable and clear. It worked and then was removed in WorkletEventHandler revamp under the assumption that is was redundant. What I'm doing here is reverting it and making it readable so that anyone that stumbles upon it in the future will know why it is this way.

3. What about useScrollViewOffset here? Is it always able to get the tag it actually requires?

Yeah, native implementation needs some changes in this regard. Thanks for pointing that out, I will add it to this PR.

[szydlovsky]

@gronxb alright I found the issue, could you please try out this patch -
react-native-reanimated+3.11.0.patch

[gronxb]

@szydlovsky it works fine 👍

[szydlovsky]

@tjzel @gorhom/bottom-sheets now works fine with the fix AND turns out useScrollViewOffset gets its tags very well for all scenarios (Web/Paper/Fabric) - it didn't need any changes. Therefore it is now reviewable again

[tjzel]

@szydlovsky I assume you tested it natively on iOS only? Android can have a different view hierarchy, please check it too.

Is this PR something that could be verified by runtime tests?

[szydlovsky]

@tjzel I tested it on all: iOS and Android (both Paper and Fabric) and Web. Works fine everywhere. If it comes to runtime tests, we would have to use flash-list and botom-sheets as dependencies so I am not so sure - taking under consideration that this fix is quite highly demanded.

[tjzel]

Oh, that's great then! We can list those as dev dependencies, that's not a problem, but let's not incorporate 3rd party libraries into our runtime tests yet. I will add it as a potential task for the future.

[tjzel]

Looks good to me, but I'd like @piaskowyk or @tomekzaw to take a look at it as well.

[ChadyAyoub4]

@szydlovsky Works perfectly.

[sraut1]

@szydlovsky I am facing this issue #4730.
should this patch fix this one too? is this patch part of 3.11.0 release?

[szydlovsky]

@sraut1 This patch will be in the next release after 3.11. And no, this one most likely doesn't fix the issue since it has nothing to do with animated styles. Nevertheless, feel free to use the patch, maybe it sorts something out.

[sraut1]

@szydlovsky the patch fixes issue #4730. thanks

[szydlovsky]

@sraut1 woah, I'll let the ppl know then 🤯