From ce493bf314bd14e4a7d0ada3e4e2c6ad150af633 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Oct 2020 09:57:28 +0100 Subject: [PATCH 1/8] Remove dead code branch This function is only called when initializing roots/containers (where we skip non-delegated events) and in the createEventHandle path for non-DOM nodes (where we never hit this path because targetElement is null). --- .../src/events/DOMPluginEventSystem.js | 27 +------------------ 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js index 74b95dea3d4e..01651ae0524b 100644 --- a/packages/react-dom/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMPluginEventSystem.js @@ -363,37 +363,12 @@ export function listenToNativeEvent( ) { target = (rootContainerElement: any).ownerDocument; } - // If the event can be delegated (or is capture phase), we can - // register it to the root container. Otherwise, we should - // register the event to the target element and mark it as - // a non-delegated event. - if ( - targetElement !== null && - !isCapturePhaseListener && - nonDelegatedEvents.has(domEventName) - ) { - // For all non-delegated events, apart from scroll, we attach - // their event listeners to the respective elements that their - // events fire on. That means we can skip this step, as event - // listener has already been added previously. However, we - // special case the scroll event because the reality is that any - // element can scroll. - // TODO: ideally, we'd eventually apply the same logic to all - // events from the nonDelegatedEvents list. Then we can remove - // this special case and use the same logic for all events. - if (domEventName !== 'scroll') { - return; - } - eventSystemFlags |= IS_NON_DELEGATED; - target = targetElement; - } + const listenerSet = getEventListenerSet(target); const listenerSetKey = getListenerSetKey( domEventName, isCapturePhaseListener, ); - // If the listener entry is empty or we should upgrade, then - // we need to trap an event listener onto the target. if (!listenerSet.has(listenerSetKey)) { if (isCapturePhaseListener) { eventSystemFlags |= IS_CAPTURE_PHASE; From 9b2fe9e3474fd3cd45b7aaf60be2a76f324c48d4 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Oct 2020 14:24:57 +0100 Subject: [PATCH 2/8] Move related functions close to each other --- .../src/events/DOMPluginEventSystem.js | 66 +++++++++---------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js index 01651ae0524b..a5c7f1842614 100644 --- a/packages/react-dom/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMPluginEventSystem.js @@ -312,39 +312,6 @@ export function listenToNonDelegatedEvent( } } -const listeningMarker = - '_reactListening' + - Math.random() - .toString(36) - .slice(2); - -export function listenToAllSupportedEvents(rootContainerElement: EventTarget) { - if ((rootContainerElement: any)[listeningMarker]) { - // Performance optimization: don't iterate through events - // for the same portal container or root node more than once. - // TODO: once we remove the flag, we may be able to also - // remove some of the bookkeeping maps used for laziness. - return; - } - (rootContainerElement: any)[listeningMarker] = true; - allNativeEvents.forEach(domEventName => { - if (!nonDelegatedEvents.has(domEventName)) { - listenToNativeEvent( - domEventName, - false, - ((rootContainerElement: any): Element), - null, - ); - } - listenToNativeEvent( - domEventName, - true, - ((rootContainerElement: any): Element), - null, - ); - }); -} - export function listenToNativeEvent( domEventName: DOMEventName, isCapturePhaseListener: boolean, @@ -383,6 +350,39 @@ export function listenToNativeEvent( } } +const listeningMarker = + '_reactListening' + + Math.random() + .toString(36) + .slice(2); + +export function listenToAllSupportedEvents(rootContainerElement: EventTarget) { + if ((rootContainerElement: any)[listeningMarker]) { + // Performance optimization: don't iterate through events + // for the same portal container or root node more than once. + // TODO: once we remove the flag, we may be able to also + // remove some of the bookkeeping maps used for laziness. + return; + } + (rootContainerElement: any)[listeningMarker] = true; + allNativeEvents.forEach(domEventName => { + if (!nonDelegatedEvents.has(domEventName)) { + listenToNativeEvent( + domEventName, + false, + ((rootContainerElement: any): Element), + null, + ); + } + listenToNativeEvent( + domEventName, + true, + ((rootContainerElement: any): Element), + null, + ); + }); +} + function addTrappedEventListener( targetContainer: EventTarget, domEventName: DOMEventName, From ed66a38c624508e6c54306b69a659f1a0a8041d3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Oct 2020 14:29:01 +0100 Subject: [PATCH 3/8] Fork listenToNativeEvent for createEventHandle It doesn't need all of the logic that's needed for normal event path. And the normal codepath doesn't use the last two arguments. --- .../src/client/ReactDOMEventHandle.js | 8 ++--- .../src/events/DOMPluginEventSystem.js | 32 ++++++++++++++++--- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMEventHandle.js b/packages/react-dom/src/client/ReactDOMEventHandle.js index 69a2eb4851f1..80834c32834d 100644 --- a/packages/react-dom/src/client/ReactDOMEventHandle.js +++ b/packages/react-dom/src/client/ReactDOMEventHandle.js @@ -22,9 +22,7 @@ import { addEventHandleToTarget, } from './ReactDOMComponentTree'; import {ELEMENT_NODE} from '../shared/HTMLNodeType'; -import {listenToNativeEvent} from '../events/DOMPluginEventSystem'; - -import {IS_EVENT_HANDLE_NON_MANAGED_NODE} from '../events/EventSystemFlags'; +import {listenToNativeEventForNonManagedEventTarget} from '../events/DOMPluginEventSystem'; import { enableScopeAPI, @@ -69,12 +67,10 @@ function registerReactDOMEvent( const eventTarget = ((target: any): EventTarget); // These are valid event targets, but they are also // non-managed React nodes. - listenToNativeEvent( + listenToNativeEventForNonManagedEventTarget( domEventName, isCapturePhaseListener, eventTarget, - null, - IS_EVENT_HANDLE_NON_MANAGED_NODE, ); } else { invariant( diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js index a5c7f1842614..fa8b03788328 100644 --- a/packages/react-dom/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMPluginEventSystem.js @@ -316,9 +316,8 @@ export function listenToNativeEvent( domEventName: DOMEventName, isCapturePhaseListener: boolean, rootContainerElement: EventTarget, - targetElement: Element | null, - eventSystemFlags?: EventSystemFlags = 0, ): void { + let eventSystemFlags = 0; let target = rootContainerElement; // selectionchange needs to be attached to the document @@ -350,6 +349,33 @@ export function listenToNativeEvent( } } +// This is only used by createEventHandle when the +// target is not a DOM element. E.g. window. +export function listenToNativeEventForNonManagedEventTarget( + domEventName: DOMEventName, + isCapturePhaseListener: boolean, + target: EventTarget, +): void { + let eventSystemFlags = IS_EVENT_HANDLE_NON_MANAGED_NODE; + const listenerSet = getEventListenerSet(target); + const listenerSetKey = getListenerSetKey( + domEventName, + isCapturePhaseListener, + ); + if (!listenerSet.has(listenerSetKey)) { + if (isCapturePhaseListener) { + eventSystemFlags |= IS_CAPTURE_PHASE; + } + addTrappedEventListener( + target, + domEventName, + eventSystemFlags, + isCapturePhaseListener, + ); + listenerSet.add(listenerSetKey); + } +} + const listeningMarker = '_reactListening' + Math.random() @@ -371,14 +397,12 @@ export function listenToAllSupportedEvents(rootContainerElement: EventTarget) { domEventName, false, ((rootContainerElement: any): Element), - null, ); } listenToNativeEvent( domEventName, true, ((rootContainerElement: any): Element), - null, ); }); } From 0c7bdbf09dd63374ad4ce40409460e3ff25861a3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Oct 2020 14:50:08 +0100 Subject: [PATCH 4/8] Expand test coverage for non-delegated events This changes a test to fail if we removed the event handler Sets. Previously, we didn't cover that. --- .../__tests__/ReactDOMEventListener-test.js | 82 +++++++++++++++++-- 1 file changed, 76 insertions(+), 6 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index a6918c69197e..c4857ce4841e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -820,19 +820,56 @@ describe('ReactDOMEventListener', () => { , container, ); + + // Update to attach. ReactDOM.render(
+ onScroll={e => onScroll(e)} + onScrollCapture={e => onScrollCapture(e)}>
+ onScroll={e => onScroll(e)} + onScrollCapture={e => onScrollCapture(e)}>
onScroll(e)} + onScrollCapture={e => onScrollCapture(e)} + ref={ref} + /> +
+
, + container, + ); + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + expect(log).toEqual([ + ['capture', 'grand'], + ['capture', 'parent'], + ['capture', 'child'], + ['bubble', 'child'], + ]); + + // Update to verify deduplication. + log.length = 0; + ReactDOM.render( +
onScroll(e)} + onScrollCapture={e => onScrollCapture(e)}> +
onScroll(e)} + onScrollCapture={e => onScrollCapture(e)}> +
onScroll(e)} + onScrollCapture={e => onScrollCapture(e)} ref={ref} />
@@ -850,6 +887,23 @@ describe('ReactDOMEventListener', () => { ['capture', 'child'], ['bubble', 'child'], ]); + + // Update to detach. + log.length = 0; + ReactDOM.render( +
+
+
+
+
, + container, + ); + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + expect(log).toEqual([]); } finally { document.body.removeChild(container); } @@ -899,6 +953,22 @@ describe('ReactDOMEventListener', () => { ['capture', 'child'], ['bubble', 'child'], ]); + + log.length = 0; + ReactDOM.render( +
+
+
+
+
, + container, + ); + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + expect(log).toEqual([]); } finally { document.body.removeChild(container); } From 59c8e7b449e8bb367c2820444e60987a2b5fa5be Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Oct 2020 15:02:51 +0100 Subject: [PATCH 5/8] Add DEV-level check that top-level events and non-delegated events do not overlap This makes us confident that they're mutually exclusive and there is no duplication between them. --- .../src/events/DOMPluginEventSystem.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js index fa8b03788328..3c9db6ae4397 100644 --- a/packages/react-dom/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMPluginEventSystem.js @@ -295,6 +295,15 @@ export function listenToNonDelegatedEvent( domEventName: DOMEventName, targetElement: Element, ): void { + if (__DEV__) { + if (!nonDelegatedEvents.has(domEventName)) { + console.error( + 'Did not expect a listenToNonDelegatedEvent() call for "%s". ' + + 'This is a bug in React. Please file an issue.', + domEventName, + ); + } + } const isCapturePhaseListener = false; const listenerSet = getEventListenerSet(targetElement); const listenerSetKey = getListenerSetKey( @@ -317,6 +326,16 @@ export function listenToNativeEvent( isCapturePhaseListener: boolean, rootContainerElement: EventTarget, ): void { + if (__DEV__) { + if (nonDelegatedEvents.has(domEventName) && !isCapturePhaseListener) { + console.error( + 'Did not expect a listenToNativeEvent() call for "%s" in the bubble phase. ' + + 'This is a bug in React. Please file an issue.', + domEventName, + ); + } + } + let eventSystemFlags = 0; let target = rootContainerElement; From c3decdcde96abc52750ea915087df9821135b657 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Oct 2020 15:24:17 +0100 Subject: [PATCH 6/8] Add a test verifying selectionchange deduplication This is why we still need the Set bookkeeping. Adding a test for it. --- .../__tests__/ReactDOMEventListener-test.js | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index c4857ce4841e..857135ad6677 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -973,4 +973,29 @@ describe('ReactDOMEventListener', () => { document.body.removeChild(container); } }); + + it('should not subscribe to selectionchange twice', () => { + const log = []; + + const originalDocAddEventListener = document.addEventListener; + document.addEventListener = function(type, fn, options) { + switch (type) { + case 'selectionchange': + log.push(options); + break; + default: + throw new Error( + `Did not expect to add a document-level listener for the "${type}" event.`, + ); + } + }; + try { + ReactDOM.render(, document.createElement('div')); + ReactDOM.render(, document.createElement('div')); + } finally { + document.addEventListener = originalDocAddEventListener; + } + + expect(log).toEqual([false, true]); + }); }); From eef4402b574734695113d1b683a1b2ad254058cd Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Oct 2020 15:44:50 +0100 Subject: [PATCH 7/8] Remove Set bookkeeping for root events Root events don't intersect with non-delegated bubbled events (so no need to deduplicate there). They also don't intersect with createEventHandle non-managed events (because those don't go on the DOM elements). So we can remove the bookeeping because we already have code ensuring the eager subscriptions only run once per element. I've moved the selectionchange special case outside, and added document-level deduplication for it alone. Technically this might change the behavior of createEventHandle with selectionchange on the document, but we're not using that, and I'm not sure that behavior makes sense anyway. --- .../__tests__/ReactDOMEventListener-test.js | 2 +- .../src/events/DOMPluginEventSystem.js | 77 ++++++++----------- 2 files changed, 31 insertions(+), 48 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index 857135ad6677..42e73d58f154 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -996,6 +996,6 @@ describe('ReactDOMEventListener', () => { document.addEventListener = originalDocAddEventListener; } - expect(log).toEqual([false, true]); + expect(log).toEqual([false]); }); }); diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js index 3c9db6ae4397..0545c2bbd89f 100644 --- a/packages/react-dom/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMPluginEventSystem.js @@ -324,7 +324,7 @@ export function listenToNonDelegatedEvent( export function listenToNativeEvent( domEventName: DOMEventName, isCapturePhaseListener: boolean, - rootContainerElement: EventTarget, + target: EventTarget, ): void { if (__DEV__) { if (nonDelegatedEvents.has(domEventName) && !isCapturePhaseListener) { @@ -337,35 +337,15 @@ export function listenToNativeEvent( } let eventSystemFlags = 0; - let target = rootContainerElement; - - // selectionchange needs to be attached to the document - // otherwise it won't capture incoming events that are only - // triggered on the document directly. - if ( - domEventName === 'selectionchange' && - (rootContainerElement: any).nodeType !== DOCUMENT_NODE - ) { - target = (rootContainerElement: any).ownerDocument; + if (isCapturePhaseListener) { + eventSystemFlags |= IS_CAPTURE_PHASE; } - - const listenerSet = getEventListenerSet(target); - const listenerSetKey = getListenerSetKey( + addTrappedEventListener( + target, domEventName, + eventSystemFlags, isCapturePhaseListener, ); - if (!listenerSet.has(listenerSetKey)) { - if (isCapturePhaseListener) { - eventSystemFlags |= IS_CAPTURE_PHASE; - } - addTrappedEventListener( - target, - domEventName, - eventSystemFlags, - isCapturePhaseListener, - ); - listenerSet.add(listenerSetKey); - } } // This is only used by createEventHandle when the @@ -402,28 +382,31 @@ const listeningMarker = .slice(2); export function listenToAllSupportedEvents(rootContainerElement: EventTarget) { - if ((rootContainerElement: any)[listeningMarker]) { - // Performance optimization: don't iterate through events - // for the same portal container or root node more than once. - // TODO: once we remove the flag, we may be able to also - // remove some of the bookkeeping maps used for laziness. - return; - } - (rootContainerElement: any)[listeningMarker] = true; - allNativeEvents.forEach(domEventName => { - if (!nonDelegatedEvents.has(domEventName)) { - listenToNativeEvent( - domEventName, - false, - ((rootContainerElement: any): Element), - ); + if (!(rootContainerElement: any)[listeningMarker]) { + (rootContainerElement: any)[listeningMarker] = true; + allNativeEvents.forEach(domEventName => { + // We handle selectionchange separately because it + // doesn't bubble and needs to be on the document. + if (domEventName !== 'selectionchange') { + if (!nonDelegatedEvents.has(domEventName)) { + listenToNativeEvent(domEventName, false, rootContainerElement); + } + listenToNativeEvent(domEventName, true, rootContainerElement); + } + }); + const ownerDocument = + (rootContainerElement: any).nodeType === DOCUMENT_NODE + ? rootContainerElement + : rootContainerElement.ownerDocument; + if (ownerDocument !== null) { + // The selectionchange event also needs deduplication + // but it is attached to the document. + if (!(ownerDocument: any)[listeningMarker]) { + (ownerDocument: any)[listeningMarker] = true; + listenToNativeEvent('selectionchange', false, ownerDocument); + } } - listenToNativeEvent( - domEventName, - true, - ((rootContainerElement: any): Element), - ); - }); + } } function addTrappedEventListener( From a286fc2a7a6370f59c92e4863b2519e44d616cc3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Oct 2020 15:58:56 +0100 Subject: [PATCH 8/8] Flow --- packages/react-dom/src/events/DOMPluginEventSystem.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js index 0545c2bbd89f..42673a2d4050 100644 --- a/packages/react-dom/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMPluginEventSystem.js @@ -397,7 +397,7 @@ export function listenToAllSupportedEvents(rootContainerElement: EventTarget) { const ownerDocument = (rootContainerElement: any).nodeType === DOCUMENT_NODE ? rootContainerElement - : rootContainerElement.ownerDocument; + : (rootContainerElement: any).ownerDocument; if (ownerDocument !== null) { // The selectionchange event also needs deduplication // but it is attached to the document.