From dd43cb5fb941cb253a9f0197a1f3957aebdcf3d8 Mon Sep 17 00:00:00 2001 From: Nicolas Gallagher Date: Mon, 3 Jun 2019 14:59:26 -0700 Subject: [PATCH] [Flare] Fix isPressWithinResponderRegion logic (#15808) Compare the viewport-relative coordinates of getBoundingClientRect with those of the event's client{X,Y} values. This fixes press within scrollable nodes. --- packages/react-events/src/Press.js | 92 +------ .../src/__tests__/Press-test.internal.js | 255 ++++-------------- 2 files changed, 59 insertions(+), 288 deletions(-) diff --git a/packages/react-events/src/Press.js b/packages/react-events/src/Press.js index ffa589ab59..189956d9d0 100644 --- a/packages/react-events/src/Press.js +++ b/packages/react-events/src/Press.js @@ -42,8 +42,8 @@ type PointerType = '' | 'mouse' | 'keyboard' | 'pen' | 'touch'; type PressState = { activationPosition: null | $ReadOnly<{| - pageX: number, - pageY: number, + x: number, + y: number, |}>, addedRootEvents: boolean, isActivePressed: boolean, @@ -247,14 +247,11 @@ function dispatchLongPressChangeEvent( function activate(event: ReactResponderEvent, context, props, state) { const nativeEvent: any = event.nativeEvent; - const {x, y} = getEventPageCoords(nativeEvent); + const {x, y} = getEventViewportCoords(nativeEvent); const wasActivePressed = state.isActivePressed; state.isActivePressed = true; if (x !== null && y !== null) { - state.activationPosition = { - pageX: x, - pageY: y, - }; + state.activationPosition = {x, y}; } if (props.onPressStart) { @@ -432,66 +429,6 @@ function calculateDelayMS(delay: ?number, min = 0, fallback = 0) { return Math.max(min, maybeNumber != null ? maybeNumber : fallback); } -function isNodeFixedPositioned(node: Node | null | void): boolean { - return ( - node != null && - (node: any).offsetParent === null && - !isNodeDocumentNode(node.parentNode) - ); -} - -function isNodeDocumentNode(node: Node | null | void): boolean { - return node != null && node.nodeType === Node.DOCUMENT_NODE; -} - -function getAbsoluteBoundingClientRect( - target: Element, -): {left: number, right: number, bottom: number, top: number} { - const clientRect = target.getBoundingClientRect(); - let {left, right, bottom, top} = clientRect; - let node = target.parentNode; - let offsetX = 0; - let offsetY = 0; - - // Traverse through all offset nodes - while (node != null) { - const parent = node.parentNode; - const scrollTop = (node: any).scrollTop; - const scrollLeft = (node: any).scrollLeft; - - // We first need to check if it's a scrollable container by - // checking if either scrollLeft or scrollTop are not 0. - // Then we check if either the current node or its parent - // are fixed position, using offsetParent node for a fast-path. - // We need to check both as offsetParent accounts for both - // itself and the parent; so we need to align with that API. - // If these all pass, we can skip traversing the relevant - // node and go directly to its parent. - if (scrollLeft !== 0 || scrollTop !== 0) { - if (isNodeFixedPositioned(parent)) { - node = ((parent: any): Node).parentNode; - continue; - } - if (isNodeFixedPositioned(node)) { - node = parent; - continue; - } - } - offsetX += scrollLeft; - offsetY += scrollTop; - if (isNodeDocumentNode(parent)) { - break; - } - node = parent; - } - return { - left: left + offsetX, - right: right + offsetX, - bottom: bottom + offsetY, - top: top + offsetY, - }; -} - // TODO: account for touch hit slop function calculateResponderRegion( context: ReactResponderContext, @@ -504,8 +441,7 @@ function calculateResponderRegion( props.pressRetentionOffset, ); - const clientRect = getAbsoluteBoundingClientRect(target); - let {left, right, bottom, top} = clientRect; + let {left, right, bottom, top} = target.getBoundingClientRect(); if (pressRetentionOffset) { if (pressRetentionOffset.bottom != null) { @@ -543,18 +479,18 @@ function getTouchFromPressEvent(nativeEvent: TouchEvent): Touch { : (nativeEvent: any); } -function getEventPageCoords( +function getEventViewportCoords( nativeEvent: Event, ): {x: null | number, y: null | number} { let eventObject = (nativeEvent: any); if (isTouchEvent(eventObject)) { eventObject = getTouchFromPressEvent(eventObject); } - const pageX = eventObject.pageX; - const pageY = eventObject.pageY; + const x = eventObject.clientX; + const y = eventObject.clientY; return { - x: pageX != null ? pageX : null, - y: pageY != null ? pageY : null, + x: x != null ? x : null, + y: y != null ? y : null, }; } @@ -578,7 +514,7 @@ function isPressWithinResponderRegion( bottom = Math.max(bottom, responderRegionOnDeactivation.bottom); } } - const {x, y} = getEventPageCoords(((nativeEvent: any): Event)); + const {x, y} = getEventViewportCoords(((nativeEvent: any): Event)); return ( left != null && @@ -833,10 +769,8 @@ const PressResponder = { state.activationPosition != null && state.longPressTimeout != null ) { - const deltaX = - state.activationPosition.pageX - nativeEvent.pageX; - const deltaY = - state.activationPosition.pageY - nativeEvent.pageY; + const deltaX = state.activationPosition.x - nativeEvent.clientX; + const deltaY = state.activationPosition.y - nativeEvent.clientY; if ( Math.hypot(deltaX, deltaY) > 10 && state.longPressTimeout != null diff --git a/packages/react-events/src/__tests__/Press-test.internal.js b/packages/react-events/src/__tests__/Press-test.internal.js index e4e6f3c4c5..044873dc60 100644 --- a/packages/react-events/src/__tests__/Press-test.internal.js +++ b/packages/react-events/src/__tests__/Press-test.internal.js @@ -206,8 +206,8 @@ describe('Event responder: Press', () => { expect(onPressStart).toHaveBeenCalledTimes(0); ref.current.dispatchEvent( createEvent('pointerup', { - pageX: 55, - pageY: 55, + clientX: 55, + clientY: 55, }), ); expect(onPressStart).toHaveBeenCalledTimes(1); @@ -471,8 +471,8 @@ describe('Event responder: Press', () => { jest.advanceTimersByTime(100); ref.current.dispatchEvent( createEvent('pointerup', { - pageX: 55, - pageY: 55, + clientX: 55, + clientY: 55, }), ); jest.advanceTimersByTime(10); @@ -544,7 +544,7 @@ describe('Event responder: Press', () => { createEvent('pointerdown', {pointerType: 'pen'}), ); ref.current.dispatchEvent( - createEvent('pointerup', {pageX: 10, pageY: 10}), + createEvent('pointerup', {clientX: 10, clientY: 10}), ); expect(onPress).toHaveBeenCalledTimes(1); expect(onPress).toHaveBeenCalledWith( @@ -592,7 +592,7 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent(createEvent('pointerdown')); ref.current.dispatchEvent( - createEvent('pointerup', {pageX: 10, pageY: 10}), + createEvent('pointerup', {clientX: 10, clientY: 10}), ); expect(onPress).toHaveBeenCalledTimes(1); }); @@ -668,10 +668,10 @@ describe('Event responder: Press', () => { right: 100, }); ref.current.dispatchEvent( - createEvent('pointerdown', {pageX: 10, pageY: 10}), + createEvent('pointerdown', {clientX: 10, clientY: 10}), ); ref.current.dispatchEvent( - createEvent('pointermove', {pageX: 50, pageY: 50}), + createEvent('pointermove', {clientX: 50, clientY: 50}), ); jest.runAllTimers(); expect(onLongPress).not.toBeCalled(); @@ -820,8 +820,8 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent( createEvent('pointermove', { pointerType: 'touch', - pageX: 10, - pageY: 10, + clientX: 10, + clientY: 10, }), ); expect(onPressMove).toHaveBeenCalledTimes(1); @@ -850,8 +850,8 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent( createEvent('pointermove', { pointerType: 'mouse', - pageX: 10, - pageY: 10, + clientX: 10, + clientY: 10, }), ); expect(onPressMove).not.toBeCalled(); @@ -880,8 +880,8 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent( createEvent('pointermove', { pointerType: 'touch', - pageX: 10, - pageY: 10, + clientX: 10, + clientY: 10, }), ); ref.current.dispatchEvent(createEvent('touchmove')); @@ -902,12 +902,12 @@ describe('Event responder: Press', () => { const pressRectOffset = 20; const getBoundingClientRectMock = () => rectMock; const coordinatesInside = { - pageX: rectMock.left - pressRectOffset, - pageY: rectMock.top - pressRectOffset, + clientX: rectMock.left - pressRectOffset, + clientY: rectMock.top - pressRectOffset, }; const coordinatesOutside = { - pageX: rectMock.left - pressRectOffset - 1, - pageY: rectMock.top - pressRectOffset - 1, + clientX: rectMock.left - pressRectOffset - 1, + clientY: rectMock.top - pressRectOffset - 1, }; describe('within bounds of hit rect', () => { @@ -1019,8 +1019,8 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent(createEvent('pointerdown')); ref.current.dispatchEvent( createEvent('pointermove', { - pageX: rectMock.left - pressRetentionOffset.left, - pageY: rectMock.top - pressRetentionOffset.top, + clientX: rectMock.left - pressRetentionOffset.left, + clientY: rectMock.top - pressRetentionOffset.top, }), ); ref.current.dispatchEvent(createEvent('pointerup', coordinatesInside)); @@ -1063,8 +1063,8 @@ describe('Event responder: Press', () => { bottom: 490, }); const coordinates = { - pageX: rectMock.left, - pageY: rectMock.top, + clientX: rectMock.left, + clientY: rectMock.top, }; // move to an area within the pre-activation region ref.current.dispatchEvent(createEvent('pointermove', coordinates)); @@ -1101,8 +1101,8 @@ describe('Event responder: Press', () => { bottom: 550, }); const coordinates = { - pageX: rectMock.left - 50, - pageY: rectMock.top - 50, + clientX: rectMock.left - 50, + clientY: rectMock.top - 50, }; // move to an area within the post-activation region ref.current.dispatchEvent(createEvent('pointermove', coordinates)); @@ -1111,176 +1111,6 @@ describe('Event responder: Press', () => { }); }); - describe('the page offset changes', () => { - it('"onPress" is called on release', () => { - let events = []; - const ref = React.createRef(); - const createEventHandler = msg => () => { - events.push(msg); - }; - - const element = ( - -
- - ); - - ReactDOM.render(element, container); - - ref.current.getBoundingClientRect = getBoundingClientRectMock; - // Emulate the element being offset with scroll - document.firstElementChild.scrollTop = 1000; - const updatedCoordinatesInside = { - pageX: coordinatesInside.pageX, - pageY: coordinatesInside.pageY + 1000, - }; - ref.current.dispatchEvent( - createEvent('pointerdown', updatedCoordinatesInside), - ); - container.dispatchEvent( - createEvent('pointermove', updatedCoordinatesInside), - ); - container.dispatchEvent( - createEvent('pointerup', updatedCoordinatesInside), - ); - jest.runAllTimers(); - document.firstElementChild.scrollTop = 0; - - expect(events).toEqual([ - 'onPressStart', - 'onPressChange', - 'onPressMove', - 'onPressEnd', - 'onPressChange', - 'onPress', - ]); - }); - - it('"onPress" is called on release inside a fixed container', () => { - let events = []; - const ref = React.createRef(); - const fixedContainerRef = React.createRef(); - const createEventHandler = msg => () => { - events.push(msg); - }; - - const element = ( -
- -
- -
- ); - - ReactDOM.render(element, container); - - const fixedDiv = fixedContainerRef.current; - Object.defineProperty(fixedDiv, 'offsetParent', { - value: null, - }); - - // The fixed container is not scrolled - fixedDiv.scrollTop = 0; - fixedDiv.scrollLeft = 0; - ref.current.getBoundingClientRect = getBoundingClientRectMock; - // Emulate the element being offset with scroll - document.firstElementChild.scrollTop = 1000; - const updatedCoordinatesInside = { - pageX: coordinatesInside.pageX, - pageY: coordinatesInside.pageY + 1000, - }; - ref.current.dispatchEvent( - createEvent('pointerdown', updatedCoordinatesInside), - ); - container.dispatchEvent( - createEvent('pointermove', updatedCoordinatesInside), - ); - container.dispatchEvent( - createEvent('pointerup', updatedCoordinatesInside), - ); - jest.runAllTimers(); - document.firstElementChild.scrollTop = 0; - - expect(events).toEqual([ - 'onPressStart', - 'onPressChange', - 'onPressMove', - 'onPressEnd', - 'onPressChange', - 'onPress', - ]); - }); - - it('"onPress" is called on release inside a fixed scrolled container', () => { - let events = []; - const ref = React.createRef(); - const fixedContainerRef = React.createRef(); - const createEventHandler = msg => () => { - events.push(msg); - }; - - const element = ( -
- -
- -
- ); - - ReactDOM.render(element, container); - - const fixedDiv = fixedContainerRef.current; - Object.defineProperty(fixedDiv, 'offsetParent', { - value: null, - }); - - // The fixed container is scrolled - fixedDiv.scrollTop = 100; - ref.current.getBoundingClientRect = getBoundingClientRectMock; - // Emulate the element being offset with scroll - document.firstElementChild.scrollTop = 1000; - const updatedCoordinatesInside = { - pageX: coordinatesInside.pageX, - pageY: coordinatesInside.pageY + 1100, - }; - ref.current.dispatchEvent( - createEvent('pointerdown', updatedCoordinatesInside), - ); - container.dispatchEvent( - createEvent('pointermove', updatedCoordinatesInside), - ); - container.dispatchEvent( - createEvent('pointerup', updatedCoordinatesInside), - ); - jest.runAllTimers(); - document.firstElementChild.scrollTop = 0; - - expect(events).toEqual([ - 'onPressStart', - 'onPressChange', - 'onPressMove', - 'onPressEnd', - 'onPressChange', - 'onPress', - ]); - }); - }); - describe('beyond bounds of hit rect', () => { /** ┌──────────────────┐ * │ ┌────────────┐ │ @@ -1569,16 +1399,16 @@ describe('Event responder: Press', () => { const coordinatesInside = { changedTouches: [ { - pageX: rectMock.left - pressRectOffset, - pageY: rectMock.top - pressRectOffset, + clientX: rectMock.left - pressRectOffset, + clientY: rectMock.top - pressRectOffset, }, ], }; const coordinatesOutside = { changedTouches: [ { - pageX: rectMock.left - pressRectOffset - 1, - pageY: rectMock.top - pressRectOffset - 1, + clientX: rectMock.left - pressRectOffset - 1, + clientY: rectMock.top - pressRectOffset - 1, }, ], }; @@ -1688,8 +1518,8 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent(createEvent('touchstart')); ref.current.dispatchEvent( createEvent('touchmove', { - pageX: rectMock.left - pressRetentionOffset.left, - pageY: rectMock.top - pressRetentionOffset.top, + clientX: rectMock.left - pressRetentionOffset.left, + clientY: rectMock.top - pressRetentionOffset.top, }), ); ref.current.dispatchEvent(createEvent('touchend', coordinatesInside)); @@ -1732,8 +1562,8 @@ describe('Event responder: Press', () => { bottom: 490, }); const coordinates = { - pageX: rectMock.left, - pageY: rectMock.top, + clientX: rectMock.left, + clientY: rectMock.top, }; // move to an area within the pre-activation region ref.current.dispatchEvent(createEvent('touchmove', coordinates)); @@ -1770,8 +1600,8 @@ describe('Event responder: Press', () => { bottom: 550, }); const coordinates = { - pageX: rectMock.left - 50, - pageY: rectMock.top - 50, + clientX: rectMock.left - 50, + clientY: rectMock.top - 50, }; // move to an area within the post-activation region ref.current.dispatchEvent(createEvent('touchmove', coordinates)); @@ -1963,11 +1793,11 @@ describe('Event responder: Press', () => { events = []; ref.current.dispatchEvent(createEvent('pointerdown')); ref.current.dispatchEvent( - createEvent('pointerup', {pageX: 10, pageY: 10}), + createEvent('pointerup', {clientX: 10, clientY: 10}), ); ref.current.dispatchEvent(createEvent('pointerdown')); ref.current.dispatchEvent( - createEvent('pointerup', {pageX: 10, pageY: 10}), + createEvent('pointerup', {clientX: 10, clientY: 10}), ); jest.runAllTimers(); @@ -1987,7 +1817,7 @@ describe('Event responder: Press', () => { jest.advanceTimersByTime(250); jest.advanceTimersByTime(500); ref.current.dispatchEvent( - createEvent('pointerup', {pageX: 10, pageY: 10}), + createEvent('pointerup', {clientX: 10, clientY: 10}), ); jest.runAllTimers(); @@ -2045,7 +1875,7 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent(createEvent('pointerdown')); ref.current.dispatchEvent( - createEvent('pointerup', {pageX: 10, pageY: 10}), + createEvent('pointerup', {clientX: 10, clientY: 10}), ); expect(events).toEqual([ 'inner: onPressStart', @@ -2079,7 +1909,7 @@ describe('Event responder: Press', () => { ref.current.dispatchEvent(createEvent('pointerdown')); ref.current.dispatchEvent( - createEvent('pointerup', {pageX: 10, pageY: 10}), + createEvent('pointerup', {clientX: 10, clientY: 10}), ); expect(fn).toHaveBeenCalledTimes(1); }); @@ -2344,6 +2174,13 @@ describe('Event responder: Press', () => { ); ReactDOM.render(element, container); + ref.current.getBoundingClientRect = () => ({ + top: 10, + left: 10, + bottom: 20, + right: 20, + }); + ref.current.dispatchEvent( createEvent('pointerdown', { pointerType: 'mouse',