onResponderGrant called before onResponderTerminate

[aleclarson]

When a responder captures the active responder, it seems that onResponderTerminate is not called until after onResponderGrant. This seems like the wrong thing to do. Are there reasons for this behavior?

[aleclarson]

Also, it looks like onResponderGrant is dispatched before onResponderTerminationRequest. This has to be a bug. Submitting a PR in a little bit.

[zpao]

Going to delegate here… @spicyj I know you don't have a ton of time but you might know this code best. You also touched it last to import some changes from RN :). Or maybe @sebmarkbage can take a look / bring in somebody from RN who knows this. I would imagine if things are broken here then RN is going to be hosed.

[sophiebits]

I commented here already: https://github.com/facebook/react/commit/6ec3b651690befb4226230855fa2e102654ad35f#commitcomment-16569138. :) I think @jordwalke is probably the best contact here.

[jordwalke]

Also, it looks like onResponderGrant is dispatched before onResponderTerminationRequest. This has to be a bug. Submitting a PR in a little bit.

Can you double check your reasoning? Note that there's a difference between finding the dispatches and then executing them. For the most part, this function merely extracts dispatches without executing them. There's a couple of special cases where they're actually executed in order to get a return value to do the negotiation. Let me know if I'm missing something.

When a responder captures the active responder, it seems that onResponderTerminate is not called until after onResponderGrant.

This may be true, although I always considered them two events that never guaranteed ordering amongst themselves.

[aleclarson]

There's a couple of special cases where they're actually executed in order

This is why they're "out of order". The grantEvent is executed directly to get the shouldBlockNativeResponder boolean. The problem is that the grantEvent is executed before the terminationRequestEvent is executed. Unless I'm misunderstanding something, this means the grantEvent will be dispatched even if the terminationRequestEvent rejects the capture.

Unless there are performance implications, I think the logical thing to do is ensure the grantEvent of the next responder comes after the terminationRequestEvent and terminateEvent of the active responder. And that's what I did with the PR (#6218).

[sophiebits]

Oh yes, that sounds right. Here's what I wrote when I imported the latest version of this plugin from RN: https://github.com/facebook/react/pull/5308#issuecomment-166764304.

[jordwalke]

Oh, I was looking at the originally imported version. I believe the original version is correct (can you confirm that with your own understanding?) @andreicoman11 - is this being fixed? This would cause a ton of touch handling issues if it's in production.

[sophiebits]

It's been in prod since August.

[jordwalke]

Pinged @andreicoman11 about this, thanks for spotting this bug @aleclarson - feel free to fix, but I want to know what the original intent of this change was.

[sophiebits]

Original summary (D2163934 internally):

Change PanResponder to allow Views to block native components from becoming the responder. This should be done through PanResponder's onResponderReject, but that does not work: onResponderReject will be called after the native View has already intercepted the touch. At that point it is too late to cancel the native intercept (similar on both android and ios) even if we wanted to. Instead, we can now block native components from intercepting the touch in the first place. On the JS side: when you set the panResponders, you can set whether your JS component should block all native components from trying to become responder (ie. range slider not allowing ScrollView or ViewPager from getting the responder, image cropper not allowing ViewPager to steal the responder etc). PanResponder will then tell native to block native components from intercepting touches when the responder is granted, and unblock the setting when the responder is released. On the native side: UIManager passes the jsResponder changes to NativeViewHierarchyManager, which can get the View for the given react tag. We then call (un)blockNativeResponder with the parent of this View because ViewParent implements requestDisallowInterceptTouchEvent - which can be set to not let any ancestors from intercepting the touch events. This required moving JSResponder from UIManager to NativeViewHierarchyManager, but it didn't seem to do much for UIManager in the first place. Also, I did not create an operation for the js responder changes, because they do not qualify as ViewOperations.

[jordwalke]

Ben, I recalled discussing this issue with you. You convinced me that the ideal solution was actually possible. To not try to perform negotiation asynchronously and instead do it entirely in JS. That would have avoided the original issue this attempted to solve. The way that could work:

• All ReactNative scroll views begin without the ability to enable their pan gesture. Then, the logic for enabling the scroll view pan gesture is performed in the React thread. The same system that enabled various React gestures would be the same system that enables the scroll view. Once enabled, we send a message to the UIViewManager that tells it which React Native UIScrollView may become activated and the native gesture takes over from there. I thought it was not possible to later enable a UIScrollView UIPanGestureRecognizer after touches had began, but IIRC you proved that assumption invalid.

(This would be much easier if/when we can eventually move the the main thread (when we have ubiquitous concurrency in React) - but I believe this approach would work until then and would have resolved many of the cases I ran into).

[andreicoman11]

This definitely looks like a bug, I'm surprised this has been going on for so long. The intent behind calling grantEvent directly was to get the result for blockNativeResponder, but it shouldn't happen before terminationRequestEvent gets executed. We should get the PR in asap. Thanks for digging @aleclarson !

[aleclarson]

Just fixed some lint errors. #6218 should be good to merge! :+1: 💰

[jordwalke]

I still don't know about the original intent of the diff:

onResponderReject will be called after the native View has already intercepted the touch.

Why is this true? And why are we looking at the return value of onResponderGrant? If something wants the gesture lock, shouldn't it always block native gestures? I'd say so.

[andreicoman11]

Without the proper flag on the native side, the native ScrollView will intercept the touch no matter what - this is still happening in iOS, it was only fixed for android. If I remember correctly, we only want to set blockNativeResponder when the responder is granted, and for that we need the return value.

[jordwalke]

Every time the responder was granted, we send the setJsResponder event to the platform. Why was that not sufficient?

[sophiebits]

Leaving this open until a fix is merged.

[gaearon]

I just closed https://github.com/facebook/react/pull/6218 because it got stale again. If it is still relevant please submit a new PR (e.g. based on that one) and provide instructions to test the new behavior.