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

Using same gesture in Gesture.Exclusive causes crash on Android #2739

Closed
jacobmolby opened this issue Feb 2, 2024 · 7 comments · Fixed by #2759
Closed

Using same gesture in Gesture.Exclusive causes crash on Android #2739

jacobmolby opened this issue Feb 2, 2024 · 7 comments · Fixed by #2759
Labels
Platform: Android This issue is specific to Android Repro provided A reproduction with a snack or repo is provided

Comments

@jacobmolby
Copy link

Description

When using the same gesture in Gesture.Exclusive the app crashes on Android in version '~2.14.0'

I have a scenario shown on the screenshot. I want to have a tap target inside a tap target inside a double press target. Only one of the gestures should be activated at any one time.

I started by using blocksExternalGesture between the orange and blue tap gesture to achieve the effect. Which was when I noticed the crash, however it still crashes in the following code without it being in place.

On version ~2.12.0 it doesn't cause a crash, but both "orange and blue" taps are activated at the same time on Android. On iOS only one of "blue" and "orange" are activated at the same time. The behaviour for iOS is the same between ~2.12.0 and ~2.14.0

I don't know what the expected behaviour is, if both "orange" and "blue" should activate, or only "orange" without using blocksExternalGesture. However it is inconsistent between platforms.

I have made the same repro with ~2.12.0 and ~2.14.0. Only ~2.12.0 is a Snack since Snacks doesn't support Expo SDK 50 yet (which has RNGH 2.14.0).

It also occurs in bare workflow, which was how I noticed it (RN 0.72.4 and RNGH 2.14.1)

~2.12.0: https://snack.expo.dev/@jacobmolby/rngh-crash-repro
~2.14.0: https://github.com/jacobmolby/rngh-bug-repro

simulator_screenshot_A7195B2E-6A1A-4790-A04B-B1758FABFC11

Example that causes crash/inconsistent behaviour:

import React from 'react';
import { ScrollView, StyleSheet, Text, View } from 'react-native';
import { Gesture, GestureDetector } from 'react-native-gesture-handler';
import Animated from 'react-native-reanimated';

const Showcase = () => {
  const [lastActions, setLastActions] = React.useState([]);

  const addAction = action => {
    setLastActions(actions => {
      const date = new Date();
      const time = date.toLocaleTimeString();

      if (actions.length >= 3) {
        actions.pop();
      }
      return [action + ': ' + time, ...actions];
    });
  };

  const longPressGesture = Gesture.LongPress()
    .onStart(() => {
      addAction('long press');
    })
    .runOnJS(true);

  const doubleTapGesture = Gesture.Tap()
    .numberOfTaps(2)
    .onStart(() => {
      addAction('double tap');
    })
    .runOnJS(true);

  const gesture = Gesture.Race(doubleTapGesture, longPressGesture);

  return (
    <View
      style={{
        flex: 1,
        backgroundColor: 'white',
        marginHorizontal: 20,
        marginVertical: 60,
      }}>
      <ScrollView contentContainerStyle={{ gap: 20 }}>
        <GestureDetector gesture={gesture}>
          <Animated.View style={styles.outer}>
            <Blue doubleTapGesture={doubleTapGesture} onAddAction={addAction} />
          </Animated.View>
        </GestureDetector>
        <View>
          {lastActions.map((action, index) => (
            <Text key={index}>
              {index} - {action}
            </Text>
          ))}
        </View>
        <View style={{ gap: 20 }}>
          <Text>
            <Bold>DoubleTap</Bold>: on all rectangles - should print "double
            tap" - no tap gesture should be recognized ✅
          </Text>
          <Text>
            <Bold>LongPress</Bold>: on all rectangles - should print "long
            press" - no tap gesture should be recognized ✅
          </Text>
          <Text>
            <Bold>Tap</Bold>: on red - should do nothing ✅
          </Text>
          <Text>
            <Bold>Tap</Bold>: on blue - should print "Blue: tap" ✅
          </Text>
          <Text>
            <Bold>Tap</Bold>: on orange - should print "Orange: tap" ⛔️ -
            Crashes on Android
          </Text>
        </View>
      </ScrollView>
    </View>
  );
};

export default Showcase;

const Blue = ({ doubleTapGesture, onAddAction }) => {
  const tapGesture = Gesture.Tap()
    .onStart(() => {
      onAddAction('Blue: tap');
    })
    .runOnJS(true);

  const composedGesture = Gesture.Exclusive(doubleTapGesture, tapGesture);

  return (
    <GestureDetector gesture={composedGesture}>
      <Animated.View style={styles.blue}>
        <Orange doubleTapGesture={doubleTapGesture} onAddAction={onAddAction} />
      </Animated.View>
    </GestureDetector>
  );
};

const Orange = ({ doubleTapGesture, onAddAction }) => {
  const tapGesture = Gesture.Tap()
    .onStart(() => {
      onAddAction('Orange: tap');
    })
    .runOnJS(true);

  return (
    <GestureDetector gesture={Gesture.Exclusive(doubleTapGesture, tapGesture)}>
      <Animated.View style={styles.orange}></Animated.View>
    </GestureDetector>
  );
};

const Bold = ({ children }) => {
  return <Text style={{ fontWeight: 'bold' }}>{children}</Text>;
};

const styles = StyleSheet.create({
  outer: {
    padding: 20,
    backgroundColor: 'red',
    width: 300,
    height: 200,
  },
  blue: {
    width: '80%',
    height: '80%',
    backgroundColor: 'blue',
    flexDirection: 'row',
    flexWrap: 'wrap',
  },
  orange: {
    margin: 5,
    width: '50%',
    height: '50%',
    backgroundColor: 'orange',
  },
});

When the crash occurs on Android the the stack trace is the following:

FATAL EXCEPTION: main
Process: host.exp.exponent, PID: 8224
java.util.ConcurrentModificationException
	at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1029)
	at java.util.ArrayList$Itr.next(ArrayList.java:982)
	at aq.g.w(SourceFile:35)
	at aq.d.b0(SourceFile:46)
	at aq.d.B(SourceFile:12)
	at aq.y.O0(SourceFile:6)
	at aq.y.M0(SourceFile:1)
	at aq.x.run(SourceFile:1)
	at android.os.Handler.handleCallback(Handler.java:938)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loopOnce(Looper.java:226)
	at android.os.Looper.loop(Looper.java:313)
	at android.app.ActivityThread.main(ActivityThread.java:8663)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:571)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1135)

Steps to reproduce

Put a GestureDetector with Gesture.Exclusive inside another one and watch it crash on android

Snack or a link to a repository

https://github.com/jacobmolby/rngh-bug-repro

Gesture Handler version

2.12.0 and 2.14.0

React Native version

0.72.4 and 0.73.2

Platforms

Android

JavaScript runtime

Hermes

Workflow

Expo managed workflow

Architecture

Paper (Old Architecture)

Build type

Debug mode

Device

Real device

Device model

Crash reproduced on both Galaxy A41 and Galaxy A52 5G

Acknowledgements

Yes

@github-actions github-actions bot added Platform: Android This issue is specific to Android Repro provided A reproduction with a snack or repo is provided labels Feb 2, 2024
@m-bert
Copy link
Contributor

m-bert commented Feb 2, 2024

HI @jacobmolby! Indeed, the app does crash, but it is expected behavior.

One of the assumptions of Gesture Handler is that no Gesture will be used in more than one gesture detector, since it leads to strange behaviors. The problem is that check for this specific case was present only on web, therefore a while ago we decided to unify it across all platforms.

As I can see, your doubleTapGesture is used across 3 different GestureDetectors, violating mentioned principle.

Note that PR that I've mentioned has not been released yet - that's why I got a bit confused at the beginning.

This is how your example looks on current main:

Zrzut ekranu 2024-02-2 o 15 48 52

@jacobmolby
Copy link
Author

Okay, makes sense. Can you tell me how I can achieve the desired gestures then? Is it possible to do with RNGH?

@m-bert
Copy link
Contributor

m-bert commented Feb 7, 2024

I will try to look at it! Seems like using doubleTap in multiple detectors wasn't the only problem. I've manged to find out what caused ConcurrentModificationException and I've already prepared PR to fix that.

Even though it no longer crashes it doesn't seem to work correctly. I've checked that on a commit before we made changes in Orchestrator and it seems like this bug was there for a long time.

I'll get back to you as soon as we find a solution!

@m-bert
Copy link
Contributor

m-bert commented Feb 7, 2024

I went through your example once more, this time focusing more on how it actually works. I believe the thing that you want to use is requireExternalGestureToFail. I've done some changes and double taps seem to work. The only issue that we have to deal with is that both, orange and blue tap, activate when you tap on orange rectangle.

Modified code with requireExternalGestureToFail
import React from 'react';
import { StyleSheet, View } from 'react-native';
import { Gesture, GestureDetector } from 'react-native-gesture-handler';
import Animated from 'react-native-reanimated';

const orangeTapGesture = Gesture.Tap()
  .onStart(() => {
    console.log('Orange: Tap');
  })
  .runOnJS(true);

const orangeDoubleTapGesture = Gesture.Tap()
  .numberOfTaps(2)
  .onStart(() => {
    console.log('Orange: Double tap');
  })
  .runOnJS(true);

const blueTapGesture = Gesture.Tap()
  .onStart(() => {
    console.log('Blue: Tap');
  })
  .runOnJS(true)
  .requireExternalGestureToFail(orangeTapGesture, orangeDoubleTapGesture);

const blueDoubleTapGesture = Gesture.Tap()
  .numberOfTaps(2)
  .onStart(() => {
    console.log('Blue: Double tap');
  })
  .runOnJS(true);

const redLongPressGesture = Gesture.LongPress()
  .onStart(() => {
    console.log('Red: Long press');
  })
  .runOnJS(true);

const redDoubleTapGesture = Gesture.Tap()
  .numberOfTaps(2)
  .onStart(() => {
    console.log('Red: Double tap');
  })
  .runOnJS(true);

const Showcase = () => {
  const gesture = Gesture.Race(redDoubleTapGesture, redLongPressGesture);

  return (
    <View
      style={{
        flex: 1,
        backgroundColor: 'white',
        marginHorizontal: 20,
        marginVertical: 60,
      }}>
      <GestureDetector gesture={gesture}>
        <Animated.View style={styles.outer}>
          <Blue />
        </Animated.View>
      </GestureDetector>
    </View>
  );
};

export default Showcase;

const Blue = () => {
  const composedGesture = Gesture.Exclusive(
    blueDoubleTapGesture,
    blueTapGesture
  );

  return (
    <GestureDetector gesture={composedGesture}>
      <Animated.View style={styles.blue}>
        <Orange />
      </Animated.View>
    </GestureDetector>
  );
};

const Orange = () => {
  const composedGesture = Gesture.Exclusive(
    orangeDoubleTapGesture,
    orangeTapGesture
  );

  return (
    <GestureDetector gesture={composedGesture}>
      <Animated.View style={styles.orange}></Animated.View>
    </GestureDetector>
  );
};

const styles = StyleSheet.create({
  outer: {
    padding: 20,
    backgroundColor: 'red',
    width: 300,
    height: 200,
  },
  blue: {
    width: '80%',
    height: '80%',
    backgroundColor: 'blue',
    flexDirection: 'row',
    flexWrap: 'wrap',
  },
  orange: {
    margin: 5,
    width: '50%',
    height: '50%',
    backgroundColor: 'orange',
  },
});

m-bert added a commit that referenced this issue Feb 9, 2024
## Description

A while ago we decided to remove limit of gesture handlers on android (#2672). We missed one thing - `awaitngHandlers` can be modified while iterating over its items (inside [this loop](https://github.com/software-mansion/react-native-gesture-handler/blob/2e5df1c9a8595929b9879dc1246a7fd78de0549a/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt#L105)). This resulted in `ConcurrentModificationException`. 

This PR changes logic so that now we iterate over copy of `awaitingHandlers`, avoiding modification of collection that we iterate through.

Even though it looks like it changes a lot, the only difference beside adding copy is changing 

```jsx
for(otherHandler in currentlyAwaitingHandlers){
    if (shouldHandlerWaitForOther(otherHandler, handler)) {
        ...
    }
}
```

into

```jsx
for(otherHandler in currentlyAwaitingHandlers){
    if (!shouldHandlerWaitForOther(otherHandler, handler)) {
        continue
    }
    ...
}
```

to remove one level of nested `ifs`.

## Test plan

<details>
<summary> Modified code from #2739 </summary>

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

const Showcase = () => {
  const [lastActions, setLastActions] = React.useState([]);

  const addAction = (action) => {
    setLastActions((actions) => {
      const date = new Date();
      const time = date.toLocaleTimeString();

      if (actions.length >= 3) {
        actions.pop();
      }
      return [action + ': ' + time, ...actions];
    });
  };

  const longPressGesture = Gesture.LongPress()
    .onStart(() => {
      addAction('long press');
    })
    .runOnJS(true);

  const doubleTapGesture = Gesture.Tap()
    .numberOfTaps(2)
    .onStart(() => {
      addAction('double tap');
    })
    .runOnJS(true);

  const gesture = Gesture.Race(doubleTapGesture, longPressGesture);

  return (
    <View
      style={{
        flex: 1,
        backgroundColor: 'white',
        marginHorizontal: 20,
        marginVertical: 60,
      }}>
      <ScrollView contentContainerStyle={{ gap: 20 }}>
        <GestureDetector gesture={gesture}>
          <Animated.View style={styles.outer}>
            <Blue onAddAction={addAction} />
          </Animated.View>
        </GestureDetector>
        <View>
          {lastActions.map((action, index) => (
            <Text key={index}>
              {index} - {action}
            </Text>
          ))}
        </View>
        <View style={{ gap: 20 }}>
          <Text>
            <Bold>DoubleTap</Bold>: on all rectangles - should print "double
            tap" - no tap gesture should be recognized ✅
          </Text>
          <Text>
            <Bold>LongPress</Bold>: on all rectangles - should print "long
            press" - no tap gesture should be recognized ✅
          </Text>
          <Text>
            <Bold>Tap</Bold>: on red - should do nothing ✅
          </Text>
          <Text>
            <Bold>Tap</Bold>: on blue - should print "Blue: tap" ✅
          </Text>
          <Text>
            <Bold>Tap</Bold>: on orange - should print "Orange: tap" ⛔️ -
            Crashes on Android
          </Text>
        </View>
      </ScrollView>
    </View>
  );
};

export default Showcase;

const Blue = ({ onAddAction }) => {
  const tapGesture = Gesture.Tap()
    .onStart(() => {
      onAddAction('Blue: tap');
    })
    .runOnJS(true);
  const doubleTapGesture = Gesture.Tap()
    .numberOfTaps(2)
    .onStart(() => {
      onAddAction('double tap');
    })
    .runOnJS(true);

  const composedGesture = Gesture.Exclusive(doubleTapGesture, tapGesture);

  return (
    <GestureDetector gesture={composedGesture}>
      <Animated.View style={styles.blue}>
        <Orange onAddAction={onAddAction} />
      </Animated.View>
    </GestureDetector>
  );
};

const Orange = ({ onAddAction }) => {
  const tapGesture = Gesture.Tap()
    .onStart(() => {
      onAddAction('Orange: tap');
    })
    .runOnJS(true);

  const doubleTapGesture = Gesture.Tap()
    .numberOfTaps(2)
    .onStart(() => {
      onAddAction('double tap');
    })
    .runOnJS(true);

  return (
    <GestureDetector gesture={Gesture.Exclusive(doubleTapGesture, tapGesture)}>
      <Animated.View style={styles.orange}></Animated.View>
    </GestureDetector>
  );
};

const Bold = ({ children }) => {
  return <Text style={{ fontWeight: 'bold' }}>{children}</Text>;
};

const styles = StyleSheet.create({
  outer: {
    padding: 20,
    backgroundColor: 'red',
    width: 300,
    height: 200,
  },
  blue: {
    width: '80%',
    height: '80%',
    backgroundColor: 'blue',
    flexDirection: 'row',
    flexWrap: 'wrap',
  },
  orange: {
    margin: 5,
    width: '50%',
    height: '50%',
    backgroundColor: 'orange',
  },
});

```

</details>

<details>
<summary> Smaller example </summary>

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

export default function App() {
  const t3 = Gesture.Tap()
    .numberOfTaps(3)
    .onEnd(() => console.log('t3'));
  const t1_1 = Gesture.Tap().onEnd(() => console.log('t1_1'));
  const t1_2 = Gesture.Tap().onEnd(() => console.log('t1_2'));

  const g = Gesture.Exclusive(t3, t1_1, t1_2);

  return (
    <GestureDetector gesture={g}>
      <View style={styles.box} />
    </GestureDetector>
  );
}

const styles = StyleSheet.create({
  box: {
    width: 250,
    height: 250,
    backgroundColor: 'crimson',
  },
});

```
</details>
@m-bert
Copy link
Contributor

m-bert commented Feb 14, 2024

Hi @jacobmolby! I've created this PR that should fix taps activation. Could you please check if it works? It is still a draft so it is possible that something will change, but I'd like to know if it does help in your case.

@jacobmolby
Copy link
Author

Hi, I tried to run your Modified code with requireExternalGestureToFail with the PR installed. It seems to work! 😄

Compared to the original, I guess I'll just have to bubble up the different doubletap gestures, but that should not be too much of an issue. Thanks!

@m-bert
Copy link
Contributor

m-bert commented Feb 15, 2024

That's great! Thanks for submitting this issue, it uncovered some flaws in our logic that we were not aware of!

I will try to merge this PR asap, but we have to discuss something before doing it (i.e. should we do it for all discrete gestures and if so, then how to do it correctly).

m-bert added a commit that referenced this issue Mar 4, 2024
## Description

While working on #2739 we discovered that our orchestration process doesn't work as expected when we have nested `Tap` gestures . This PR introduces few changes that make those gestures work as intended. 

Fixes #2739 

## Test plan

Tested on example app and on modified code from issue.

<details>
<summary> Test code </summary>

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

const log = (e: any, handler: string) =>
  console.log(`[${e.handlerTag}][${handler}]: ${e.state}`);

const orangeTapGesture = Gesture.Tap()
  .runOnJS(true)
  .maxDeltaX(5)
  .onBegin((e) => log(e, 'Orange tap'))
  .onStart((e) => log(e, 'Orange tap'))
  .onFinalize((e, success) => {
    log(e, 'Orange tap');
    if (success) {
      console.log('---> Orange Tap <---');
    }
  });

const orangeDoubleTapGesture = Gesture.Tap()
  .numberOfTaps(2)
  .runOnJS(true)
  .onBegin((e) => log(e, 'Orange Double Tap'))
  .onStart((e) => log(e, 'Orange Double Tap'))
  .onFinalize((e, success) => {
    log(e, 'Orange Double Tap');
    if (success) {
      console.log('---> Orange Double Tap <---');
    }
  });

const blueTapGesture = Gesture.Tap()
  .runOnJS(true)
  .requireExternalGestureToFail(orangeTapGesture, orangeDoubleTapGesture)
  .onBegin((e) => log(e, 'Blue Tap'))
  .onStart((e) => log(e, 'Blue Tap'))
  .onFinalize((e, success) => {
    log(e, 'Blue Tap');
    if (success) {
      console.log('---> Blue Tap <---');
    }
  });

const blueDoubleTapGesture = Gesture.Tap()
  .numberOfTaps(2)
  .runOnJS(true)
  .onBegin((e) => log(e, 'Blue Double Tap'))
  .onStart((e) => log(e, 'Blue Double Tap'))
  .onFinalize((e, success) => {
    log(e, 'Blue Double Tap');
    if (success) {
      console.log('---> Blue Double Tap <---');
    }
  });

const redLongPressGesture = Gesture.LongPress()
  .runOnJS(true)
  .onBegin((e) => log(e, 'Red Longpress'))
  .onStart((e) => log(e, 'Red Longpress'))
  .onFinalize((e, success) => {
    log(e, 'Red Longpress');
    if (success) {
      console.log('---> Red Longpress <---');
    }
  });

const redDoubleTapGesture = Gesture.Tap()
  .numberOfTaps(2)
  .runOnJS(true)
  .onBegin((e) => log(e, 'Red Double Tap'))
  .onStart((e) => log(e, 'Red Double Tap'))
  .onFinalize((e, success) => {
    log(e, 'Red Double Tap');
    if (success) {
      console.log('---> Red Double Tap <---');
    }
  });

const Showcase = () => {
  const gesture = Gesture.Race(redDoubleTapGesture, redLongPressGesture);

  return (
    <View style={[styles.center, { flex: 1, backgroundColor: 'white' }]}>
      <GestureDetector gesture={gesture}>
        <Animated.View style={[styles.outer, styles.center]}>
          <Blue />
        </Animated.View>
      </GestureDetector>
    </View>
  );
};

export default Showcase;

const Blue = () => {
  const composedGesture = Gesture.Exclusive(
    blueDoubleTapGesture,
    blueTapGesture
  );

  return (
    <GestureDetector gesture={composedGesture}>
      <Animated.View style={[styles.blue, styles.center]}>
        <Orange />
      </Animated.View>
    </GestureDetector>
  );
};

const Orange = () => {
  const composedGesture = Gesture.Exclusive(
    orangeDoubleTapGesture,
    orangeTapGesture
  );

  return (
    <GestureDetector gesture={composedGesture}>
      <Animated.View style={styles.orange} />
    </GestureDetector>
  );
};

const styles = StyleSheet.create({
  center: {
    display: 'flex',
    justifyContent: 'space-around',
    alignItems: 'center',
  },

  outer: {
    backgroundColor: 'red',
    width: 400,
    height: 400,
    borderRadius: 200,
  },
  blue: {
    width: 250,
    height: 250,
    backgroundColor: 'blue',

    borderRadius: 175,
  },
  orange: {
    width: 100,
    height: 100,
    backgroundColor: 'orange',
    borderRadius: 50,
  },
});

```

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

Successfully merging a pull request may close this issue.

2 participants