Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Feb 10, 2025
Merged

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2ea33d7
Return false if gesture recognizers are present but all disabled
jcolicchio Feb 4, 2025
e18bd81
Move implementation into iOS-specific check, use YES/NO
jcolicchio Feb 4, 2025
497bafe
Tweak implementation to support macos
jcolicchio Feb 4, 2025
17be2e1
Merge branch 'main' into accessibility_gesture_fix
jcolicchio Feb 5, 2025
e902658
Merge branch 'main' into accessibility_gesture_fix
m-bert Feb 10, 2025
80c7cf6
Change comment
m-bert Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions apple/RNGestureHandlerButton.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@m-bert m-bert Feb 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jcolicchio jcolicchio Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@m-bert m-bert Feb 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@m-bert m-bert Feb 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jcolicchio jcolicchio Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@m-bert m-bert Feb 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jcolicchio jcolicchio Feb 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@m-bert m-bert Feb 5, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jcolicchio jcolicchio Feb 5, 2025

Choose a reason for hiding this comment

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

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

// the returned view not actually responding to touches. This in turn breaks button
// 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) {
Copy link
Contributor

@m-bert m-bert Feb 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jcolicchio jcolicchio Feb 4, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@m-bert m-bert Feb 4, 2025

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@jcolicchio jcolicchio Feb 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jcolicchio jcolicchio Feb 4, 2025

Choose a reason for hiding this comment

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

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

if (gestureRecognizer.isEnabled) {
hasEnabledGestureRecognizer = true;
break;
}
}

#if !TARGET_OS_OSX
return [view isKindOfClass:[UIControl class]] || [view.gestureRecognizers count] > 0;
return [view isKindOfClass:[UIControl class]] || hasEnabledGestureRecognizer;
#else
return [view isKindOfClass:[NSControl class]] || [view.gestureRecognizers count] > 0;
return [view isKindOfClass:[NSControl class]] || hasEnabledGestureRecognizer;
#endif
}

Expand Down