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
Dan Abramov committed
Oct 16, 2020 at 16:49 UTC
e9f5ad2584ddb0d42a96045c9b23d0b4d9311331
3 files changed
+173
-81
packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
+101
-6
@@ -820,19 +820,21 @@ describe('ReactDOMEventListener', () => {
820
</div>,
821
container,
822
);
823
+
824
+ // Update to attach.
825
ReactDOM.render(
826
<div
827
className="grand"
826
- onScroll={onScroll}
827
- onScrollCapture={onScrollCapture}>
828
+ onScroll={e => onScroll(e)}
829
+ onScrollCapture={e => onScrollCapture(e)}>
830
<div
831
className="parent"
830
- onScroll={onScroll}
831
- onScrollCapture={onScrollCapture}>
832
+ onScroll={e => onScroll(e)}
833
+ onScrollCapture={e => onScrollCapture(e)}>
834
<div
835
className="child"
834
- onScroll={onScroll}
835
- onScrollCapture={onScrollCapture}
836
+ onScroll={e => onScroll(e)}
837
+ onScrollCapture={e => onScrollCapture(e)}
838
ref={ref}
839
/>
840
</div>
@@ -850,6 +852,58 @@ describe('ReactDOMEventListener', () => {
852
['capture', 'child'],
853
['bubble', 'child'],
854
]);
855
+
856
+ // Update to verify deduplication.
857
+ log.length = 0;
858
+ ReactDOM.render(
859
+ <div
860
+ className="grand"
861
+ // Note: these are intentionally inline functions so that
862
+ // we hit the reattachment codepath instead of bailing out.
863
+ onScroll={e => onScroll(e)}
864
+ onScrollCapture={e => onScrollCapture(e)}>
865
+ <div
866
+ className="parent"
867
+ onScroll={e => onScroll(e)}
868
+ onScrollCapture={e => onScrollCapture(e)}>
869
+ <div
870
+ className="child"
871
+ onScroll={e => onScroll(e)}
872
+ onScrollCapture={e => onScrollCapture(e)}
873
+ ref={ref}
874
+ />
875
+ </div>
876
+ </div>,
877
+ container,
878
+ );
879
+ ref.current.dispatchEvent(
880
+ new Event('scroll', {
881
+ bubbles: false,
882
+ }),
883
+ );
884
+ expect(log).toEqual([
885
+ ['capture', 'grand'],
886
+ ['capture', 'parent'],
887
+ ['capture', 'child'],
888
+ ['bubble', 'child'],
889
+ ]);
890
+
891
+ // Update to detach.
892
+ log.length = 0;
893
+ ReactDOM.render(
894
+ <div>
895
+ <div>
896
+ <div ref={ref} />
897
+ </div>
898
+ </div>,
899
+ container,
900
+ );
901
+ ref.current.dispatchEvent(
902
+ new Event('scroll', {
903
+ bubbles: false,
904
+ }),
905
+ );
906
+ expect(log).toEqual([]);
907
} finally {
908
document.body.removeChild(container);
909
}
@@ -899,8 +953,49 @@ describe('ReactDOMEventListener', () => {
953
['capture', 'child'],
954
['bubble', 'child'],
955
]);
956
+
957
+ log.length = 0;
958
+ ReactDOM.render(
959
+ <div>
960
+ <div>
961
+ <div ref={ref} />
962
+ </div>
963
+ </div>,
964
+ container,
965
+ );
966
+ ref.current.dispatchEvent(
967
+ new Event('scroll', {
968
+ bubbles: false,
969
+ }),
970
+ );
971
+ expect(log).toEqual([]);
972
} finally {
973
document.body.removeChild(container);
974
}
975
});
976
+
977
+ it('should not subscribe to selectionchange twice', () => {
978
+ const log = [];
979
+
980
+ const originalDocAddEventListener = document.addEventListener;
981
+ document.addEventListener = function(type, fn, options) {
982
+ switch (type) {
983
+ case 'selectionchange':
984
+ log.push(options);
985
+ break;
986
+ default:
987
+ throw new Error(
988
+ `Did not expect to add a document-level listener for the "${type}" event.`,
989
+ );
990
+ }
991
+ };
992
+ try {
993
+ ReactDOM.render(<input />, document.createElement('div'));
994
+ ReactDOM.render(<input />, document.createElement('div'));
995
+ } finally {
996
+ document.addEventListener = originalDocAddEventListener;
997
+ }
998
+
999
+ expect(log).toEqual([false]);
1000
+ });
1001
});
packages/react-dom/src/client/ReactDOMEventHandle.js
+2
-6
@@ -22,9 +22,7 @@ import {
22
addEventHandleToTarget,
23
} from './ReactDOMComponentTree';
24
import {ELEMENT_NODE} from '../shared/HTMLNodeType';
25
-import {listenToNativeEvent} from '../events/DOMPluginEventSystem';
26
-
27
-import {IS_EVENT_HANDLE_NON_MANAGED_NODE} from '../events/EventSystemFlags';
25
+import {listenToNativeEventForNonManagedEventTarget} from '../events/DOMPluginEventSystem';
26
27
import {
28
enableScopeAPI,
@@ -69,12 +67,10 @@ function registerReactDOMEvent(
67
const eventTarget = ((target: any): EventTarget);
68
// These are valid event targets, but they are also
69
// non-managed React nodes.
72
- listenToNativeEvent(
70
+ listenToNativeEventForNonManagedEventTarget(
71
domEventName,
72
isCapturePhaseListener,
73
eventTarget,
76
- null,
77
- IS_EVENT_HANDLE_NON_MANAGED_NODE,
74
);
75
} else {
76
invariant(
packages/react-dom/src/events/DOMPluginEventSystem.js
+70
-69
@@ -295,6 +295,15 @@ export function listenToNonDelegatedEvent(
295
domEventName: DOMEventName,
296
targetElement: Element,
297
): void {
298
+ if (__DEV__) {
299
+ if (!nonDelegatedEvents.has(domEventName)) {
300
+ console.error(
301
+ 'Did not expect a listenToNonDelegatedEvent() call for "%s". ' +
302
+ 'This is a bug in React. Please file an issue.',
303
+ domEventName,
304
+ );
305
+ }
306
+ }
307
const isCapturePhaseListener = false;
308
const listenerSet = getEventListenerSet(targetElement);
309
const listenerSetKey = getListenerSetKey(
@@ -312,88 +321,46 @@ export function listenToNonDelegatedEvent(
321
}
322
}
323
315
-const listeningMarker =
316
- '_reactListening' +
317
- Math.random()
318
- .toString(36)
319
- .slice(2);
320
-
321
-export function listenToAllSupportedEvents(rootContainerElement: EventTarget) {
322
- if ((rootContainerElement: any)[listeningMarker]) {
323
- // Performance optimization: don't iterate through events
324
- // for the same portal container or root node more than once.
325
- // TODO: once we remove the flag, we may be able to also
326
- // remove some of the bookkeeping maps used for laziness.
327
- return;
328
- }
329
- (rootContainerElement: any)[listeningMarker] = true;
330
- allNativeEvents.forEach(domEventName => {
331
- if (!nonDelegatedEvents.has(domEventName)) {
332
- listenToNativeEvent(
324
+export function listenToNativeEvent(
325
+ domEventName: DOMEventName,
326
+ isCapturePhaseListener: boolean,
327
+ target: EventTarget,
328
+): void {
329
+ if (__DEV__) {
330
+ if (nonDelegatedEvents.has(domEventName) && !isCapturePhaseListener) {
331
+ console.error(
332
+ 'Did not expect a listenToNativeEvent() call for "%s" in the bubble phase. ' +
333
+ 'This is a bug in React. Please file an issue.',
334
domEventName,
334
- false,
335
- ((rootContainerElement: any): Element),
336
- null,
335
);
336
}
339
- listenToNativeEvent(
340
- domEventName,
341
- true,
342
- ((rootContainerElement: any): Element),
343
- null,
344
- );
345
- });
337
+ }
338
+
339
+ let eventSystemFlags = 0;
340
+ if (isCapturePhaseListener) {
341
+ eventSystemFlags |= IS_CAPTURE_PHASE;
342
+ }
343
+ addTrappedEventListener(
344
+ target,
345
+ domEventName,
346
+ eventSystemFlags,
347
+ isCapturePhaseListener,
348
+ );
349
}
350
348
-export function listenToNativeEvent(
351
+// This is only used by createEventHandle when the
352
+// target is not a DOM element. E.g. window.
353
+export function listenToNativeEventForNonManagedEventTarget(
354
domEventName: DOMEventName,
355
isCapturePhaseListener: boolean,
351
- rootContainerElement: EventTarget,
352
- targetElement: Element | null,
353
- eventSystemFlags?: EventSystemFlags = 0,
356
+ target: EventTarget,
357
): void {
355
- let target = rootContainerElement;
356
-
357
- // selectionchange needs to be attached to the document
358
- // otherwise it won't capture incoming events that are only
359
- // triggered on the document directly.
360
- if (
361
- domEventName === 'selectionchange' &&
362
- (rootContainerElement: any).nodeType !== DOCUMENT_NODE
363
- ) {
364
- target = (rootContainerElement: any).ownerDocument;
365
- }
366
- // If the event can be delegated (or is capture phase), we can
367
- // register it to the root container. Otherwise, we should
368
- // register the event to the target element and mark it as
369
- // a non-delegated event.
370
- if (
371
- targetElement !== null &&
372
- !isCapturePhaseListener &&
373
- nonDelegatedEvents.has(domEventName)
374
- ) {
375
- // For all non-delegated events, apart from scroll, we attach
376
- // their event listeners to the respective elements that their
377
- // events fire on. That means we can skip this step, as event
378
- // listener has already been added previously. However, we
379
- // special case the scroll event because the reality is that any
380
- // element can scroll.
381
- // TODO: ideally, we'd eventually apply the same logic to all
382
- // events from the nonDelegatedEvents list. Then we can remove
383
- // this special case and use the same logic for all events.
384
- if (domEventName !== 'scroll') {
385
- return;
386
- }
387
- eventSystemFlags |= IS_NON_DELEGATED;
388
- target = targetElement;
389
- }
358
+ let eventSystemFlags = IS_EVENT_HANDLE_NON_MANAGED_NODE;
359
const listenerSet = getEventListenerSet(target);
360
const listenerSetKey = getListenerSetKey(
361
domEventName,
362
isCapturePhaseListener,
363
);
395
- // If the listener entry is empty or we should upgrade, then
396
- // we need to trap an event listener onto the target.
364
if (!listenerSet.has(listenerSetKey)) {
365
if (isCapturePhaseListener) {
366
eventSystemFlags |= IS_CAPTURE_PHASE;
@@ -408,6 +375,40 @@ export function listenToNativeEvent(
375
}
376
}
377
378
+const listeningMarker =
379
+ '_reactListening' +
380
+ Math.random()
381
+ .toString(36)
382
+ .slice(2);
383
+
384
+export function listenToAllSupportedEvents(rootContainerElement: EventTarget) {
385
+ if (!(rootContainerElement: any)[listeningMarker]) {
386
+ (rootContainerElement: any)[listeningMarker] = true;
387
+ allNativeEvents.forEach(domEventName => {
388
+ // We handle selectionchange separately because it
389
+ // doesn't bubble and needs to be on the document.
390
+ if (domEventName !== 'selectionchange') {
391
+ if (!nonDelegatedEvents.has(domEventName)) {
392
+ listenToNativeEvent(domEventName, false, rootContainerElement);
393
+ }
394
+ listenToNativeEvent(domEventName, true, rootContainerElement);
395
+ }
396
+ });
397
+ const ownerDocument =
398
+ (rootContainerElement: any).nodeType === DOCUMENT_NODE
399
+ ? rootContainerElement
400
+ : (rootContainerElement: any).ownerDocument;
401
+ if (ownerDocument !== null) {
402
+ // The selectionchange event also needs deduplication
403
+ // but it is attached to the document.
404
+ if (!(ownerDocument: any)[listeningMarker]) {
405
+ (ownerDocument: any)[listeningMarker] = true;
406
+ listenToNativeEvent('selectionchange', false, ownerDocument);
407
+ }
408
+ }
409
+ }
410
+}
411
+
412
function addTrappedEventListener(
413
targetContainer: EventTarget,
414
domEventName: DOMEventName,