software-mansion / react-native-gesture-handler

Declarative API exposing platform native touch and gesture system to React Native.
https://docs.swmansion.com/react-native-gesture-handler/
MIT License
5.85k stars 954 forks source link

Don't call deprecated `[RCTRootView cancelTouches]` #657

Closed janicduplessis closed 4 years ago

janicduplessis commented 4 years ago

According to the message in the commit it should not be needed anymore https://github.com/facebook/react-native/commit/36307d87e1974aff1abac598da2fd11c4e8e23c1. Not sure if we have a good test case for this to make sure it doesn't break but I've used it in my app for a while and everything seems fine (although I don't really mix RN and RNGH gesture components).

I kept didActivateInRootView empty since it is needed by the protocol but it might be possible to remove it from the protocol also.

shergin commented 4 years ago

đź‘Ť

kmagiera commented 4 years ago

Is this change only compatible with a specific version of RN?

The simplest test case to verify this works correctly is:

janicduplessis commented 4 years ago

@kmagiera According to the comment by @shergin the need for calling this was removed in 2017 so I assume this is fine compatibility wise.

I can try testing this case.

janicduplessis commented 4 years ago

Yea I can confirm this breaks the case you mentionned

<PanGestureHandler
      onHandlerStateChange={e => {
        console.warn(e.nativeEvent.state);
      }}
    >
      <View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}>
        <TouchableOpacity
          onPress={() => {}}
          style={{ backgroundColor: 'blue', width: 200, height: 100 }}
        />
      </View>
    </PanGestureHandler>

Not sure what the best way forward would be, @shergin do you know why this api was deprecated?

kmagiera commented 4 years ago

I also tried this change and I'm afraid calling cancelTouches is in fact necessary. I'd suggest that we change core to remove deprecated annotation for that method as it is needed for the cases when touch stream needs to be cancelled as some other gesture recognizer takes over (note that on IOS gesture recognizers does not get notified if some other gesture recognizer takes over the touch stream)

dan-lee commented 4 years ago

How can this be deprecated if it's still needed? Sorry if I am ignorant, but I don't get what the actual problem is and where it needs to be fixed

jonstuebe commented 4 years ago

is there a PR open on react-native core that removes the deprecation? My app is crashing due to this right now (in a release build on a real device).

IjzerenHein commented 4 years ago

is there a PR open on react-native core that removes the deprecation? My app is crashing due to this right now (in a release build on a real device).

Wow. On iOS or Android? @jonstuebe

3luyka commented 4 years ago

@IjzerenHein same here. Crashes on iOS (don't tested yet on Android).

pouria-bashar commented 4 years ago

Same here!!

gomestai commented 4 years ago

same here!

phantom1299 commented 4 years ago

I had crashes on both android and ios release builds. Fixed for android by adding import 'react-native-gesture-handler' on top of index.js. Going to try it for ios now.

Source

rodrigowpl commented 4 years ago

this was already fixed? Still happing here when focus on any TextInput.

Panthom solution above doesn't work here

System:
    OS: macOS Mojave 10.14.6
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
    Memory: 2.13 GB / 16.00 GB
    Shell: 5.3 - /bin/zsh
  Binaries:
    Node: 8.12.0 - /usr/local/bin/node
    Yarn: 1.17.3 - ~/.yarn/bin/yarn
    npm: 6.4.1 - /usr/local/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  SDKs:
    iOS SDK:
      Platforms: iOS 12.4, macOS 10.14, tvOS 12.4, watchOS 5.3
    Android SDK:
      API Levels: 23, 25, 26, 27, 28
      Build Tools: 23.0.1, 23.0.2, 25.0.0, 25.0.2, 25.0.3, 26.0.1, 26.0.2, 26.0.3, 27.0.3, 28.0.2, 28.0.3
      System Images: android-25 | Google APIs Intel x86 Atom, android-26 | Google APIs Intel x86 Atom_64, android-26 | Google Play Intel x86 Atom, android-27 | Google APIs Intel x86 Atom
  IDEs:
    Android Studio: 3.4 AI-183.6156.11.34.5522156
    Xcode: 10.3/10G8 - /usr/bin/xcodebuild
  npmPackages:
    react: 16.9.0 => 16.9.0 
    react-native: 0.61.2 => 0.61.2 
  npmGlobalPackages:
    react-native-cli: 2.0.1
    react-native-git-upgrade: 0.2.7
kristijantomic commented 4 years ago

I see that the warning will not be triggered if you put multiline=true TextInput property (iOS). RN 61

Andriiklymiuk commented 4 years ago

This error crashes in production. This is terrible. Has anyone fixed it?

phantom1299 commented 4 years ago

@Andriiklymiuk did you try adding import 'react-native-gesture-handler' on top of index.js

Andriiklymiuk commented 4 years ago

@phantom1299 somehow on the 3rd try it worked. Really strange fix, but thanks)

swingywc commented 4 years ago

Is there any solution right now? If Facebook isn't going to add back the cancelTouches function, will this package use an alternative solution for this error? Thanks.

AbanoubNassem commented 4 years ago

release build on IOS crashes , anywork around ?

wesleynfreitas commented 4 years ago

I try put

1 - import 'react-native-gesture-handler' at the top of index.js 2 - YellowBox.ignoreWarnings([ 'RCTRootView cancelTouches' ]);

And don't works. Any news about the solution?

usrbowe commented 4 years ago

@kmagiera is there any issue in main fb/rn repo to track needed fix? Right now, the fix with importing the rn-gesture in top lvl index.js file is bit hacky.

rptoma commented 4 years ago

Any news? Tried importing in index.js, tried wrapping screens in gestureHandlerRootHOC but with no success. It happens when pressing the return button on Inputs (when the keyboard disappears).

devilabhi commented 4 years ago

import { YellowBox } from 'react-native'; import App from './app/index';

YellowBox.ignoreWarnings(['RCTRootView cancelTouches']);

export default App;

cipriancaba commented 4 years ago

@devilabhi The problem is for production builds, where this warning is basically crashing the app

JuanCamilo0809 commented 4 years ago

@Andriiklymiuk did you try adding import 'react-native-gesture-handler' on top of index.js

it Work for Android but No for IOS

klosowsk commented 4 years ago

Same here, it works for android but not for IOS. Even when I do:

`import 'react-native-gesture-handler';`
... #### ...
import {gestureHandlerRootHOC} from 'react-native-gesture-handler';
AppRegistry.registerComponent(appName, () => gestureHandlerRootHOC(App));
"react": "16.9.0",
"react-native": "0.61.2",
"react-native-maps": "^0.26.1",
"react-native-reanimated": "^1.3.0",
"react-native-gesture-handler": "^1.4.1",
shergin commented 4 years ago

@kmagiera Krzysztof, could you please describe why exactly react-native-gesture-handler needs this feature; why it cannot use standard UIGestureRecognizer-based conflict-resolution approach?

I assume the library contains some GR, so why it's a problem to "ask" RN built-in GR to fail in favor of your GR using standard UIKit APIs (which are way more flexible)? If you have any questions, I would love to help with it.

I am afraid, cancelTouches is a hack. It does not work already in some cases (e.g. inside <Modal>), and the new Surface/Fabric-based APIs will not support that.

kmagiera commented 4 years ago

Hey @shergin – so the reason why it is needed it to keep RN's JS touch system and native gesture system in sync in case an external gesture handler captures the touch stream. What happens now in RN core is that we send touch down, move, up, and cancel events to javascript where there is a state machine relying on receiving all the events in order not to dispatch them properly to the respective targets.

When there are other native gesture recognizers taking part in the process what could happen is that they start recognizing along with the main RN's touch recognizer. When they ended up recognizing the gesture and want to take precedence over RN's touch recognizer the RN's gesture handler will stop receving events. Apparently UIGestureRecognizer does not get notified when some other recognizer captures the touch stream. In particular it does not receive a state change event. This has been a bit counterintuitive for me when I started learning more about gesture rezognizers on iOS but it is just how they work. As a result we need a way to tell the RN's gesture recognizer that some other recognizer has captured the stream in order for it to dispatch touch cancel event to the JS to keep the JS state machine in sync.

An example of this is with native UIViewNavigationController in case when the builtin UIScreenEdgePanGestureRecognizer can kick in when swiping back (same thing happens with new iOS 13 modals that can be pulled down). If the swipe starts at a place where we have TouchablHighlight it will highlight and not get back to the normal state when the swipe starts. It is because both gesture recognizers will start recognizing, the edge one will recognize at some point but JS will never learn of that fact unless we call cancelTouches.

This is a very similar case to when we have touchables inside a ScrollView, however for the case of scrollview JS state machine uses scroll events to reset its state.

I am not sure if there is another way to workaround this problem as I don't have enough time now to verify this, but one possibility could be that we can rely on reset being called in case some other recognizer steals touch stream https://github.com/facebook/react-native/blob/aee88b6843cea63d6aa0b5879ad6ef9da4701846/React/Base/RCTTouchHandler.m#L354 – I suspect what may happen in this case is that _nativeTouches may have more than 0 touches registered. It could be because of the reasons I mentioned and the fact that touchesCancelled will never be called when other recognizer activates.

Hope this is helpful, lmk if you need more details.

richardgroves commented 4 years ago

Should the cancelling of touches not at least be more discriminating? ie only if the gesture recognizer has cancelsTouchesInView set? Otherwise the GR is indicating it is happy to cooperate with other touch consumers.

This cancelTouches code is caused me issues with a native module that attaches gesture recognizers to the main window that have no interaction with the RN system but trigger this code and that breaks other touch/gesture recognition.

swingywc commented 4 years ago

For anyone facing this problem, @osdnk has already committed the fix to the master branch. You can now fix this error by updating the package.json with following:

"react-native-gesture-handler": "git@github.com:kmagiera/react-native-gesture-handler#1f47596713a709af0d368b45ac3ba02a67375347",

and npm install again. This solution is a temporary solution before react-native-gesture-handler releases v1.4.2.

kmagiera commented 4 years ago

hi @richardgroves – appreciate your comment , however in order to answer your question I'd need to learn more about your setup. What I don't quite understand is how your window GR that has no integration with RN code may trigger a method in a RN library. What would work best in this case is if you could open a new issue and provide a small repository illustrating this problem. I'd prefer in this particular issue to focus on the problem of crashes related to calling a deprecated method.

kmagiera commented 4 years ago

@swingywc FYI I just published 1.5.0 version with this update (we bump 1.4.1 -> 1.5.0 and not 1.4.2 because of native changes made on top of 1.4.1)

swingywc commented 4 years ago

Thanks @kmagiera.

hkan commented 4 years ago

1.5.0 update, followed by unlinking and relinking the library, seems to solve the issue for me.

joseguya commented 4 years ago

1.5.0 update, followed by unlinking and relinking the library, seems to solve the issue for me.

I updated, unlinked and relinked, pod install, but the issue still persists env:

rn: 0.61.2 device: iPhone 7, iOS 13.1.2 react-native-gestures: 1.5.0

hkan commented 4 years ago

@joseguya Did you also rebuild in Xcode? I forgot to mention that part. Update > unlink > relink > rebuild. That was my procedure.

joseguya commented 4 years ago

@hkan yes! I always do a clean, delete DerivedData and then build. It usually works but this error still persists in a release build

hkan commented 4 years ago

@joseguya how does putting import 'react-native-gesture-handler'; on top of your entry JS file work now? I do have that in my codebase.

joseguya commented 4 years ago

@hkan deleted that from mine after updating thinking it was not longer necessary, will try that

joseguya commented 4 years ago

@hkan deleted that from mine after updating thinking it was not longer necessary, will try that

That did the trick, but was already working with the previous version of gestures handler. At least the warning is gone in dev builds

shergin commented 4 years ago

@kmagiera We are mostly on the same page! Thank you for your awesome effort to make gesture handling better! ❤️ Let's talk the tech stuff:

The reason why it is needed it to keep RN's JS touch system and native gesture system in sync in case an external gesture handler captures the touch stream.

Yeah!

What happens now in RN core is that we send touch down, move, up, and cancel events to javascript where there is a state machine relying on receiving all the events in order not to dispatch them properly to the respective targets.

Yes!

When there are other native gesture recognizers taking part in the process what could happen is that they start recognizing along with the main RN's touch recognizer. When they ended up recognizing the gesture and want to take precedence over RN's touch recognizer the RN's gesture handler will stop receving events.

I am deeply aware of this issue. I think, that's not exactly what happens. IIRC, by design, RN's GR always fails in favor of any native gesture recognizer. When it happens, it sends cancel touch events to JS side notifying the state machine that those touches are not valid anymore. I know about that because I implemented that ~3 years ago especially to solve all GR problems which we have internally at Facebook (and we have plenty of them). That's why I strongly believe that global cancelTouches is not a good API for that.

Apparently UIGestureRecognizer does not get notified when some other recognizer captures the touch stream.

Which one? AFAIK, iOS has several dedicated methods on GR class that designed to solve this exact problem.

In particular it does not receive a state change event. This has been a bit counterintuitive for me when I started learning more about gesture recognizers on iOS but it is just how they work.

Really? Could you please elaborate on that? I think we can force state to be ended when all touches got cancelled (via touchesCancelled:withEvent:).

As a result we need a way to tell the RN's gesture recognizer that some other recognizer has captured the stream in order for it to dispatch touch cancel event to the JS to keep the JS state machine in sync.

That's the most controversial part to me. I think any attempt to mess with touch recognizers bypassing GR resolution protocol will eventually messy and fail miserably.

An example of this is with native UIViewNavigationController in case when the builtin UIScreenEdgePanGestureRecognizer can kick in when swiping back (same thing happens with new iOS 13 modals that can be pulled down). If the swipe starts at a place where we have TouchablHighlight it will highlight and not get back to the normal state when the swipe starts. It is because both gesture recognizers will start recognizing, the edge one will recognize at some point but JS will never learn of that fact unless we call cancelTouches.

Something wrong happens somewhere else. RN GR must deliver cancel touches to JS side which must help internal state machine to be in-sync.

This is a very similar case to when we have touchables inside a ScrollView, however for the case of scrollview JS state machine uses scroll events to reset its state.

Same.

I am not sure if there is another way to workaround this problem as I don't have enough time now to verify this, but one possibility could be that we can rely on reset being called in case some other recognizer steals touch stream https://github.com/facebook/react-native/blob/aee88b6843cea63d6aa0b5879ad6ef9da4701846/React/Base/RCTTouchHandler.m#L354 – I suspect what may happen in this case is that _nativeTouches may have more than 0 touches registered. It could be because of the reasons I mentioned and the fact that touchesCancelled will never be called when other recognizer activates.

Oh yeah! That's probably how it works. I don't remember details anymore, 3 years passed.

Hope this is helpful, lmk if you need more details.

Krzysztof, we both share the passion of majestic gestures working properly. Let's figure this out together.

shergin commented 4 years ago

@swingywc @osdnk Which fix exactly?

osdnk commented 4 years ago

@shergin https://github.com/kmagiera/react-native-gesture-handler/commit/1f47596713a709af0d368b45ac3ba02a67375347#diff-317a7c9e735ce50581669d39ea18c805L162 I know that this is not perfect

osdnk commented 4 years ago

It's actually fix for displaying a warning, not a bug itself

kmagiera commented 4 years ago

Thanks @shergin for your input on this. Really appreciate you chiming in here.

I want to be mindful of your time spend on this so will do my best to explain what we're trying to achieve here and if you have time to read through all of that would appreciate if you could suggest on how this is resolved in fb codebase.

First, for the sake of simplicity let us focus on the following usecase:

In an app we render a native view with an attached pan gesture recognizer (UIPanGestureRecognizer) which is set up in a way that you can drag that view around. Inside that view (as a subview) we render RN's touchable element – say a TouchableHighlight with some text.

Now when you tap on touchable you want its onPress to be triggered. When you hold finger on touchable and start dragging it should first highlight, then pan recognizer should kick in and the view should start moving along while the touchable should unhighlight (cancel). When you lift the finger the onPress should not trigger.

In order for the above to work what needs to happen is that JS responder should deliver touch cancel to touchable when the pan gesture recognizer activates.

A very similar situation occurs when we have touchable inside of a ScrollView. I believe that in this case the following code is responsible for cancelling JS responder: see ResponderEventPlugin.js

However, in case of pan gesture responder we are not sending scroll events and the only alternative to make JS responder deliver cancel touchable is to dispatch topTouchCancel.

After checking how topTouchCancel can be dispatched on iOS the only place I've found was touchesCancelled:withEvent method in RCTTouchHandler.m. And so we should expect that method to be invoked when pan gesture handler from the described use case kicks in. However based on Apple's GR documentation this method is only called when system event cancels touch sequence (e.g. when alert dialog appears while the touch sequence is ongoing). From the testing I've done, this method also gets triggered when recognizer gets disabled while receiving touch sequence (this is what RCTTouchHandler#cancel does).

I do not see a way based on GR API means how that method could've get triggered otherwise. From my expirience working with gesture recognizer the fact that some other gesture recognizer activates does not result in any of the delegate method getting triggered. I also analyzed the case of reset method I mentioned earlier and it turns out that reset only gets called when touch sequence is over (so in our usecase it'd get triggered when you lift the finger up and not when the pan gesture activates).

kmagiera commented 4 years ago

I am sure I'm definitely missing something and I wonder if you could share any pointers on how touchables might get cancel event delivered in fb codebase if there exists a similar usecase there.

shergin commented 4 years ago

Sure. If some gesture recognizer needs to "win" over RN one, it returns YES in canPreventGestureRecognizer: (or something like this). RN GR always obeys.

shergin commented 4 years ago

Now when you tap on touchable you want its onPress to be triggered. When you hold finger on touchable and start dragging it should first highlight, then pan recognizer should kick in and the view should start moving along while the touchable should unhighlight (cancel). When you lift the finger the onPress should not trigger.

FB blue app has swipe-left-to-pop-view-controller GR. That just works. They (that GR and RN GR) know nothing about each other. The first one is just "being bold" and return YES in canPreventGestureRecognizer: (or some similar method, I don't recall the exact semantic, GR has like a four of those methods) and RN GR just obeys. It sends cancel touches to JS and the JS-based state machine react accordingly.

shergin commented 4 years ago

Correction: I think it works this way: The bold GH does this:

RN's one returns opposite values (IIRC).

kmagiera commented 4 years ago

Hey @shergin, so I played with the suggested methods and what I found out is: