software-mansion / react-native-gesture-handler

Declarative API exposing platform native touch and gesture system to React Native.
https://docs.swmansion.com/react-native-gesture-handler/
MIT License
6.12k stars 979 forks source link

Removing a nested gesture handler causes parent to become unresponsive #2688

Closed computerjazz closed 11 months ago

computerjazz commented 11 months ago

Description

If gesture handlers are nested with simultaneousHandlers enabled, removing the inner handler causes the outer handler to become unresponsive. This is true whether the inner handler is removed while a gesture is in progress or not.

There is a workaround — including myGesture.initialize() in a useEffect fixes it, though I'm not sure why.

In the video below I'm removing the child 1 second after beginning the gesture. In the "broken" case, the gesture immediately becomes unresponsive when the child is removed, and subsequent drags also do not work. In the "fixed" case with the workaround above, everything works as expected:

rngh-broken-nested-handlers

Steps to reproduce

  1. Create a nested component with a gesture handler that has simultaneousHandlers with parent handlers
  2. Render parent handler and child
  3. Remove child
  4. Parent becomes unresponsive
import React, { useEffect, useMemo, useState } from 'react'
import { View, Dimensions, Text, TouchableOpacity } from 'react-native'
import {
  ComposedGesture,
  Gesture,
  GestureDetector,
  GestureHandlerRootView,
  GestureType,
} from 'react-native-gesture-handler'
import Animated, { useAnimatedStyle, useSharedValue } from 'react-native-reanimated'

type SimultaneousGesture = ComposedGesture | GestureType

const PAGE_BUFFER = 4

export default function App() {
  const [isWorkaroundEnabled, setIsWorkaroundEnabled] = useState(false)

  return (
    <GestureHandlerRootView
      style={{
        flex: 1,
        backgroundColor: 'seashell',
        justifyContent: 'center',
      }}
    >
      <DraggableSquare
        key={`draggable-square-${isWorkaroundEnabled}`}
        size={Dimensions.get('window').width}
        depth={0}
        debugTag="root"
        isWorkaroundEnabled={isWorkaroundEnabled}
      />
      <View style={{ position: 'absolute', top: 50, right: 40 }}>
        <Text>{`Gestures are ${isWorkaroundEnabled ? 'working' : 'broken'}`}</Text>
        <TouchableOpacity
          onPress={() => setIsWorkaroundEnabled((prev) => !prev)}
          style={{
            backgroundColor: 'rgba(255, 255, 255, 0.75)',
            borderRadius: 3,
            margin: 8,
            padding: 12,
            justifyContent: 'center',
            alignItems: 'center',
          }}
        >
          <Text>{isWorkaroundEnabled ? 'Break it' : 'Fix it'}</Text>
        </TouchableOpacity>
      </View>
    </GestureHandlerRootView>
  )
}

function DraggableSquare({
  size = 100,
  depth = 0,
  simultaneousGestures = [],
  debugTag = '',
  isWorkaroundEnabled,
}: {
  size?: number
  depth?: number
  simultaneousGestures?: SimultaneousGesture[]
  debugTag?: string
  isWorkaroundEnabled?: boolean
}) {
  const numChildren = depth ? 0 : 1
  const [isLastChildHidden, setIsLastChildHidden] = useState(false)

  const translateX = useSharedValue(0)
  const translateY = useSharedValue(0)
  const prevTransX = useSharedValue(0)
  const prevTransY = useSharedValue(0)

  const animStyle = useAnimatedStyle(() => {
    return {
      transform: [{ translateX: translateX.value }, { translateY: translateY.value }],
      width: size,
      height: size,
      position: 'absolute',
    }
  }, [size])

  const panGesture = useMemo(() => Gesture.Pan(), [])

  panGesture
    .onBegin((evt) => {
      console.log(`${debugTag} begin`)
      setTimeout(() => {
        console.log(`${debugTag} initialize`)
        // Remove the last child during the drag to break gestures
        setIsLastChildHidden(true)
      }, 1000)
    })
    .onUpdate((evt) => {
      translateX.value = prevTransX.value + evt.translationX
      translateY.value = prevTransY.value + evt.translationY
    })
    .onFinalize((evt) => {
      prevTransX.value = prevTransX.value + evt.translationX
      prevTransY.value = prevTransY.value + evt.translationY
      console.log(`${debugTag} finalize`)
    })
    .onTouchesCancelled(() => {
      console.log(`${debugTag} cancel`)
    })

  const allGestures = [panGesture, ...simultaneousGestures]

  useEffect(() => {
    if (isWorkaroundEnabled) {
      // This fixes the issue
      panGesture.initialize()
    }
  }, [isWorkaroundEnabled])

  return (
    <GestureDetector gesture={Gesture.Simultaneous(...allGestures)}>
      <Animated.View style={{ padding: 4 }}>
        <Animated.View style={[animStyle, { backgroundColor: getColor(depth), padding: 4 }]}>
          <Text style={{ color: 'white', fontFamily: 'bold' }}>{debugTag}</Text>

          {[...Array(PAGE_BUFFER)].fill(0).map((_, i) => {
            const isLastChild = i === numChildren - 1
            if (i >= numChildren) {
              return null
            }
            if (isLastChild && isLastChildHidden) {
              // Switching from returning a nested DraggableSquare to
              // returning an empty view breaks gestures
              return <View key={`view-${i}`} />
            }
            return (
              <DraggableSquare
                key={`slider-${i}`}
                debugTag={`[depth:${depth}, index:${i}]`}
                size={size / 2}
                depth={depth + 1}
                simultaneousGestures={allGestures}
              />
            )
          })}
        </Animated.View>
      </Animated.View>
    </GestureDetector>
  )
}

function getColor(i: number) {
  const multiplier = 255 / 10
  const colorVal = Math.abs(i) * multiplier
  return `rgb(${colorVal}, ${Math.abs(128 - colorVal)}, ${255 - colorVal})`
}

Snack or a link to a repository

https://snack.expo.dev/@easydan/removing-nested-handler-bug

Gesture Handler version

2.13.4

React Native version

0.72.3

Platforms

iOS

JavaScript runtime

Hermes

Workflow

Expo managed workflow

Architecture

Paper (Old Architecture)

Build type

Release mode

Device

Real device

Device model

iPhone 15 Pro

Acknowledgements

Yes

m-bert commented 11 months ago

Hi @computerjazz! We've looked into your code and it seems that it uses Gesture Handler in a wrong way.

First of all, if you try to run this code on web, you'll get Handler with tag x already exists error. Native platforms don't check that, but this error message already tells us that something is wrong.

From what we can see, you're using a recursive component that receives gestures from parent as a prop, then you combine them in Gesture.Simultaneous(). The thing is, GestureDetector in child tries to attach handler that already has been used in parent, thus we get error on web. On native part this gesture will be attached, but when child disappears, gestures get cleared - that's why parent becomes unresponsive. This is not a bug though - it is expected behavior.

You can try to use simultaneousWithExternalGesture instead, maybe this will help.

computerjazz commented 11 months ago

Thanks for the quick response @m-bert! I didn't know that a gesture can't/shouldn't be used across multiple GestureDetectors. I tested simultaneousWithExternalGesture and it did fix the issue in my toy example — I'll try plugging it into https://github.com/computerjazz/react-native-infinite-pager, which is where I identified the main issue.

I noticed ComposedGesture is not a valid argument to simultaneousWithExternalGesture — any idea if this is just a type error or if composed gestures cannot be used in this way?

Screenshot 2023-12-06 at 9 09 01 AM

m-bert commented 11 months ago

I'm not sure why you used SimultaneousGesture[] as a type in this prop. I can see that it looks pretty natural given the name of this property, but as far as I can see, you simply pass there array of gestures.

Correct me if I'm wrong, but in parent simultaneousGestures is simply [], and in child it is [pan]. In that case, that is not the array of simultaneous gestures when it comes to types.

Edit: I've missed that SimultaneousGesture is defined at the top. In that case it should be sufficient to change simultaneousGestures?: SimultaneousGesture[] to simultaneousGestures?: GestureType[]

computerjazz commented 11 months ago

In my toy example that would work, but I want to be able to support simultaneous composed gestures in react-native-infinite-pager.

example where I want to wrap the DraggableSquare in a tap gesture that has a composed gesture: https://snack.expo.dev/@easydan/removing-nested-handler-bugfix?platform=ios

It seems like the code works as expected when I use a composed gesture as a simultaneousWithExternalGesture, despite the type error.

m-bert commented 11 months ago

I've just tested that on a small example. Even though it looks like it works in your case, we can't guarantee that it will always be the case. I've prepared small repro to show what I mean:

Example code ```jsx import React from 'react'; import { View, StyleSheet } from 'react-native'; import { GestureDetector, Gesture } from 'react-native-gesture-handler'; export default function App() { const outerPan1 = Gesture.Pan() .activeOffsetX(20) .onChange((e) => console.log(e.handlerTag)); const outerPan2 = Gesture.Pan() .activeOffsetX(-20) .onChange((e) => console.log(e.handlerTag)); const outerPan = Gesture.Simultaneous(outerPan1, outerPan2); const innerPan = Gesture.Pan() .onChange((e) => console.log(e.handlerTag)) .simultaneousWithExternalGesture(outerPan); return ( ); } const styles = StyleSheet.create({ container: { flex: 1, justifyContent: 'space-around', alignItems: 'center', }, box1: { width: 500, height: 500, backgroundColor: 'green', display: 'flex', justifyContent: 'space-around', alignItems: 'center', }, box2: { width: 250, height: 250, backgroundColor: 'blue', }, }); ```

Code above has 2 boxes, outer one has 2 Pan gestures which can work simultaneously. Inner box has one Pan gesture, which can work simultaneously with outer pans.

If you run code above you can see, that panning inside inner box will give logs with only one handler tag - that's why outer ones are not triggered. However, if you change this line:

.simultaneousWithExternalGesture(outerPan);

to this:

.simultaneousWithExternalGesture(...outerPan.toGestureArray());

you can see all handlers tag being logged (given that they all meet activation criteria).

m-bert commented 11 months ago

Hi! I hope everything is clear now.

Given that it was expected behavior, not a problem, I will close this issue. Feel free to re-open it, or submit a new one, if you find something that needs to be addressed.

computerjazz commented 11 months ago

Thanks for all your help @m-bert ! Yes, all clear now, although it might be useful to more clearly document the different ways multiple gestures can be defined (and warn against doing what I did 😅). Maybe also a console warning with info on how to fix if multiple gesture detectors try to register the same gesture?

m-bert commented 11 months ago

Thanks for sharing your thoughts!

I've mentioned it, but I don't know whether you've seen it yourself or not. This is how original code works on web:

image

I will check why native platforms do not perform this check and, if possible, I'll add either error or warning. I will also mention it within our documentation.