From cddd7473d8ca434f69f1937ce35730e1c8623236 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 16 Oct 2020 16:49:41 +0100 Subject: [PATCH] Remove Set bookkeeping for root events (#19990) * 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). * Move related functions close to each other * 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. * 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. * 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. * Add a test verifying selectionchange deduplication This is why we still need the Set bookkeeping. Adding a test for it. * 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. * Flow --- .../__tests__/ReactDOMEventListener-test.js | 107 +++++++++++++- .../src/client/ReactDOMEventHandle.js | 8 +- .../src/events/DOMPluginEventSystem.js | 139 +++++++++--------- 3 files changed, 173 insertions(+), 81 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index a6918c69197e1..42e73d58f1546 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -820,19 +820,21 @@ 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} />
@@ -850,6 +852,58 @@ describe('ReactDOMEventListener', () => { ['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} + /> +
+
, + container, + ); + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + expect(log).toEqual([ + ['capture', 'grand'], + ['capture', 'parent'], + ['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,8 +953,49 @@ 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); } }); + + 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]); + }); }); diff --git a/packages/react-dom/src/client/ReactDOMEventHandle.js b/packages/react-dom/src/client/ReactDOMEventHandle.js index 69a2eb4851f1d..80834c32834d2 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 c43d96dcb1c9c..874fa394e5c48 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( @@ -312,88 +321,46 @@ 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( +export function listenToNativeEvent( + domEventName: DOMEventName, + isCapturePhaseListener: boolean, + target: 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, - false, - ((rootContainerElement: any): Element), - null, ); } - listenToNativeEvent( - domEventName, - true, - ((rootContainerElement: any): Element), - null, - ); - }); + } + + let eventSystemFlags = 0; + if (isCapturePhaseListener) { + eventSystemFlags |= IS_CAPTURE_PHASE; + } + addTrappedEventListener( + target, + domEventName, + eventSystemFlags, + isCapturePhaseListener, + ); } -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, - rootContainerElement: EventTarget, - targetElement: Element | null, - eventSystemFlags?: EventSystemFlags = 0, + target: EventTarget, ): void { - 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 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; - } + let eventSystemFlags = IS_EVENT_HANDLE_NON_MANAGED_NODE; 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; @@ -408,6 +375,40 @@ export function listenToNativeEvent( } } +const listeningMarker = + '_reactListening' + + Math.random() + .toString(36) + .slice(2); + +export function listenToAllSupportedEvents(rootContainerElement: EventTarget) { + 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: any).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); + } + } + } +} + function addTrappedEventListener( targetContainer: EventTarget, domEventName: DOMEventName,