From 7126a37bf4284a30377e08b2ef84f61b64fdb8cd Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 4 Sep 2019 18:25:05 +0100 Subject: [PATCH] [react-events] Keyboard responder propagation handling (#16657) --- .../legacy-events/ReactGenericBatching.js | 8 +- .../src/events/DOMEventResponderSystem.js | 29 +++++-- packages/react-events/src/dom/Keyboard.js | 9 +- .../dom/__tests__/Keyboard-test.internal.js | 84 +++++++++++++++++++ packages/shared/ReactDOMTypes.js | 1 + 5 files changed, 117 insertions(+), 14 deletions(-) diff --git a/packages/legacy-events/ReactGenericBatching.js b/packages/legacy-events/ReactGenericBatching.js index d3cf009c4571a..3d1acebde6126 100644 --- a/packages/legacy-events/ReactGenericBatching.js +++ b/packages/legacy-events/ReactGenericBatching.js @@ -11,8 +11,6 @@ import { } from './ReactControlledComponent'; import {enableFlareAPI} from 'shared/ReactFeatureFlags'; -import {invokeGuardedCallbackAndCatchFirstError} from 'shared/ReactErrorUtils'; - // Used as a way to call batchedUpdates when we don't have a reference to // the renderer. Such as when we're dispatching events or if third party // libraries need to call batchedUpdates. Eventually, this API will go away when @@ -77,12 +75,12 @@ export function batchedEventUpdates(fn, a, b) { } } -export function executeUserEventHandler(fn: any => void, value: any) { +// This is for the React Flare event system +export function executeUserEventHandler(fn: any => void, value: any): any { const previouslyInEventHandler = isInsideEventHandler; try { isInsideEventHandler = true; - const type = typeof value === 'object' && value !== null ? value.type : ''; - invokeGuardedCallbackAndCatchFirstError(type, fn, undefined, value); + return fn(value); } finally { isInsideEventHandler = previouslyInEventHandler; } diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index ec2d977d97efd..01cb120709b28 100644 --- a/packages/react-dom/src/events/DOMEventResponderSystem.js +++ b/packages/react-dom/src/events/DOMEventResponderSystem.js @@ -76,11 +76,17 @@ const rootEventTypesToEventResponderInstances: Map< Set, > = new Map(); +type PropagationBehavior = 0 | 1; + +const DoNotPropagateToNextResponder = 0; +const PropagateToNextResponder = 1; + let currentTimeStamp = 0; let currentTimers = new Map(); let currentInstance: null | ReactDOMEventResponderInstance = null; let currentTimerIDCounter = 0; let currentDocument: null | Document = null; +let currentPropagationBehavior: PropagationBehavior = DoNotPropagateToNextResponder; let currentTargetFiber: null | Fiber = null; const eventResponderContext: ReactDOMResponderContext = { @@ -88,30 +94,27 @@ const eventResponderContext: ReactDOMResponderContext = { eventValue: any, eventListener: any => void, eventPriority: EventPriority, - ): void { + ): any { validateResponderContext(); validateEventValue(eventValue); switch (eventPriority) { case DiscreteEvent: { flushDiscreteUpdatesIfNeeded(currentTimeStamp); - discreteUpdates(() => + return discreteUpdates(() => executeUserEventHandler(eventListener, eventValue), ); - break; } case UserBlockingEvent: { if (enableUserBlockingEvents) { - runWithPriority(UserBlockingPriority, () => + return runWithPriority(UserBlockingPriority, () => executeUserEventHandler(eventListener, eventValue), ); } else { - executeUserEventHandler(eventListener, eventValue); + return executeUserEventHandler(eventListener, eventValue); } - break; } case ContinuousEvent: { - executeUserEventHandler(eventListener, eventValue); - break; + return executeUserEventHandler(eventListener, eventValue); } } }, @@ -266,6 +269,9 @@ const eventResponderContext: ReactDOMResponderContext = { } return false; }, + continuePropagation() { + currentPropagationBehavior = PropagateToNextResponder; + }, enqueueStateRestore, getCurrentTarget(): Element | null { validateResponderContext(); @@ -489,6 +495,10 @@ function traverseAndHandleEventResponderInstances( if (onEvent !== null) { currentInstance = responderInstance; onEvent(responderEvent, eventResponderContext, props, state); + if (currentPropagationBehavior === PropagateToNextResponder) { + visitedResponders.delete(responder); + currentPropagationBehavior = DoNotPropagateToNextResponder; + } } } } @@ -588,7 +598,9 @@ export function dispatchEventForResponderEventSystem( const previousTimers = currentTimers; const previousTimeStamp = currentTimeStamp; const previousDocument = currentDocument; + const previousPropagationBehavior = currentPropagationBehavior; const previousTargetFiber = currentTargetFiber; + currentPropagationBehavior = DoNotPropagateToNextResponder; currentTimers = null; currentTargetFiber = targetFiber; // nodeType 9 is DOCUMENT_NODE @@ -613,6 +625,7 @@ export function dispatchEventForResponderEventSystem( currentInstance = previousInstance; currentTimeStamp = previousTimeStamp; currentDocument = previousDocument; + currentPropagationBehavior = previousPropagationBehavior; currentTargetFiber = previousTargetFiber; } } diff --git a/packages/react-events/src/dom/Keyboard.js b/packages/react-events/src/dom/Keyboard.js index c636f97fb1f26..68f05e49f5904 100644 --- a/packages/react-events/src/dom/Keyboard.js +++ b/packages/react-events/src/dom/Keyboard.js @@ -174,7 +174,14 @@ function dispatchKeyboardEvent( type, defaultPrevented, ); - context.dispatchEvent(syntheticEvent, listener, DiscreteEvent); + const shouldPropagate = context.dispatchEvent( + syntheticEvent, + listener, + DiscreteEvent, + ); + if (shouldPropagate) { + context.continuePropagation(); + } } const keyboardResponderImpl = { diff --git a/packages/react-events/src/dom/__tests__/Keyboard-test.internal.js b/packages/react-events/src/dom/__tests__/Keyboard-test.internal.js index 412aaa84b98d2..fdfc9b40dc997 100644 --- a/packages/react-events/src/dom/__tests__/Keyboard-test.internal.js +++ b/packages/react-events/src/dom/__tests__/Keyboard-test.internal.js @@ -251,4 +251,88 @@ describe('Keyboard event responder', () => { ); }); }); + + describe('correctly handles responder propagation', () => { + describe('onKeyDown', () => { + let onKeyDownInner, onKeyDownOuter, ref; + + function renderPropagationTest(propagates) { + onKeyDownInner = jest.fn(() => propagates); + onKeyDownOuter = jest.fn(); + ref = React.createRef(); + const Component = () => { + const listenerInner = useKeyboard({ + onKeyDown: onKeyDownInner, + }); + const listenerOuter = useKeyboard({ + onKeyDown: onKeyDownOuter, + }); + return ( +
+
+
+ ); + }; + ReactDOM.render(, container); + } + + it('propagates when cb returns true', () => { + renderPropagationTest(true); + const target = createEventTarget(ref.current); + target.keydown(); + expect(onKeyDownInner).toBeCalled(); + expect(onKeyDownOuter).toBeCalled(); + }); + + it('does not propagate when cb returns false', () => { + renderPropagationTest(false); + const target = createEventTarget(ref.current); + target.keydown(); + expect(onKeyDownInner).toBeCalled(); + expect(onKeyDownOuter).not.toBeCalled(); + }); + }); + + describe('onKeyUp', () => { + let onKeyUpInner, onKeyUpOuter, ref; + + function renderPropagationTest(propagates) { + onKeyUpInner = jest.fn(() => propagates); + onKeyUpOuter = jest.fn(); + ref = React.createRef(); + const Component = () => { + const listenerInner = useKeyboard({ + onKeyUp: onKeyUpInner, + }); + const listenerOuter = useKeyboard({ + onKeyUp: onKeyUpOuter, + }); + return ( +
+
+
+ ); + }; + ReactDOM.render(, container); + } + + it('propagates when cb returns true', () => { + renderPropagationTest(true); + const target = createEventTarget(ref.current); + target.keydown(); + target.keyup(); + expect(onKeyUpInner).toBeCalled(); + expect(onKeyUpOuter).toBeCalled(); + }); + + it('does not propagate when cb returns false', () => { + renderPropagationTest(false); + const target = createEventTarget(ref.current); + target.keydown(); + target.keyup(); + expect(onKeyUpInner).toBeCalled(); + expect(onKeyUpOuter).not.toBeCalled(); + }); + }); + }); }); diff --git a/packages/shared/ReactDOMTypes.js b/packages/shared/ReactDOMTypes.js index bbaabb9e60326..9adc678ae8044 100644 --- a/packages/shared/ReactDOMTypes.js +++ b/packages/shared/ReactDOMTypes.js @@ -72,6 +72,7 @@ export type ReactDOMResponderContext = { target: Element | Document, elementType: string, ) => boolean, + continuePropagation(): void, // Used for controller components enqueueStateRestore(Element | Document): void, getCurrentTarget(): Element | null,