From 80f695d50aa9d19817c6125e2c1442ca6437eaa8 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 14 Oct 2020 13:47:21 +0100 Subject: [PATCH 1/2] [EventSystem] Revise onBeforeBlur propagation mechancis Fix host configs Fix lint --- packages/react-art/src/ReactARTHostConfig.js | 2 +- .../src/client/ReactDOMHostConfig.js | 14 +- .../src/events/DOMPluginEventSystem.js | 17 ++- .../DOMPluginEventSystem-test.internal.js | 135 ++++++++++++++++++ .../src/events/plugins/SimpleEventPlugin.js | 1 + .../src/ReactFabricHostConfig.js | 2 +- .../src/ReactNativeHostConfig.js | 2 +- .../src/ReactFiberWorkLoop.new.js | 4 +- .../src/ReactFiberWorkLoop.old.js | 4 +- .../src/ReactTestHostConfig.js | 2 +- 10 files changed, 171 insertions(+), 12 deletions(-) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index 459f714b90cf0..43a563e90aa23 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -467,7 +467,7 @@ export function makeClientIdInDEV(warnOnAccessInDEV: () => void): OpaqueIDType { throw new Error('Not yet implemented'); } -export function beforeActiveInstanceBlur() { +export function beforeActiveInstanceBlur(internalInstanceHandle: Object) { // noop } diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 63fdb08a3ae84..8de2f2c5c7ca4 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -232,10 +232,13 @@ export function prepareForCommit(containerInfo: Container): Object | null { return activeInstance; } -export function beforeActiveInstanceBlur(): void { +export function beforeActiveInstanceBlur(internalInstanceHandle: Object): void { if (enableCreateEventHandleAPI) { ReactBrowserEventEmitterSetEnabled(true); - dispatchBeforeDetachedBlur((selectionInformation: any).focusedElem); + dispatchBeforeDetachedBlur( + (selectionInformation: any).focusedElem, + internalInstanceHandle, + ); ReactBrowserEventEmitterSetEnabled(false); } } @@ -499,12 +502,17 @@ function createEvent(type: DOMEventName, bubbles: boolean): Event { return event; } -function dispatchBeforeDetachedBlur(target: HTMLElement): void { +function dispatchBeforeDetachedBlur( + target: HTMLElement, + internalInstanceHandle: Object, +): void { if (enableCreateEventHandleAPI) { const event = createEvent('beforeblur', true); // Dispatch "beforeblur" directly on the target, // so it gets picked up by the event system and // can propagate through the React internal tree. + // $FlowFixMe: internal field + event._detachedInterceptFiber = internalInstanceHandle; target.dispatchEvent(event); } } diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js index 74b95dea3d4eb..0afb45d3f844e 100644 --- a/packages/react-dom/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMPluginEventSystem.js @@ -658,10 +658,11 @@ export function accumulateSinglePhaseListeners( nativeEventType: string, inCapturePhase: boolean, accumulateTargetOnly: boolean, + nativeEvent: AnyNativeEvent, ): Array { const captureName = reactName !== null ? reactName + 'Capture' : null; const reactEventName = inCapturePhase ? captureName : reactName; - const listeners: Array = []; + let listeners: Array = []; let instance = targetFiber; let lastHostComponent = null; @@ -740,6 +741,20 @@ export function accumulateSinglePhaseListeners( if (accumulateTargetOnly) { break; } + // If we are processing the onBeforeBlur event, then we need to take + // into consideration that part of the React tree might have been hidden + // or deleted (as we're invoking this event during commit). We can find + // this out by checking if intercept fiber set on the event matches the + // current instance fiber. In which case, we should clear all existing + // listeners. + if ( + enableCreateEventHandleAPI && + nativeEvent.type === 'beforeblur' && + // $FlowFixMe: internal field + nativeEvent._detachedInterceptFiber === instance + ) { + listeners = []; + } instance = instance.return; } return listeners; diff --git a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js index 6a3662962a740..50645bb2020d5 100644 --- a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js +++ b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js @@ -2424,6 +2424,60 @@ describe('DOMPluginEventSystem', () => { expect(log).toEqual(['beforeblur', 'afterblur']); }); + // @gate experimental + it('beforeblur has the correct propagation mechancis after a nested focused element is unmounted', () => { + const onBeforeBlur = jest.fn(); + const innerRef = React.createRef(); + const innerRef2 = React.createRef(); + const setBeforeBlurHandle = ReactDOM.unstable_createEventHandle( + 'beforeblur', + ); + const ref2 = React.createRef(); + + const Component = ({show}) => { + const ref = React.useRef(null); + + React.useEffect(() => { + const clear1 = setBeforeBlurHandle(ref.current, onBeforeBlur); + let clear2; + if (ref2.current) { + clear2 = setBeforeBlurHandle(ref2.current, onBeforeBlur); + } + + return () => { + clear1(); + if (clear2) { + clear2(); + } + }; + }); + + return ( +
+ {show && ( +
+ +
+ )} +
+
+ ); + }; + + ReactDOM.render(, container); + Scheduler.unstable_flushAll(); + + const inner = innerRef.current; + const target = createEventTarget(inner); + target.focus(); + expect(onBeforeBlur).toHaveBeenCalledTimes(0); + + ReactDOM.render(, container); + Scheduler.unstable_flushAll(); + + expect(onBeforeBlur).toHaveBeenCalledTimes(1); + }); + // @gate experimental it('beforeblur and afterblur are called after a focused element is suspended', () => { const log = []; @@ -2510,6 +2564,87 @@ describe('DOMPluginEventSystem', () => { document.body.removeChild(container2); }); + // @gate experimental + it('beforeblur has the correct propagation mechancis after a nested focused element is suspended', () => { + const onBeforeBlur = jest.fn(); + const innerRef = React.createRef(); + const innerRef2 = React.createRef(); + const setBeforeBlurHandle = ReactDOM.unstable_createEventHandle( + 'beforeblur', + ); + const ref2 = React.createRef(); + const Suspense = React.Suspense; + let suspend = false; + let resolve; + const promise = new Promise( + resolvePromise => (resolve = resolvePromise), + ); + + function Child() { + if (suspend) { + throw promise; + } else { + return ; + } + } + + const Component = () => { + const ref = React.useRef(null); + + React.useEffect(() => { + const clear1 = setBeforeBlurHandle(ref.current, onBeforeBlur); + let clear2; + if (ref2.current) { + clear2 = setBeforeBlurHandle(ref2.current, onBeforeBlur); + } + + return () => { + clear1(); + if (clear2) { + clear2(); + } + }; + }); + + return ( +
+ +
+ +
+ +
+
+ ); + }; + + const container2 = document.createElement('div'); + document.body.appendChild(container2); + + const root = ReactDOM.createRoot(container2); + + act(() => { + root.render(); + }); + jest.runAllTimers(); + + const inner = innerRef.current; + const target = createEventTarget(inner); + target.focus(); + expect(onBeforeBlur).toHaveBeenCalledTimes(0); + + suspend = true; + act(() => { + root.render(); + }); + jest.runAllTimers(); + + expect(onBeforeBlur).toHaveBeenCalledTimes(1); + resolve(); + + document.body.removeChild(container2); + }); + // @gate experimental it('regression: does not fire beforeblur/afterblur if target is already hidden', () => { const Suspense = React.Suspense; diff --git a/packages/react-dom/src/events/plugins/SimpleEventPlugin.js b/packages/react-dom/src/events/plugins/SimpleEventPlugin.js index fafccff92014c..03bf9484b2c04 100644 --- a/packages/react-dom/src/events/plugins/SimpleEventPlugin.js +++ b/packages/react-dom/src/events/plugins/SimpleEventPlugin.js @@ -200,6 +200,7 @@ function extractEvents( nativeEvent.type, inCapturePhase, accumulateTargetOnly, + nativeEvent, ); if (listeners.length > 0) { // Intentionally create event lazily. diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index dcdceb7a60dbd..636542e8d426b 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -481,7 +481,7 @@ export function makeClientIdInDEV(warnOnAccessInDEV: () => void): OpaqueIDType { throw new Error('Not yet implemented'); } -export function beforeActiveInstanceBlur() { +export function beforeActiveInstanceBlur(internalInstanceHandle: Object) { // noop } diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index 0369755c136a0..eba0a2b90462b 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -534,7 +534,7 @@ export function makeClientIdInDEV(warnOnAccessInDEV: () => void): OpaqueIDType { throw new Error('Not yet implemented'); } -export function beforeActiveInstanceBlur() { +export function beforeActiveInstanceBlur(internalInstanceHandle: Object) { // noop } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 4f40bde182310..b844413196ad4 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2172,7 +2172,7 @@ function commitBeforeMutationEffectsImpl(fiber: Fiber) { doesFiberContain(fiber, focusedInstanceHandle) ) { shouldFireAfterActiveInstanceBlur = true; - beforeActiveInstanceBlur(); + beforeActiveInstanceBlur(current); } } @@ -2206,7 +2206,7 @@ function commitBeforeMutationEffectsDeletions(deletions: Array) { if (doesFiberContain(fiber, ((focusedInstanceHandle: any): Fiber))) { shouldFireAfterActiveInstanceBlur = true; - beforeActiveInstanceBlur(); + beforeActiveInstanceBlur(fiber); } } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index c0fcb9c5c09b2..22bdb9fe0f967 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2261,7 +2261,7 @@ function commitBeforeMutationEffects() { if ((nextEffect.flags & Deletion) !== NoFlags) { if (doesFiberContain(nextEffect, focusedInstanceHandle)) { shouldFireAfterActiveInstanceBlur = true; - beforeActiveInstanceBlur(); + beforeActiveInstanceBlur(nextEffect); } } else { // TODO: Move this out of the hot path using a dedicated effect tag. @@ -2271,7 +2271,7 @@ function commitBeforeMutationEffects() { doesFiberContain(nextEffect, focusedInstanceHandle) ) { shouldFireAfterActiveInstanceBlur = true; - beforeActiveInstanceBlur(); + beforeActiveInstanceBlur(current); } } } diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index b244810736d8a..a6d661d5fe9cc 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -377,7 +377,7 @@ export function makeOpaqueHydratingObject( }; } -export function beforeActiveInstanceBlur() { +export function beforeActiveInstanceBlur(internalInstanceHandle: Object) { // noop } From 000b0fe678d44b0d7fa2bf0af98fa49c61ab84cb Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 14 Oct 2020 18:55:51 +0100 Subject: [PATCH 2/2] Address feedback Address feedback --- .../react-dom/src/events/DOMPluginEventSystem.js | 15 +++++++++------ .../DOMPluginEventSystem-test.internal.js | 4 ++-- .../src/ReactFiberWorkLoop.new.js | 2 +- .../src/ReactFiberWorkLoop.old.js | 2 +- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js index 0afb45d3f844e..c43d96dcb1c9c 100644 --- a/packages/react-dom/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMPluginEventSystem.js @@ -747,13 +747,16 @@ export function accumulateSinglePhaseListeners( // this out by checking if intercept fiber set on the event matches the // current instance fiber. In which case, we should clear all existing // listeners. - if ( - enableCreateEventHandleAPI && - nativeEvent.type === 'beforeblur' && + if (enableCreateEventHandleAPI && nativeEvent.type === 'beforeblur') { // $FlowFixMe: internal field - nativeEvent._detachedInterceptFiber === instance - ) { - listeners = []; + const detachedInterceptFiber = nativeEvent._detachedInterceptFiber; + if ( + detachedInterceptFiber !== null && + (detachedInterceptFiber === instance || + detachedInterceptFiber === instance.alternate) + ) { + listeners = []; + } } instance = instance.return; } diff --git a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js index 50645bb2020d5..004f0bb631f31 100644 --- a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js +++ b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js @@ -2425,7 +2425,7 @@ describe('DOMPluginEventSystem', () => { }); // @gate experimental - it('beforeblur has the correct propagation mechancis after a nested focused element is unmounted', () => { + it('beforeblur should skip handlers from a deleted subtree after the focused element is unmounted', () => { const onBeforeBlur = jest.fn(); const innerRef = React.createRef(); const innerRef2 = React.createRef(); @@ -2565,7 +2565,7 @@ describe('DOMPluginEventSystem', () => { }); // @gate experimental - it('beforeblur has the correct propagation mechancis after a nested focused element is suspended', () => { + it('beforeblur should skip handlers from a deleted subtree after the focused element is suspended', () => { const onBeforeBlur = jest.fn(); const innerRef = React.createRef(); const innerRef2 = React.createRef(); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index b844413196ad4..fffa71d20801b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2172,7 +2172,7 @@ function commitBeforeMutationEffectsImpl(fiber: Fiber) { doesFiberContain(fiber, focusedInstanceHandle) ) { shouldFireAfterActiveInstanceBlur = true; - beforeActiveInstanceBlur(current); + beforeActiveInstanceBlur(fiber); } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 22bdb9fe0f967..cece268e3d017 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2271,7 +2271,7 @@ function commitBeforeMutationEffects() { doesFiberContain(nextEffect, focusedInstanceHandle) ) { shouldFireAfterActiveInstanceBlur = true; - beforeActiveInstanceBlur(current); + beforeActiveInstanceBlur(nextEffect); } } }