From 07e578edb8af1237e3dc127e9a674c4f97d70e54 Mon Sep 17 00:00:00 2001 From: Nicolas Gallagher Date: Thu, 29 Oct 2020 15:51:03 -0700 Subject: [PATCH] [fix] ResponderSystem negotiation logic This fixes a bug in the negotiation logic that caused a cycle of terminate->grant events to be sent to the current responder during a pointer move. The root cause was using an incorrect event path in the calculation of the lowest common ancestor's index. The fix is to ensure that the event path stored with the current responder is pruned to begin with the node that is the current responder (rather than any child responders it may have contained). --- .../Pressable/Pressable.stories.mdx | 6 + .../Pressable/examples/FeedbackEvents.js | 3 +- .../Pressable/examples/PanAndPress.js | 100 +++++++++ .../components/Pressable/examples/index.js | 1 + .../useResponderEvents/ResponderSystem.js | 41 +++- .../__tests__/index-test.js | 205 +++++++++++++++++- 6 files changed, 335 insertions(+), 21 deletions(-) create mode 100644 packages/docs/src/components/Pressable/examples/PanAndPress.js diff --git a/packages/docs/src/components/Pressable/Pressable.stories.mdx b/packages/docs/src/components/Pressable/Pressable.stories.mdx index 5ac540e8..e88b573c 100644 --- a/packages/docs/src/components/Pressable/Pressable.stories.mdx +++ b/packages/docs/src/components/Pressable/Pressable.stories.mdx @@ -64,3 +64,9 @@ Called when the pointer is released, but not if cancelled (e.g. by a scroll that + + + + + + diff --git a/packages/docs/src/components/Pressable/examples/FeedbackEvents.js b/packages/docs/src/components/Pressable/examples/FeedbackEvents.js index 883b726b..0e1714f6 100644 --- a/packages/docs/src/components/Pressable/examples/FeedbackEvents.js +++ b/packages/docs/src/components/Pressable/examples/FeedbackEvents.js @@ -56,13 +56,12 @@ export default function FeedbackEvents() { }} > { - console.log(focused); let backgroundColor = 'white'; if (hovered) { backgroundColor = 'lightgray'; diff --git a/packages/docs/src/components/Pressable/examples/PanAndPress.js b/packages/docs/src/components/Pressable/examples/PanAndPress.js new file mode 100644 index 00000000..1b979104 --- /dev/null +++ b/packages/docs/src/components/Pressable/examples/PanAndPress.js @@ -0,0 +1,100 @@ +import React, { useRef, useState } from 'react'; +import { Animated, View, StyleSheet, PanResponder, Text, TouchableOpacity } from 'react-native'; + +const App = () => { + const pan = useRef(new Animated.ValueXY()).current; + const [x, setX] = useState(0); + + const panResponder = useRef(null); + if (panResponder.current == null) { + panResponder.current = PanResponder.create({ + onMoveShouldSetPanResponder: () => true, + onPanResponderGrant: e => { + console.log('pan grant'); + pan.setOffset({ + x: pan.x._value, + y: pan.y._value + }); + }, + onPanResponderMove: Animated.event([null, { dx: pan.x, dy: pan.y }]), + onPanResponderRelease: () => { + console.log('pan release'); + pan.flattenOffset(); + }, + onPanResponderTerminate() { + console.log('pan terminate'); + pan.flattenOffset(); + } + }); + } + + return ( + + Pressed: {x} + + + setX(x + 1)} style={styles.outerTouchable}> + { + console.log('press inner'); + }} + style={styles.innerTouchable} + /> + + + + + + + ); +}; + +const styles = StyleSheet.create({ + container: { + flex: 1, + alignItems: 'center', + justifyContent: 'center', + userSelect: 'none' + }, + titleText: { + fontSize: 14, + lineHeight: 24, + fontWeight: 'bold' + }, + box: { + height: 200, + width: 150, + backgroundColor: 'lightblue', + borderRadius: 5 + }, + outerTouchable: { + height: 150, + width: 100, + margin: 25, + backgroundColor: 'blue', + borderRadius: 5, + justifyContent: 'center' + }, + innerTouchable: { + height: 20, + flex: 1, + marginVertical: 10, + marginHorizontal: 20, + backgroundColor: 'green', + borderRadius: 5 + }, + disabledButton: { + backgroundColor: 'red' + } +}); + +export default App; diff --git a/packages/docs/src/components/Pressable/examples/index.js b/packages/docs/src/components/Pressable/examples/index.js index 5e41fe04..1b523aff 100644 --- a/packages/docs/src/components/Pressable/examples/index.js +++ b/packages/docs/src/components/Pressable/examples/index.js @@ -1,3 +1,4 @@ export { default as delayEvents } from './DelayEvents'; export { default as disabled } from './Disabled'; export { default as feedbackEvents } from './FeedbackEvents'; +export { default as panAndPress } from './PanAndPress'; diff --git a/packages/react-native-web/src/modules/useResponderEvents/ResponderSystem.js b/packages/react-native-web/src/modules/useResponderEvents/ResponderSystem.js index ffcfc90b..258fd3c2 100644 --- a/packages/react-native-web/src/modules/useResponderEvents/ResponderSystem.js +++ b/packages/react-native-web/src/modules/useResponderEvents/ResponderSystem.js @@ -374,12 +374,14 @@ function eventListener(domEvent: any) { // Start if (isStartEvent) { if (onResponderStart != null) { + responderEvent.dispatchConfig.registrationName = 'onResponderStart'; onResponderStart(responderEvent); } } // Move else if (isMoveEvent) { if (onResponderMove != null) { + responderEvent.dispatchConfig.registrationName = 'onResponderMove'; onResponderMove(responderEvent); } } else { @@ -404,12 +406,14 @@ function eventListener(domEvent: any) { // End if (isEndEvent) { if (onResponderEnd != null) { + responderEvent.dispatchConfig.registrationName = 'onResponderEnd'; onResponderEnd(responderEvent); } } // Release if (isReleaseEvent) { if (onResponderRelease != null) { + responderEvent.dispatchConfig.registrationName = 'onResponderRelease'; onResponderRelease(responderEvent); } changeCurrentResponder(emptyResponder); @@ -424,18 +428,20 @@ function eventListener(domEvent: any) { eventType === 'scroll' || eventType === 'selectionchange' ) { - if ( - wasNegotiated || - // Only call this function is it wasn't already called during negotiation. - (onResponderTerminationRequest != null && - onResponderTerminationRequest(responderEvent) === false) - ) { + // Only call this function is it wasn't already called during negotiation. + if (wasNegotiated) { shouldTerminate = false; + } else if (onResponderTerminationRequest != null) { + responderEvent.dispatchConfig.registrationName = 'onResponderTerminationRequest'; + if (onResponderTerminationRequest(responderEvent) === false) { + shouldTerminate = false; + } } } if (shouldTerminate) { if (onResponderTerminate != null) { + responderEvent.dispatchConfig.registrationName = 'onResponderTerminate'; onResponderTerminate(responderEvent); } changeCurrentResponder(emptyResponder); @@ -466,8 +472,11 @@ function findWantsResponder(eventPaths, domEvent, responderEvent) { const config = getResponderConfig(id); const shouldSetCallback = config[callbackName]; if (shouldSetCallback != null) { + responderEvent.currentTarget = node; if (shouldSetCallback(responderEvent) === true) { - return { id, node, idPath }; + // Start the path from the potential responder + const prunedIdPath = idPath.slice(idPath.indexOf(id)); + return { id, node, idPath: prunedIdPath }; } } }; @@ -521,6 +530,7 @@ function attemptTransfer(responderEvent: ResponderEvent, wantsResponder: ActiveR responderEvent.bubbles = false; responderEvent.cancelable = false; responderEvent.currentTarget = node; + // Set responder if (currentId == null) { if (onResponderGrant != null) { @@ -533,22 +543,35 @@ function attemptTransfer(responderEvent: ResponderEvent, wantsResponder: ActiveR // Negotiate with current responder else { const { onResponderTerminate, onResponderTerminationRequest } = getResponderConfig(currentId); - const allowTransfer = - onResponderTerminationRequest != null && onResponderTerminationRequest(responderEvent); + + let allowTransfer = true; + if (onResponderTerminationRequest != null) { + responderEvent.currentTarget = currentNode; + responderEvent.dispatchConfig.registrationName = 'onResponderTerminationRequest'; + if (onResponderTerminationRequest(responderEvent) === false) { + allowTransfer = false; + } + } + if (allowTransfer) { // Terminate existing responder if (onResponderTerminate != null) { responderEvent.currentTarget = currentNode; + responderEvent.dispatchConfig.registrationName = 'onResponderTerminate'; onResponderTerminate(responderEvent); } // Grant next responder if (onResponderGrant != null) { + responderEvent.currentTarget = node; + responderEvent.dispatchConfig.registrationName = 'onResponderGrant'; onResponderGrant(responderEvent); } changeCurrentResponder(wantsResponder); } else { // Reject responder request if (onResponderReject != null) { + responderEvent.currentTarget = node; + responderEvent.dispatchConfig.registrationName = 'onResponderReject'; onResponderReject(responderEvent); } } diff --git a/packages/react-native-web/src/modules/useResponderEvents/__tests__/index-test.js b/packages/react-native-web/src/modules/useResponderEvents/__tests__/index-test.js index e9c5fb05..7d652c54 100644 --- a/packages/react-native-web/src/modules/useResponderEvents/__tests__/index-test.js +++ b/packages/react-native-web/src/modules/useResponderEvents/__tests__/index-test.js @@ -205,10 +205,9 @@ describe('useResponderEvents', () => { }); testWithPointerType('start grants responder to grandParent', pointerType => { - let grantCurrentTarget, shouldSetCurrentTarget; + let grantCurrentTarget; const grandParentCallbacks = { onStartShouldSetResponderCapture: jest.fn(e => { - shouldSetCurrentTarget = e.currentTarget; return true; }), onResponderGrant: jest.fn(e => { @@ -246,7 +245,6 @@ describe('useResponderEvents', () => { }); // responder set (capture phase) expect(grandParentCallbacks.onStartShouldSetResponderCapture).toBeCalledTimes(1); - expect(shouldSetCurrentTarget).toBe(null); expect(parentCallbacks.onStartShouldSetResponderCapture).not.toBeCalled(); expect(targetCallbacks.onStartShouldSetResponderCapture).not.toBeCalled(); // responder grant @@ -1637,11 +1635,185 @@ describe('useResponderEvents', () => { }); /** - * When there is an active responder, negotiation captures to and bubbles from - * the ancestor registered with the system. The responder is transferred and - * the relevant termination events are called. + * When there is an active responder, negotiation of the active pointer captures to + * and bubbles from the closest common ancestor registered with the system. The + * responder is transferred and maintained for subsequent events of the same type. */ - test('negotiates from first registered ancestor of responder and transfers', () => { + test('negotiates single-touch from first registered ancestor of responder and transfers', () => { + const pointerType = 'touch'; + const eventLog = []; + const grandParentCallbacks = { + onStartShouldSetResponderCapture() { + eventLog.push('grandParent: onStartShouldSetResponderCapture'); + return false; + }, + onStartShouldSetResponder() { + eventLog.push('grandParent: onStartShouldSetResponder'); + return false; + }, + onMoveShouldSetResponderCapture() { + eventLog.push('grandParent: onMoveShouldSetResponderCapture'); + return false; + }, + onMoveShouldSetResponder() { + eventLog.push('grandParent: onMoveShouldSetResponder'); + return true; + }, + onResponderGrant() { + eventLog.push('grandParent: onResponderGrant'); + }, + onResponderStart() { + eventLog.push('grandParent: onResponderStart'); + }, + onResponderMove() { + eventLog.push('grandParent: onResponderMove'); + }, + onResponderEnd() { + eventLog.push('grandParent: onResponderEnd'); + }, + onResponderRelease() { + eventLog.push('grandParent: onResponderRelease'); + }, + onResponderTerminate() { + eventLog.push('grandParent: onResponderTerminate'); + }, + onResponderTerminationRequest() { + eventLog.push('grandParent: onResponderTerminationRequest'); + return true; + } + }; + const parentCallbacks = { + onStartShouldSetResponderCapture() { + eventLog.push('parent: onStartShouldSetResponderCapture'); + return false; + }, + onStartShouldSetResponder() { + eventLog.push('parent: onStartShouldSetResponder'); + return false; + }, + onMoveShouldSetResponderCapture() { + eventLog.push('parent: onMoveShouldSetResponderCapture'); + return false; + }, + onMoveShouldSetResponder() { + eventLog.push('parent: onMoveShouldSetResponder'); + return false; + }, + onResponderGrant() { + eventLog.push('parent: onResponderGrant'); + }, + onResponderStart() { + eventLog.push('parent: onResponderStart'); + }, + onResponderMove() { + eventLog.push('parent: onResponderMove'); + }, + onResponderEnd() { + eventLog.push('parent: onResponderEnd'); + }, + onResponderRelease() { + eventLog.push('parent: onResponderRelease'); + }, + onResponderTerminate() { + eventLog.push('parent: onResponderTerminate'); + }, + onResponderTerminationRequest() { + eventLog.push('parent: onResponderTerminationRequest'); + return true; + } + }; + const targetCallbacks = { + onStartShouldSetResponderCapture() { + eventLog.push('target: onStartShouldSetResponderCapture'); + return false; + }, + onStartShouldSetResponder() { + eventLog.push('target: onStartShouldSetResponder'); + return true; + }, + onMoveShouldSetResponderCapture() { + eventLog.push('target: onMoveShouldSetResponderCapture'); + return false; + }, + onMoveShouldSetResponder() { + eventLog.push('target: onMoveShouldSetResponder'); + return false; + }, + onResponderGrant() { + eventLog.push('target: onResponderGrant'); + }, + onResponderStart() { + eventLog.push('target: onResponderStart'); + }, + onResponderMove() { + eventLog.push('target: onResponderMove'); + }, + onResponderEnd() { + eventLog.push('target: onResponderEnd'); + }, + onResponderRelease() { + eventLog.push('target: onResponderRelease'); + }, + onResponderTerminate() { + eventLog.push('target: onResponderTerminate'); + }, + onResponderTerminationRequest() { + eventLog.push('target: onResponderTerminationRequest'); + return true; + } + }; + + const Component = () => { + useResponderEvents(grandParentRef, grandParentCallbacks); + useResponderEvents(parentRef, parentCallbacks); + useResponderEvents(targetRef, targetCallbacks); + return ( +
+
+
+
+
+ ); + }; + + // render + act(() => { + render(); + }); + const target = createEventTarget(targetRef.current); + + // gesture start + act(() => { + target.pointerdown({ pointerType, pointerId: 1 }); + target.pointermove({ pointerType, pointerId: 1 }); + target.pointermove({ pointerType, pointerId: 1 }); + }); + expect(eventLog).toEqual([ + 'grandParent: onStartShouldSetResponderCapture', + 'parent: onStartShouldSetResponderCapture', + 'target: onStartShouldSetResponderCapture', + 'target: onStartShouldSetResponder', + 'target: onResponderGrant', + 'target: onResponderStart', + 'grandParent: onMoveShouldSetResponderCapture', + 'parent: onMoveShouldSetResponderCapture', + 'parent: onMoveShouldSetResponder', + 'grandParent: onMoveShouldSetResponder', + 'target: onResponderTerminationRequest', + 'target: onResponderTerminate', + 'grandParent: onResponderGrant', + 'grandParent: onResponderMove', + // Continues calling 'move' rather than entering into negotiation again + 'grandParent: onResponderMove' + ]); + }); + + /** + * When there is an active responder, negotiation of a second pointer captures to + * and bubbles from the closest common ancestor registered with the system. The + * responder is transferred andvthe relevant termination events are called. + */ + test('negotiates multi-touch from first registered ancestor of responder and transfers', () => { const pointerType = 'touch'; let eventLog = []; const grandParentCallbacks = { @@ -1667,6 +1839,9 @@ describe('useResponderEvents', () => { onResponderStart() { eventLog.push('grandParent: onResponderStart'); }, + onResponderMove() { + eventLog.push('grandParent: onResponderMove'); + }, onResponderEnd() { eventLog.push('grandParent: onResponderEnd'); }, @@ -1704,6 +1879,9 @@ describe('useResponderEvents', () => { onResponderStart() { eventLog.push('parent: onResponderStart'); }, + onResponderMove() { + eventLog.push('parent: onResponderMove'); + }, onResponderEnd() { eventLog.push('parent: onResponderEnd'); }, @@ -1741,6 +1919,9 @@ describe('useResponderEvents', () => { onResponderStart() { eventLog.push('target: onResponderStart'); }, + onResponderMove() { + eventLog.push('target: onResponderMove'); + }, onResponderEnd() { eventLog.push('target: onResponderEnd'); }, @@ -1816,21 +1997,25 @@ describe('useResponderEvents', () => { 'parent: onMoveShouldSetResponder', 'target: onResponderTerminationRequest', 'target: onResponderTerminate', - 'parent: onResponderGrant' + 'parent: onResponderGrant', + 'parent: onResponderMove' ]); eventLog = []; // second move gesture act(() => { target.pointermove({ pointerType, pointerId: 1 }); + target.pointermove({ pointerType, pointerId: 2 }); }); - // parent becomes responder, parent terminates + // grand parent becomes responder, parent terminates expect(getResponderNode()).toBe(grandParentRef.current); expect(eventLog).toEqual([ 'grandParent: onMoveShouldSetResponderCapture', 'grandParent: onMoveShouldSetResponder', 'parent: onResponderTerminationRequest', 'parent: onResponderTerminate', - 'grandParent: onResponderGrant' + 'grandParent: onResponderGrant', + 'grandParent: onResponderMove', + 'grandParent: onResponderMove' ]); eventLog = []; // end gestures