Skip to content

Conversation

DorianMazur
Copy link
Contributor

@DorianMazur DorianMazur commented Feb 2, 2025

Description

Partially resolves problem in #3370

This PR fixes an issue where a behind button could stay stuck in the "pressed" state if another overlapping button took the responder. By adding a check in onTouchEvent, we ensure that only one button can stay pressed at a time. If another button is already the responder, the behind button cancels itself and won't remain highlighted.

It will not solve the issue if both button are not exclusive. For this edge case better solution is needed.

With this fix:

withFix.mp4

Without this fix:

withoutFix.mp4

Test plan

https://snack.expo.dev/@oblador/gesture-handler-overlap-reproduction

Collapsed test code
import { Text, View, StyleSheet, ScrollView } from 'react-native';
import {
  BaseButton,
  GestureHandlerRootView,
} from 'react-native-gesture-handler';

export default function App() {
  return (
    <GestureHandlerRootView>
      <View style={styles.container}>
        <BaseButton
          style={styles.button}
          shouldCancelWhenOutside={true}
          onHandlerStateChange={(event) => console.log(event.nativeEvent)}>
          <Text>Behind</Text>
        </BaseButton>
        <BaseButton
          style={[styles.button, styles.onTop]}
          shouldCancelWhenOutside={true}
          onHandlerStateChange={(event) => console.log(event.nativeEvent)}>
          <Text>On top</Text>
        </BaseButton>
      </View>
    </GestureHandlerRootView>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    backgroundColor: '#ecf0f1',
    justifyContent: 'center',
  },
  button: {
    padding: 30,
    borderWidth: 1,
    backgroundColor: '#ffffff99',
  },
  onTop: {
    position: 'relative',
    top: -40,
    left: 100,
    right: 20,
  },
});

Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR!

@@ -291,6 +291,15 @@ class RNGestureHandlerButtonViewManager : ViewGroupManager<ButtonViewGroup>(), R
val eventTime = event.eventTime
val action = event.action

if (touchResponder != null && touchResponder !== this) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be enough?

Suggested change
if (touchResponder != null && touchResponder !== this) {
if (touchResponder !== this) {

Copy link
Contributor Author

@DorianMazur DorianMazur Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-bert Using only if (touchResponder !== this) would also be true when touchResponder is null, so when no other button has grabbed the responder. In that case, we want to proceed normally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right 😅

Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked more into it and it seems that this change breaks buttons with exclusive={false}.

Consider this example:

<GestureHandlerRootView>
  <View style={styles.container}>
    <BaseButton
      exclusive={false}
      style={styles.button}
      shouldCancelWhenOutside={true}
      onHandlerStateChange={(event) => console.log(event.nativeEvent)}>
      <Text>Behind</Text>
    </BaseButton>
    <BaseButton
      exclusive={false}
      style={styles.button}
      shouldCancelWhenOutside={true}
      onHandlerStateChange={(event) => console.log(event.nativeEvent)}>
      <Text>On top</Text>
    </BaseButton>
  </View>
</GestureHandlerRootView>

Now buttons are not stacked on each other and with exclusive set to false you should be able to click both of them at the same time. However, with these changes it is not possible.

@DorianMazur
Copy link
Contributor Author

@m-bert I added a check to verify if the button is exclusive.

@DorianMazur DorianMazur requested a review from m-bert February 3, 2025 15:53
Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, when both buttons have exclusive={false}, this problem is still reproducible 😞

@@ -296,6 +296,17 @@ class RNGestureHandlerButtonViewManager : ViewGroupManager<ButtonViewGroup>(), R
val eventTime = event.eventTime
val action = event.action

if (touchResponder != null && touchResponder !== this) {
if (touchResponder!!.exclusive) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd combine it with outer if to avoid unnecessary nesting

@DorianMazur
Copy link
Contributor Author

@m-bert What do you mean? Your example is working for me

Screen_Recording_20250204_121330_example.mp4

@m-bert
Copy link
Contributor

m-bert commented Feb 4, 2025

@m-bert What do you mean? Your example is working for me

I meant the original one, when the buttons are stacked one on top of the other. The button below gets blocked as in the original issue.

@DorianMazur
Copy link
Contributor Author

DorianMazur commented Feb 4, 2025

@m-bert Yes, that's already happening in the latest version of react-native-gesture-handler 🤷 . If we want to fix this bug for two buttons with exclusive=false, we might need a different solution, but I’m not sure what that would be. Maybe some ray tracing and checking if there is another button on top? That sounds heavy...

My PR already fixes the bug for regular buttons, and the case where two non-exclusive buttons overlap is a rare edge case.

@m-bert
Copy link
Contributor

m-bert commented Feb 4, 2025

@m-bert Yes, that's already happening in the latest version of react-native-gesture-handler 🤷 .

Yes, we are aware. We should find better solution for that. But I agree that it improves current behavior, so I'll give it a green light once You address this comment

Also, You can either remove fixes from description, or open new issue so that we will know that it is still a problem.

If you could do it quickly, we may include this in 2.23.0 (cc @latekvo)

@DorianMazur
Copy link
Contributor Author

@m-bert Thanks, I updated description and pushed new commit

Copy link
Member

@latekvo latekvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, will include it in the 2.23.0 👍

@latekvo latekvo merged commit b696961 into software-mansion:main Feb 4, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants