From f3aa134d2b1250ab828d5e404b7377f3e411384e Mon Sep 17 00:00:00 2001 From: Nicolas Gallagher Date: Wed, 24 Apr 2019 14:50:27 -0700 Subject: [PATCH] React events: fix press end event dispatching This patch fixes an issue related to determining whether the end event occurs within the responder region. Previously we only checked if the event target was within the responder region for moves, otherwise we checked if the target was within the event component. Since the dimensions of the child element can change after activation, we need to recalculate the responder region before deactivation as well if the target is not within the event component. --- packages/react-events/src/Press.js | 97 ++++++-- .../src/__tests__/Press-test.internal.js | 216 +++++++++++++++--- packages/react-events/src/utils.js | 2 +- 3 files changed, 262 insertions(+), 53 deletions(-) diff --git a/packages/react-events/src/Press.js b/packages/react-events/src/Press.js index c96c1aae03499..065cbffc6403b 100644 --- a/packages/react-events/src/Press.js +++ b/packages/react-events/src/Press.js @@ -52,10 +52,16 @@ type PressState = { isPressWithinResponderRegion: boolean, longPressTimeout: null | Symbol, pointerType: PointerType, - pressTarget: null | Element | Document, + pressTarget: null | Element, pressEndTimeout: null | Symbol, pressStartTimeout: null | Symbol, - responderRegion: null | $ReadOnly<{| + responderRegionOnActivation: null | $ReadOnly<{| + bottom: number, + left: number, + right: number, + top: number, + |}>, + responderRegionOnDeactivation: null | $ReadOnly<{| bottom: number, left: number, right: number, @@ -312,7 +318,7 @@ function calculateDelayMS(delay: ?number, min = 0, fallback = 0) { } // TODO: account for touch hit slop -function calculateResponderRegion(target, props) { +function calculateResponderRegion(target: Element, props: PressProps) { const pressRetentionOffset = { ...DEFAULT_PRESS_RETENTION_OFFSET, ...props.pressRetentionOffset, @@ -352,15 +358,33 @@ function isPressWithinResponderRegion( nativeEvent: $PropertyType, state: PressState, ): boolean { - const {responderRegion} = state; + const {responderRegionOnActivation, responderRegionOnDeactivation} = state; const event = (nativeEvent: any); + let left, top, right, bottom; + + if (responderRegionOnActivation != null) { + left = responderRegionOnActivation.left; + top = responderRegionOnActivation.top; + right = responderRegionOnActivation.right; + bottom = responderRegionOnActivation.bottom; + + if (responderRegionOnDeactivation != null) { + left = Math.min(left, responderRegionOnDeactivation.left); + top = Math.min(top, responderRegionOnDeactivation.top); + right = Math.max(right, responderRegionOnDeactivation.right); + bottom = Math.max(bottom, responderRegionOnDeactivation.bottom); + } + } return ( - responderRegion != null && - (event.pageX >= responderRegion.left && - event.pageX <= responderRegion.right && - event.pageY >= responderRegion.top && - event.pageY <= responderRegion.bottom) + left != null && + right != null && + top != null && + bottom != null && + (event.pageX >= left && + event.pageX <= right && + event.pageY >= top && + event.pageY <= bottom) ); } @@ -408,7 +432,8 @@ const PressResponder = { pressEndTimeout: null, pressStartTimeout: null, pressTarget: null, - responderRegion: null, + responderRegionOnActivation: null, + responderRegionOnDeactivation: null, ignoreEmulatedMouseEvents: false, }; }, @@ -469,7 +494,11 @@ const PressResponder = { } state.pointerType = pointerType; - state.pressTarget = target; + state.pressTarget = getEventCurrentTarget(event, context); + state.responderRegionOnActivation = calculateResponderRegion( + state.pressTarget, + props, + ); state.isPressWithinResponderRegion = true; dispatchPressStartEvents(context, props, state); context.addRootEventTypes(rootEventTypes); @@ -519,26 +548,34 @@ const PressResponder = { case 'touchmove': { if (state.isPressed) { // Ignore emulated events (pointermove will dispatch touch and mouse events) - // Ignore pointermove events during a keyboard press + // Ignore pointermove events during a keyboard press. if (state.pointerType !== pointerType) { return; } - if (state.responderRegion == null) { - state.responderRegion = calculateResponderRegion( - getEventCurrentTarget(event, context), + // Calculate the responder region we use for deactivation, as the + // element dimensions may have changed since activation. + if ( + state.pressTarget !== null && + state.responderRegionOnDeactivation == null + ) { + state.responderRegionOnDeactivation = calculateResponderRegion( + state.pressTarget, props, ); } - if (isPressWithinResponderRegion(nativeEvent, state)) { - state.isPressWithinResponderRegion = true; + state.isPressWithinResponderRegion = isPressWithinResponderRegion( + nativeEvent, + state, + ); + + if (state.isPressWithinResponderRegion) { if (props.onPressMove) { dispatchEvent(context, state, 'pressmove', props.onPressMove, { discrete: false, }); } } else { - state.isPressWithinResponderRegion = false; dispatchPressEndEvents(context, props, state); } } @@ -551,18 +588,38 @@ const PressResponder = { case 'mouseup': case 'touchend': { if (state.isPressed) { - // Ignore unrelated keyboard events + // Ignore unrelated keyboard events and verify press is within + // responder region for non-keyboard events. if (pointerType === 'keyboard') { if (!isValidKeyPress(nativeEvent.key)) { return; } + // If the event target isn't within the press target, check if we're still + // within the responder region. The region may have changed if the + // element's layout was modified after activation. + } else if ( + state.pressTarget != null && + !context.isTargetWithinElement(target, state.pressTarget) + ) { + // Calculate the responder region we use for deactivation if not + // already done during move event. + if (state.responderRegionOnDeactivation == null) { + state.responderRegionOnDeactivation = calculateResponderRegion( + state.pressTarget, + props, + ); + } + state.isPressWithinResponderRegion = isPressWithinResponderRegion( + nativeEvent, + state, + ); } const wasLongPressed = state.isLongPressed; dispatchPressEndEvents(context, props, state); if (state.pressTarget !== null && props.onPress) { - if (context.isTargetWithinElement(target, state.pressTarget)) { + if (state.isPressWithinResponderRegion) { if ( !( wasLongPressed && diff --git a/packages/react-events/src/__tests__/Press-test.internal.js b/packages/react-events/src/__tests__/Press-test.internal.js index bcb310b217e61..c2f8570efc004 100644 --- a/packages/react-events/src/__tests__/Press-test.internal.js +++ b/packages/react-events/src/__tests__/Press-test.internal.js @@ -189,10 +189,22 @@ describe('Event responder: Press', () => { ); ReactDOM.render(element, container); + ref.current.getBoundingClientRect = () => ({ + top: 50, + left: 50, + bottom: 500, + right: 500, + }); + ref.current.dispatchEvent(createPointerEvent('pointerdown')); jest.advanceTimersByTime(499); expect(onPressStart).toHaveBeenCalledTimes(0); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', { + pageX: 55, + pageY: 55, + }), + ); expect(onPressStart).toHaveBeenCalledTimes(1); jest.runAllTimers(); expect(onPressStart).toHaveBeenCalledTimes(1); @@ -420,9 +432,21 @@ describe('Event responder: Press', () => { ); ReactDOM.render(element, container); + ref.current.getBoundingClientRect = () => ({ + top: 50, + left: 50, + bottom: 500, + right: 500, + }); + ref.current.dispatchEvent(createPointerEvent('pointerdown')); jest.advanceTimersByTime(100); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', { + pageX: 55, + pageY: 55, + }), + ); jest.advanceTimersByTime(10); expect(onPressChange).toHaveBeenCalledWith(true); expect(onPressChange).toHaveBeenCalledWith(false); @@ -479,13 +503,21 @@ describe('Event responder: Press', () => { ); ReactDOM.render(element, container); + ref.current.getBoundingClientRect = () => ({ + top: 0, + left: 0, + bottom: 100, + right: 100, + }); }); it('is called after "pointerup" event', () => { ref.current.dispatchEvent( createPointerEvent('pointerdown', {pointerType: 'pen'}), ); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', {pageX: 10, pageY: 10}), + ); expect(onPress).toHaveBeenCalledTimes(1); expect(onPress).toHaveBeenCalledWith( expect.objectContaining({pointerType: 'pen', type: 'press'}), @@ -510,7 +542,9 @@ describe('Event responder: Press', () => { ReactDOM.render(element, container); ref.current.dispatchEvent(createPointerEvent('pointerdown')); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', {pageX: 10, pageY: 10}), + ); expect(onPress).toHaveBeenCalledTimes(1); }); @@ -709,10 +743,10 @@ describe('Event responder: Press', () => { ReactDOM.render(element, container); ref.current.getBoundingClientRect = () => ({ - top: 50, - left: 50, - bottom: 500, - right: 500, + top: 0, + left: 0, + bottom: 100, + right: 100, }); ref.current.dispatchEvent( createPointerEvent('pointerdown', {pointerType: 'touch'}), @@ -720,8 +754,8 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent( createPointerEvent('pointermove', { pointerType: 'touch', - pageX: 55, - pageY: 55, + pageX: 10, + pageY: 10, }), ); expect(onPressMove).toHaveBeenCalledTimes(1); @@ -741,17 +775,17 @@ describe('Event responder: Press', () => { ReactDOM.render(element, container); ref.current.getBoundingClientRect = () => ({ - top: 50, - left: 50, - bottom: 500, - right: 500, + top: 0, + left: 0, + bottom: 100, + right: 100, }); ref.current.dispatchEvent(createKeyboardEvent('keydown', {key: 'Enter'})); ref.current.dispatchEvent( createPointerEvent('pointermove', { pointerType: 'mouse', - pageX: 55, - pageY: 55, + pageX: 10, + pageY: 10, }), ); expect(onPressMove).not.toBeCalled(); @@ -768,10 +802,10 @@ describe('Event responder: Press', () => { ReactDOM.render(element, container); ref.current.getBoundingClientRect = () => ({ - top: 50, - left: 50, - bottom: 500, - right: 500, + top: 0, + left: 0, + bottom: 100, + right: 100, }); ref.current.dispatchEvent( createPointerEvent('pointerdown', {pointerType: 'touch'}), @@ -780,8 +814,8 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent( createPointerEvent('pointermove', { pointerType: 'touch', - pageX: 55, - pageY: 55, + pageX: 10, + pageY: 10, }), ); ref.current.dispatchEvent(createPointerEvent('touchmove')); @@ -843,7 +877,9 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent( createPointerEvent('pointermove', coordinatesInside), ); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', coordinatesInside), + ); jest.runAllTimers(); expect(events).toEqual([ @@ -890,7 +926,9 @@ describe('Event responder: Press', () => { expect(events).toEqual(['onPressStart', 'onPressChange']); events = []; - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', coordinatesInside), + ); expect(events).toEqual(['onPressEnd', 'onPressChange', 'onPress']); }); @@ -923,7 +961,9 @@ describe('Event responder: Press', () => { pageY: rectMock.top - pressRetentionOffset.top, }), ); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', coordinatesInside), + ); expect(events).toEqual([ 'onPressStart', 'onPressChange', @@ -933,6 +973,86 @@ describe('Event responder: Press', () => { 'onPress', ]); }); + + it('responder region accounts for decrease in element dimensions', () => { + let events = []; + const ref = React.createRef(); + const createEventHandler = msg => () => { + events.push(msg); + }; + + const element = ( + +
+ + ); + + ReactDOM.render(element, container); + ref.current.getBoundingClientRect = getBoundingClientRectMock; + ref.current.dispatchEvent(createPointerEvent('pointerdown')); + // emulate smaller dimensions change on activation + ref.current.getBoundingClientRect = () => ({ + width: 80, + height: 80, + top: 60, + left: 60, + right: 490, + bottom: 490, + }); + const coordinates = { + pageX: rectMock.left, + pageY: rectMock.top, + }; + // move to an area within the pre-activation region + ref.current.dispatchEvent( + createPointerEvent('pointermove', coordinates), + ); + ref.current.dispatchEvent(createPointerEvent('pointerup', coordinates)); + expect(events).toEqual(['onPressStart', 'onPressEnd', 'onPress']); + }); + + it('responder region accounts for increase in element dimensions', () => { + let events = []; + const ref = React.createRef(); + const createEventHandler = msg => () => { + events.push(msg); + }; + + const element = ( + +
+ + ); + + ReactDOM.render(element, container); + ref.current.getBoundingClientRect = getBoundingClientRectMock; + ref.current.dispatchEvent(createPointerEvent('pointerdown')); + // emulate larger dimensions change on activation + ref.current.getBoundingClientRect = () => ({ + width: 200, + height: 200, + top: 0, + left: 0, + right: 550, + bottom: 550, + }); + const coordinates = { + pageX: rectMock.left - 50, + pageY: rectMock.top - 50, + }; + // move to an area within the post-activation region + ref.current.dispatchEvent( + createPointerEvent('pointermove', coordinates), + ); + ref.current.dispatchEvent(createPointerEvent('pointerup', coordinates)); + expect(events).toEqual(['onPressStart', 'onPressEnd', 'onPress']); + }); }); describe('beyond bounds of hit rect', () => { @@ -973,7 +1093,9 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent( createPointerEvent('pointermove', coordinatesOutside), ); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', coordinatesOutside), + ); jest.runAllTimers(); expect(events).toEqual([ @@ -1019,7 +1141,9 @@ describe('Event responder: Press', () => { jest.runAllTimers(); expect(events).toEqual(['onPressMove']); events = []; - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', coordinatesOutside), + ); jest.runAllTimers(); expect(events).toEqual([]); }); @@ -1049,13 +1173,23 @@ describe('Event responder: Press', () => { ); ReactDOM.render(element, container); + ref.current.getBoundingClientRect = () => ({ + top: 0, + left: 0, + bottom: 0, + right: 0, + }); // 1 events = []; ref.current.dispatchEvent(createPointerEvent('pointerdown')); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', {pageX: 10, pageY: 10}), + ); ref.current.dispatchEvent(createPointerEvent('pointerdown')); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', {pageX: 10, pageY: 10}), + ); jest.runAllTimers(); expect(events).toEqual([ @@ -1073,7 +1207,9 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent(createPointerEvent('pointerdown')); jest.advanceTimersByTime(250); jest.advanceTimersByTime(500); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', {pageX: 10, pageY: 10}), + ); jest.runAllTimers(); expect(events).toEqual([ @@ -1121,9 +1257,17 @@ describe('Event responder: Press', () => { ); ReactDOM.render(element, container); + ref.current.getBoundingClientRect = () => ({ + top: 0, + left: 0, + bottom: 0, + right: 0, + }); ref.current.dispatchEvent(createPointerEvent('pointerdown')); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', {pageX: 10, pageY: 10}), + ); expect(events).toEqual([ 'pointerdown', 'inner: onPressStart', @@ -1147,9 +1291,17 @@ describe('Event responder: Press', () => { ); ReactDOM.render(element, container); + ref.current.getBoundingClientRect = () => ({ + top: 0, + left: 0, + bottom: 0, + right: 0, + }); ref.current.dispatchEvent(createPointerEvent('pointerdown')); - ref.current.dispatchEvent(createPointerEvent('pointerup')); + ref.current.dispatchEvent( + createPointerEvent('pointerup', {pageX: 10, pageY: 10}), + ); expect(fn).toHaveBeenCalledTimes(1); }); diff --git a/packages/react-events/src/utils.js b/packages/react-events/src/utils.js index a54c72ed18c66..9fb90c6e167b6 100644 --- a/packages/react-events/src/utils.js +++ b/packages/react-events/src/utils.js @@ -15,7 +15,7 @@ import type { export function getEventCurrentTarget( event: ReactResponderEvent, context: ReactResponderContext, -) { +): Element { const target: any = event.target; let currentTarget = target; while (