fix: handle multiple gesture recognizers

[kkafar]

Description

Fixes #1510

Fixed & improved RNSScreenStackView logic regarding UIGestureRecognizerDelegate protocol.

Changes

• Added gestureRecognizer:shouldReceiveEvent:. Events for particular gesture recognisers are now filtered before gestureRecognizerShouldBegin method is called.
• Any UIScreenEdgePanGestureRecognizer must fail for scroll view gesture recogniser to begin.
• When fullScreenSwipeEnabled == true scroll view gesture recogniser is prioritised before our RNSPanGestureRecognizer.
• Simplified logic in gestureRecognzerShouldBegin.

Now, when adding another gesture recogniser with screen stack as delegate one must make sure that events are properly filtered.

Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

Before

As on screen recordings attached in #1510

After

fullScreenSwipeEnabled: true,
gestureEnabled: true,
stackAnimation: 'simple_push',
customAnimationOnSwipe: true,

Screen.Recording.2022-07-06.at.11.39.02.mov

fullScreenSwipeEnabled: false,
gestureEnabled: true,
stackAnimation: 'simple_push',
customAnimationOnSwipe: false,

Screen.Recording.2022-07-06.at.11.41.16.mov

Test code and steps to reproduce

Test1072

Checklist

• Included code example that can be used to test this change
• Updated TS types
• Updated documentation:
  • https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
  • https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
  • https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
  • https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
• Ensured that CI passes

[hirbod]

@kkafar I would like to take the opportunity to talk about a general issue with multiple recognizers.
@intergalacticspacehighway and I use this patch regular (and adapting) once RNS releases a new version.

https://github.com/intergalacticspacehighway/pager-view-ios-swipe-back-fix/blob/last-page-scroll-fix/patches/react-native-screens+3.13.1.patch

This patch allows us to use https://github.com/callstack/react-native-pager-view and https://github.com/satya164/react-native-tab-view in a way that also Twitter does.

While having fullScreenGestureEnabled: true, we are still able to swipe the tabs and when reached the first tab to also activate the pop gesture (edge + fullscreen pop). This PR also adds a shouldRecognizeSimultaneouslyWithGestureRecognizer which breaks our patch or at least made it more complicated.

Do you think there is any chance to have this somehow supported OOTB by RNS? I understand that RNS would most likely not add ReactNativePageView as a dependency, but maybe you are aware of a more generic way to have this supported.

Here is a video demonstrating what this patch does. Without this patch, fullScreenGestureEnabled would just take over and break swiping. Using only edge swiping, it will even break and make it impossible to swipe back. The edge issue has been fixed lately but fullScreen need this patch.

RPReplay_Final1657015453.mp4

[hirbod]

I pulled in your changes and the bugs I reported are gone. Unfortunately, it did not "fix" the Tab Pager Fullscreen Swiping, so I will need to patch it

[kkafar]

Hi @hirbod
I believe we are not willing to depend on RNPageView, and looking at your patch implementation it seems to me that it is highly related to how page view is implemented.
There is something I've noted down and we will implement at some point in the future: activating the pop gesture when scrollview has offset == 0. It would also allow us to dismiss the screen with scroll view with vertical gesture (swipeDirection == 'vertical'). However I dunno whether it would satisfy you.

[hirbod]

I totally understand that. I was not requesting RNPageView dependency, I was more likely asking if you have an idea of a generic solution, but I just think that it is too custom.

For now we will need to patch this on every release then.

[WoLewicki]

LGTM, but logic gets more complicated with each fix 😅

[hirbod]

I just upgraded to SDK 46 (custom dev client) and it looks like this change messes around with plenty of gestures. Like I can't even close Gorhoms Bottom Sheet anymore because the fullscreenSwipe messes with it. I still have to investigate but it specially has issues with ScrollViews and FlatLists

[hirbod]

False alarm. It was RNGH which caused the issue. We filed an issue there

[timurridjanovic]

There is something I've noted down and we will implement at some point in the future: activating the pop gesture when scrollview has offset == 0. It would also allow us to dismiss the screen with scroll view with vertical gesture (swipeDirection == 'vertical').

@kkafar Is this something that is still planned or has this been solved? I'm trying to figure out how I can swipe away a vertical modal that has a scrollview in it.

[timurridjanovic]

@kkafar Sorry to be annoying with this, but I would like to follow up on my question. I'd just like to know if the feature you mentioned (quoted in my previous comment) is planned in a recent future? Thx

[kkafar]

HI @timurridjanovic,
sorry for the delayed response. This is still something that I have in my TODO list. Best estimation I can give at this moment is: "it will be implemented in near future"... But will definitely notify you when it's done.

[timurridjanovic]

@kkafar Thank you!

[Madumo]

There is something I've noted down and we will implement at some point in the future: activating the pop gesture when scrollview has offset == 0. It would also allow us to dismiss the screen with scroll view with vertical gesture (swipeDirection == 'vertical').

@kkafar do you know if there's any workaround possible to implement this behaviour currently? The "swipe down to dismiss" feature works with those options:

    presentation: 'card',
    animation: 'slide_from_bottom',
    fullScreenGestureEnabled: true,
    customAnimationOnGesture: true,
    gestureDirection: 'vertical',

But i can't make it work when there's a scrollview that swallows the gesture events... Is there any way to propagate those events manually when i know that the scrollOffset is 0? Or do you have any idea on how you would implement this? I might take a crack at it.

[kkafar]

Hey @Madumo
This would surely require native changes. I would start playing around with this method:

- (BOOL)gestureRecognizer:(UIGestureRecognizer *)gestureRecognizer
    shouldRecognizeSimultaneouslyWithGestureRecognizer:(UIGestureRecognizer *)otherGestureRecognizer

and others affected in this PR. I would check whether scollview is attached. If it is the case - check its offset. If it is < / <= than some threshold (0?) allow our gesture recognizer to receive the event (recognise simultaneously with the scroll view gesture recognizer). Then in gesture handling method (I do not recall whether it is currently overrided) trigger screen dismissal if the event comes with y offset > 0.

Any PR is welcome, however as you brought it to my attention I might take crack at it once I finish my current task.

Edit: I would also see this behaviour exposed as separate prop.

[RaphBlanchet]

Hey @kkafar, I work with @Madumo and we looked into fixing this issue. Following your recommendations, we ended up with this patch that seems to be fixing the problem for us: https://gist.github.com/RaphBlanchet/d260376370b39ee5c9ba7e4c45bcd939

We can now drag the screen down to dismiss it, even if the gesture is from the underlying ScrollView. We are even able to keep the bounces of the ScrollView by cancelling the ScrollView's gesture when we detect we are doing a dismiss.

If this patch looks good to you, I will do a PR to include this fix!

Also, regarding the flag you mentioned, I think that checking the already existing fullScreenGestureEnabled and gestureDirection would be more than enough, instead of adding a new one. This is a pretty common behaviour on iOS to dismiss a screen even if we scroll from inside a scroll view, so this patch would be more like a "bug fix" than a new feature. What do you think?

[kkafar]

@RaphBlanchet, took a look at the gist and it looks like it could work. Definitely open up a PR!

Also, regarding the flag you mentioned, I think that checking the already existing fullScreenGestureEnabled and gestureDirection would be more than enough, instead of adding a new one. This is a pretty common behaviour on iOS to dismiss a screen even if we scroll from inside a scroll view, so this patch would be more like a "bug fix" than a new feature. What do you think?

We can think about treating it like that :D But will see when the PR is open & have some time to test it.

[RaphBlanchet]

@kkafar I just opened this PR: #1902

Sorry for the delay, just found some time to do it, hope it helps! :)