Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
computerjazz opened this issue Dec 5, 2023 · 8 comments
Closed
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided

Comments

@computerjazz
Copy link
Contributor

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

@github-actions github-actions bot added Repro provided A reproduction with a snack or repo is provided Platform: iOS This issue is specific to iOS labels Dec 5, 2023
@m-bert
Copy link
Contributor

m-bert commented Dec 6, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

m-bert commented Dec 7, 2023

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
Copy link
Contributor Author

computerjazz commented Dec 7, 2023

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
Copy link
Contributor

m-bert commented Dec 11, 2023

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
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 (
    <View style={styles.container}>
      <GestureDetector gesture={outerPan}>
        <View style={styles.box1}>
          <GestureDetector gesture={innerPan}>
            <View style={styles.box2} />
          </GestureDetector>
        </View>
      </GestureDetector>
    </View>
  );
}

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
Copy link
Contributor

m-bert commented Dec 14, 2023

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.

@m-bert m-bert closed this as completed Dec 14, 2023
@computerjazz
Copy link
Contributor Author

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
Copy link
Contributor

m-bert commented Dec 14, 2023

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.

m-bert added a commit that referenced this issue Dec 19, 2023
…stureDetectors` (#2694)

## Description

Some of our users incorrectly use gesture handlers - they pass the same gesture handler instance into multiple `GestureDetectors`. It very often leads to unexpected behavior, like described in #2688.

Currently web version of our library throws error `Handler with tag x already exists`. However, there are 2 problems with that:

1. This error message is not really helpful in case of fixing that problem.
2. Native platforms do not perform this check, so people don't know that they're using our handlers in a wrong way.

This PR:
- Improves error message
- Adds check on native platforms
- Adds information about error in `GestureDetector` documentation in remarks section

## Test plan

Tested with code below on all platforms.

<details>
<summary>Code that throws error</summary>

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

  export default function Example() {
    const pan = Gesture.Pan();

    return (
      <View>
        <GestureDetector gesture={pan}>
          <View>
            <GestureDetector gesture={pan}>
              <View />
            </GestureDetector>
          </View>
        </GestureDetector>
      </View>
    );
  }
```

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided
Projects
None yet
Development

No branches or pull requests

2 participants