Skip to content

Commit

Permalink
Fix GestureDetector not working when the underlying view changes (#…
Browse files Browse the repository at this point in the history
…2092)

## Description

`GestureDetector` was not reattaching gestures if the underlying view
has changed, which was especially noticeable when using layout
animations.
This PR updates `GestureDetector` to keep track of the tag of the view
it's attached to and to reattach gestures it the tag changes.

The second commit also fixes gestures not reattaching when manually
changing the underlying view (at the expense of forcing another render),
but only when Reanimated is not used.
Applying the following patch:
<details>
<summary>Expand</summary>

```diff
diff --git a/node_modules/react-native-reanimated/src/createAnimatedComponent.tsx b/node_modules/react-native-reanimated/src/createAnimatedComponent.tsx
index 1cf0c3f..3f22437 100644
--- a/node_modules/react-native-reanimated/src/createAnimatedComponent.tsx
+++ b/node_modules/react-native-reanimated/src/createAnimatedComponent.tsx
@@ -294,19 +294,12 @@ export default function createAnimatedComponent(
       const node = this._getEventViewRef();
       const attached = new Set();
       const nextEvts = new Set();
-      let viewTag: number | undefined;
+      let viewTag: number | undefined =  RNRenderer.findHostInstance_DEPRECATED(this)._nativeTag;
 
       for (const key in this.props) {
         const prop = this.props[key];
         if (prop instanceof AnimatedEvent) {
           nextEvts.add((prop as AnimatedEvent).__nodeID);
-        } else if (
-          has('current', prop) &&
-          prop.current instanceof WorkletEventHandler
-        ) {
-          if (viewTag === undefined) {
-            viewTag = prop.current.viewTag;
-          }
         }
       }
       for (const key in prevProps) {

```

</details>
also makes it work when using Reanimated, but I'm not sure whether it's
fine to change it this way upstream. This needs to be discussed.

## Test plan

Tested on the Example app and on the following code:

<details>
<summary>Expand</summary>

```jsx
import React, { useState } from 'react';
import { Text, View } from 'react-native';
import {
  FlatList,
  Gesture,
  GestureDetector,
} from 'react-native-gesture-handler';
import Animated, { BounceIn } from 'react-native-reanimated';

const items = [
  { name: 'Item A' },
  { name: 'Item B' },
  { name: 'Item C' },
  { name: 'Item D' },
  { name: 'Item A' },
  { name: 'Item B' },
  { name: 'Item C' },
  { name: 'Item D' },
  { name: 'Item A' },
  { name: 'Item B' },
  { name: 'Item C' },
  { name: 'Item D' },
  { name: 'Item A' },
  { name: 'Item B' },
  { name: 'Item C' },
  { name: 'Item D' },
  { name: 'Item A' },
  { name: 'Item B' },
  { name: 'Item C' },
  { name: 'Item D' },
];

function Item() {
  const [faved, setFaved] = useState(false);
  const color = faved ? '#900' : '#aaa';
  const tap = Gesture.Tap()
    .onEnd(() => {
      setFaved(!faved);
    })
    .runOnJS(true);

  return (
    <GestureDetector gesture={tap}>
      <Animated.View
        key={color}
        entering={BounceIn}
        style={{ backgroundColor: color, width: 30, height: 30 }}
      />
    </GestureDetector>
  );
}

function renderItem({ item }: { item: { name: string } }) {
  return (
    <View
      style={{
        width: '100%',
        height: 50,
        backgroundColor: 'red',
        flexDirection: 'row',
        justifyContent: 'space-between',
        padding: 10,
        alignItems: 'center',
      }}>
      <Text>{item.name}</Text>
      <Item />
    </View>
  );
}

export default function Example() {
  return (
    <View style={{ flex: 1 }}>
      <FlatList style={{ flex: 1 }} data={items} renderItem={renderItem} />
    </View>
  );
}

```

</details>

Code to test the second commit:

<details>
<summary>Expand</summary>

```jsx
import React from 'react';
import { View } from 'react-native';
import { Gesture, GestureDetector } from 'react-native-gesture-handler';
import Animated from 'react-native-reanimated';

function Item() {
  console.log('render item');
  return (
    <Animated.View
      style={{
        alignSelf: 'center',
        width: 200,
        height: 200,
        backgroundColor: 'red',
      }}
    />
  );
}

export default function Example() {
  const gesture = Gesture.Tap()
    .onStart(() => {
      console.log('a', _WORKLET);
    })
    .runOnJS(true);
  console.log('render parent');
  return (
    <View style={{ flex: 1 }}>
      <GestureDetector gesture={gesture}>
        <Item />
      </GestureDetector>
    </View>
  );
}
```

Change between `View` and `Animated.View` while the app is running and
check if the tap still works. Remove `.runOnJS(true)` to test using
Reanimated.

</details>
  • Loading branch information
j-piasecki committed Jan 9, 2023
1 parent 57761e4 commit 8d114ae
Showing 1 changed file with 72 additions and 29 deletions.
101 changes: 72 additions & 29 deletions src/handlers/gestures/GestureDetector.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useEffect, useRef, RefObject } from 'react';
import React, { useEffect, useRef, useState } from 'react';
import {
GestureType,
HandlerCallbacks,
Expand Down Expand Up @@ -238,7 +238,7 @@ function updateHandlers(
preparedGesture: GestureConfigReference,
gestureConfig: ComposedGesture | GestureType,
gesture: GestureType[],
mountedRef: RefObject<boolean>
mountedRef: React.RefObject<boolean>
) {
gestureConfig.prepare();

Expand Down Expand Up @@ -596,6 +596,12 @@ interface GestureDetectorProps {
userSelect?: UserSelect;
children?: React.ReactNode;
}
interface GestureDetectorState {
firstRender: boolean;
viewRef: React.Component | null;
previousViewTag: number;
forceReattach: boolean;
}
export const GestureDetector = (props: GestureDetectorProps) => {
const gestureConfig = props.gesture;

Expand All @@ -605,8 +611,14 @@ export const GestureDetector = (props: GestureDetectorProps) => {

const gesture = gestureConfig.toGestureArray();
const useReanimatedHook = gesture.some((g) => g.shouldUseReanimated);
const viewRef = useRef(null);
const firstRenderRef = useRef(true);

// store state in ref to prevent unnecessary renders
const state = useRef<GestureDetectorState>({
firstRender: true,
viewRef: null,
previousViewTag: -1,
forceReattach: false,
}).current;
const mountedRef = useRef(false);
const webEventHandlersRef = useRef<WebEventHandler>({
onGestureHandlerEvent: (e: HandlerStateChangeEvent<unknown>) => {
Expand All @@ -619,6 +631,11 @@ export const GestureDetector = (props: GestureDetectorProps) => {
: undefined,
});

const [renderState, setRenderState] = useState(false);
function forceRender() {
setRenderState(!renderState);
}

const preparedGesture = React.useRef<GestureConfigReference>({
config: gesture,
animatedEventHandler: null,
Expand All @@ -635,10 +652,41 @@ export const GestureDetector = (props: GestureDetectorProps) => {
);
}

function onHandlersUpdate(skipConfigUpdate?: boolean) {
// if the underlying view has changed we need to reattach handlers to the new view
const viewTag = findNodeHandle(state.viewRef) as number;
const forceReattach = viewTag !== state.previousViewTag;

if (forceReattach || needsToReattach(preparedGesture, gesture)) {
validateDetectorChildren(state.viewRef);
dropHandlers(preparedGesture);
attachHandlers({
preparedGesture,
gestureConfig,
gesture,
webEventHandlersRef,
viewTag,
mountedRef,
});

state.previousViewTag = viewTag;
state.forceReattach = forceReattach;
if (forceReattach) {
forceRender();
}
} else if (!skipConfigUpdate) {
updateHandlers(preparedGesture, gestureConfig, gesture, mountedRef);
}
}

// Reanimated event should be rebuilt only when gestures are reattached, otherwise
// config update will be enough as all necessary items are stored in shared values anyway
const needsToRebuildReanimatedEvent =
preparedGesture.firstExecution || needsToReattach(preparedGesture, gesture);
preparedGesture.firstExecution ||
needsToReattach(preparedGesture, gesture) ||
state.forceReattach;

state.forceReattach = false;

if (preparedGesture.firstExecution) {
gestureConfig.initialize();
Expand All @@ -651,17 +699,18 @@ export const GestureDetector = (props: GestureDetectorProps) => {
}

useEffect(() => {
firstRenderRef.current = true;
const viewTag = findNodeHandle(state.viewRef) as number;
state.firstRender = true;
mountedRef.current = true;
const viewTag = findNodeHandle(viewRef.current) as number;

validateDetectorChildren(viewRef.current);
validateDetectorChildren(state.viewRef);

attachHandlers({
preparedGesture,
gestureConfig,
gesture,
viewTag,
webEventHandlersRef,
viewTag,
mountedRef,
});

Expand All @@ -672,32 +721,26 @@ export const GestureDetector = (props: GestureDetectorProps) => {
}, []);

useEffect(() => {
if (!firstRenderRef.current) {
const viewTag = findNodeHandle(viewRef.current) as number;

if (needsToReattach(preparedGesture, gesture)) {
validateDetectorChildren(viewRef.current);
dropHandlers(preparedGesture);
attachHandlers({
preparedGesture,
gestureConfig,
gesture,
viewTag,
webEventHandlersRef,
mountedRef,
});
} else {
updateHandlers(preparedGesture, gestureConfig, gesture, mountedRef);
}
if (!state.firstRender) {
onHandlersUpdate();
} else {
firstRenderRef.current = false;
state.firstRender = false;
}
}, [props]);

const refFunction = (ref: unknown) => {
if (ref !== null) {
//@ts-ignore Just setting the ref
viewRef.current = ref;
// @ts-ignore Just setting the view ref
state.viewRef = ref;

// if it's the first render, also set the previousViewTag to prevent reattaching gestures when not needed
if (state.previousViewTag === -1) {
state.previousViewTag = findNodeHandle(state.viewRef) as number;
}

// pass true as `skipConfigUpdate`, here we only want to trigger the eventual reattaching of handlers
// in case the view has changed, while config update would be handled be the `useEffect` above
onHandlersUpdate(true);

if (isFabric()) {
const node = getShadowNodeFromRef(ref);
Expand Down

0 comments on commit 8d114ae

Please sign in to comment.