Return false if gesture recognizers are present but all disabled #3377

jcolicchio · 2025-02-04T00:47:43Z

Description

This PR adjusts shouldHandleTouch to check for enabled gesture recognizers instead of returning true if the count of gesture recognizers is greater than zero.

The motivation behind this PR is to address a bug where buttons become unresponsive if, I guess, iOS is inserting disabled accessibility gesture recognizers into the child views??

Fixes #3376

Test plan

I was able to reproduce this reliably on a private repo, and used the debugger to observe shouldHandleTouch returning too early from a descendant of the button meant to be tapped. With this fix, I confirmed that the correct button returns to handle to touch event and the buttons all behave as expected

I also was able to reproduce the issue with this sample code:

Tap Courses
Tap Hello World 1
Tap All Courses

The All Courses button has an onPress which alerts, and in this snippet, I observed no alert occurring

Click to expand

import { StyleSheet, View, Text } from 'react-native';
import { BorderlessButton, FlatList } from 'react-native-gesture-handler';
import Animated from 'react-native-reanimated';
import { useNavigation } from 'expo-router';
import { useState } from 'react';
import { useSafeAreaInsets } from 'react-native-safe-area-context';
import {useEffect} from 'react';
import {
    FadeInLeft,
    FadeInUp,
    FadeOutLeft,
    FadeOutUp,
} from 'react-native-reanimated';

const AnimatedBorderlessButton = Animated.createAnimatedComponent(BorderlessButton);

const OptionsScreen = ({
    options,
    onSelect
}: {
    options: string[];
    onSelect: (option?: string) => void;
}) => {
    const insets = useSafeAreaInsets();
    const navigation = useNavigation();
    const styles = StyleSheet.create({
        container: {
          paddingTop: insets.top,
            backgroundColor: 'rgba(0,0,0,0.7)',
            flex: 1,
            paddingHorizontal: 24,
            position: 'absolute',
            top: 0,
            left: 0,
            right: 0,
            bottom: 0,
        },
        text: {
            paddingTop: 16,
            color: 'white',
        },
    });
    useEffect(() => {
        navigation.setOptions({
          tabBarStyle: { display: 'none' },
        });
        return () => {
          navigation.setOptions({
            tabBarStyle: { display: 'flex' },
          });
        };
    }, [navigation]);
    return (
        <Animated.View
            style={styles.container}
            entering={FadeInUp}
            exiting={FadeOutUp}
        >
            <FlatList
                data={options}
                renderItem={({item}) => (
                    <AnimatedBorderlessButton onPress={() => onSelect(item)}>
                      <View accessible accessibilityRole="button">
                        <Text style={styles.text}>{item}</Text>
                      </View>
                    </AnimatedBorderlessButton>
                )}
            />
        </Animated.View>
    );
};

function HomeScreen() {
    const insets = useSafeAreaInsets();
    const [selectedCategory, setSelectedCategory] = useState('');
    const [options, setOptions] = useState<string[]>([]);

    const styles = StyleSheet.create({
        container: {
            flex: 1,
            paddingTop: insets.top,
            backgroundColor: 'white',
        },
        categoryContainer: {
            gap: 16,
            paddingHorizontal: 24,
        },
    });
    const dummyData = {
        categoryOptions: (Array.from({length: 14}, (_, i) => `Hello World ${i + 1}`)),
    };

    return (
        <View style={styles.container}>
            <AnimatedBorderlessButton
              onPress={() => setOptions(dummyData.categoryOptions)}
              entering={FadeInLeft}
              exiting={FadeOutLeft}
            >
                <View accessible accessibilityRole="button">
                    <Text>{selectedCategory.length > 0 ? selectedCategory : 'Courses'}</Text>
                </View>
            </AnimatedBorderlessButton>
            {selectedCategory.length > 0 && (
                <AnimatedBorderlessButton
                    onPress={() => alert('It Worked')}
                    entering={FadeInLeft}
                    exiting={FadeOutLeft}
                >
                  <View accessible accessibilityRole="button">
                      <Text>All Courses</Text>
                  </View>
                </AnimatedBorderlessButton>
            )}
            {options.length > 0 && (
                <OptionsScreen
                    options={options}
                    onSelect={(selectedOption) => {
                        setSelectedCategory(selectedOption ?? '');
                        setOptions([]);
                    }}
                />
            )}
        </View>
    );
}

export default function TabTwoScreen() {
  return (
    <HomeScreen />
  );
}

m-bert

Hi @jcolicchio! Thanks for submitting this PR! I have some remarks though:

Let's use YES and NO instead of true/false. I'm also not a fan of this, but for now I'd like to stay consistent with our codebase.
I think that this PR will break macos.
Before we merge it, we have to test those changes, so please provide a reproduction that we can use

m-bert · 2025-02-04T09:11:09Z

apple/RNGestureHandlerButton.mm

+  // touch handling. Therefore, as a bandaid we can check to ensure we don't return
+  // true here if the only gesture recognizers we have are all disabled.
+  BOOL hasEnabledGestureRecognizer = false;
+  for (UIGestureRecognizer *gestureRecognizer in view.gestureRecognizers) {


I'm pretty sure that this will break on macos 😅

Should I go ahead and move this implementation into #if !TARGET_OS_OSX for the sake of this fix, which may be an iOS-specific issue to begin with?

Actually I would prefer something like this:

#if !TARGET_OS_OSX for (UIGestureRecognizer *gestureRecognizer in view.gestureRecognizers) { #else for (NSGestureRecognizer *gestureRecognizer in view.gestureRecognizers) { #endif

Now when I checked this macos did compile without issues, but I'm not sure why it does recognize UIGestureRecognizer if it has its own NSGestureRecognizer, so I believe it is better to split it into two parts. Also making it iOS exclusive may not be the best as it may also occur on macos (but for that we would need a reproduction)

Got it, I'll take a look at such an implementation tomorrow

I actually went with an implementation using NSPredicate to filter on isEnabled == YES, which I believe should work equally well on OSX as iOS? I don't have a good test setup to validate on OSX though, so please take a look and let me know if it works alright.

I also updated the comment, and confirmed that my snippet indeed produces the bug on 2.22.0 (Gestures' Text was unfortunately a non-starter), and that this proposed change addresses the bug using Xcode's debugger, so I think this PR should be good to go! Requesting re-review

m-bert · 2025-02-04T09:13:25Z

apple/RNGestureHandlerButton.mm

@@ -64,10 +64,24 @@ - (BOOL)shouldHandleTouch:(RNGHUIView *)view
    return button.userEnabled;
  }

+  // Certain subviews such as RCTViewComponentView have been observed to have disabled
+  // accessibility gesture recognizers such as _UIAccessibilityHUDGateGestureRecognizer,
+  // ostensibly set by iOS. Such gesture recognizers cause us to return early, despite


I'm not sure if "return early" is correct in that case

I think it is incorrect. In my case, the handler for a touch returned by this algorithm was the native representation of <View accessible accessibilityRole="button"><Text>Static Text</Text></View>. It didn't have any valid tap gesture recognizers to handle touch events, but it was still chosen by this algorithm to be the view which "shouldHandleTouch"

If the algorithm does not return this child view "early", it instead traverses up to the parent RNGestureHandlerButton and returns the parent view, which does have gesture handlers due to, in this case, my BorderlessButton's onPress, which faithfully gets invoked on tap

So, I think this algorithm should not return a child view which happens to have two disabled accessibility gesture recognizers, which were mysteriously added from some unknown source, just because the number of gesture recognizers is greater than zero. Instead, I think it should ignore these disabled gesture recognizers when determining which view should handle touch, so that the parent RNGestureHandlerButton can instead be returned and handle touches

I see what you mean. What I meant was that the way it is phrased in this comment suggests something like early return in this specific function, not in the whole process.

(...) view which happens to have two disabled accessibility gesture recognizers, which were mysteriously added from some unknown source (...)

My guess is that they are added because of View with accessibilityRole="button", but I have not checked that yet.

Anyway, I think that we should rephrase this comment when we have more information

Also, do you use Text from React Native, or from Gesture Handler?

I'll rephrase the comment to clarify. I find it odd that only in certain scenarios do certain Views get these accessibility tap gestures added to them

And, I'm using Text from React Native, how would I import Text from Gesture Handler? I'm seeing Module '"react-native-gesture-handler"' has no exported member 'Text' when I try

As far as I remember Text component was introduced form 2.22. I asked because that could explain why those views have additional gesture recognizers.

I've checked example with only View with props accessible and accessibilityRole="button" and it seems that it doesn't have any additional recognizer by default

Hmm... Using Text from RNGH 2.22 actually causes the button to not be tappable at all, as in the following:

<AnimatedBorderlessButton onPress={() => onSelect(item)}> <View accessible accessibilityRole="button"> <Text style={styles.text}>{item}</Text> </View> </AnimatedBorderlessButton>

According to the debugger, the RCTParagraphTextView has a RNDummyGestureRecognizer initially, even before taking the steps which produce the bug in React Native's Text

According to the debugger, the RCTParagraphTextView has a RNDummyGestureRecognizer initially, even before taking the steps which produce the bug in React Native's Text

That's exactly what I meant by:

I asked because that could explain why those views have additional gesture recognizers.

Got it — I’d thought you might have been suggesting that NOT using that particular Text might explain why I was seeing additional gesture recognizers

jcolicchio · 2025-02-04T09:55:31Z

Hi @jcolicchio! Thanks for submitting this PR! I have some remarks though:

Let's use YES and NO instead of true/false. I'm also not a fan of this, but for now I'd like to stay consistent with our codebase.

I think that this PR will break macos.

Before we merge it, we have to test those changes, so please provide a reproduction that we can use

Got it, I'll make that change
I'll move the fix into the iOS-specific check, if that sounds ok?
I had trouble reliably reproducing this bug from scratch, it involved an amount of clicking to animate buttons in and out. Still, tomorrow I'll clean up the code I'm working with and see if the issue easily persists in a vanilla project. For what it's worth, my forked branch completely addresses the issue 👍

m-bert · 2025-02-05T08:54:48Z

The code looks ok, have you been able to prepare a reproduction? It would be great to check what causes this issue 😅

jcolicchio · 2025-02-05T16:49:17Z

The code looks ok, have you been able to prepare a reproduction? It would be great to check what causes this issue 😅

I added it to the description of the PR, at the bottom inside of Click to expand!

m-bert

I've just tested that on the repro that you've provided and it indeed works 😅

What's interesting, it does work inside our example app without this fix. I thought it may be related to React old/new architectures, but it seems that it is not a case.

Also, I took the liberty of slighgtly changing comment, hope you don't mind 😅

Return false if gesture recognizers are present but all disabled

2ea33d7

m-bert requested changes Feb 4, 2025

View reviewed changes

jcolicchio added 2 commits February 4, 2025 00:02

Move implementation into iOS-specific check, use YES/NO

e18bd81

Tweak implementation to support macos

497bafe

jcolicchio requested a review from m-bert February 4, 2025 20:29

jcolicchio and others added 3 commits February 5, 2025 06:49

Merge branch 'main' into accessibility_gesture_fix

17be2e1

Merge branch 'main' into accessibility_gesture_fix

e902658

Change comment

80c7cf6

m-bert approved these changes Feb 10, 2025

View reviewed changes

m-bert merged commit 7c3e002 into software-mansion:main Feb 10, 2025
3 checks passed

Uh oh!

Return false if gesture recognizers are present but all disabled #3377

Return false if gesture recognizers are present but all disabled #3377

Uh oh!

Conversation

jcolicchio commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test plan

Uh oh!

m-bert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcolicchio Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcolicchio Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcolicchio commented Feb 4, 2025

Uh oh!

m-bert commented Feb 5, 2025

Uh oh!

jcolicchio commented Feb 5, 2025

Uh oh!

m-bert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jcolicchio commented Feb 4, 2025 •

edited

Loading

jcolicchio Feb 4, 2025 •

edited

Loading

jcolicchio Feb 4, 2025 •

edited

Loading