From 8d114aedba6568ea13dc0eba22c763d0bb579a3c Mon Sep 17 00:00:00 2001 From: Jakub Piasecki Date: Mon, 9 Jan 2023 11:38:31 +0100 Subject: [PATCH] Fix `GestureDetector` not working when the underlying view changes (#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:
Expand ```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) { ```
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:
Expand ```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 ( ); } function renderItem({ item }: { item: { name: string } }) { return ( {item.name} ); } export default function Example() { return ( ); } ```
Code to test the second commit:
Expand ```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 ( ); } export default function Example() { const gesture = Gesture.Tap() .onStart(() => { console.log('a', _WORKLET); }) .runOnJS(true); console.log('render parent'); return ( ); } ``` 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.
--- src/handlers/gestures/GestureDetector.tsx | 101 +++++++++++++++------- 1 file changed, 72 insertions(+), 29 deletions(-) diff --git a/src/handlers/gestures/GestureDetector.tsx b/src/handlers/gestures/GestureDetector.tsx index c9d4681095..1be80d76e7 100644 --- a/src/handlers/gestures/GestureDetector.tsx +++ b/src/handlers/gestures/GestureDetector.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useRef, RefObject } from 'react'; +import React, { useEffect, useRef, useState } from 'react'; import { GestureType, HandlerCallbacks, @@ -238,7 +238,7 @@ function updateHandlers( preparedGesture: GestureConfigReference, gestureConfig: ComposedGesture | GestureType, gesture: GestureType[], - mountedRef: RefObject + mountedRef: React.RefObject ) { gestureConfig.prepare(); @@ -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; @@ -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({ + firstRender: true, + viewRef: null, + previousViewTag: -1, + forceReattach: false, + }).current; const mountedRef = useRef(false); const webEventHandlersRef = useRef({ onGestureHandlerEvent: (e: HandlerStateChangeEvent) => { @@ -619,6 +631,11 @@ export const GestureDetector = (props: GestureDetectorProps) => { : undefined, }); + const [renderState, setRenderState] = useState(false); + function forceRender() { + setRenderState(!renderState); + } + const preparedGesture = React.useRef({ config: gesture, animatedEventHandler: null, @@ -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(); @@ -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, }); @@ -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);