Skip to content

Commit 356c171

Browse files
trueadmgaearon
trueadm
and
gaearon
authored
Remove capturePhaseEvents and separate events by bubbling (facebook#19278)
* Remove capturePhaseEvents and separate events by bubbling WIP Refine all logic Revise types Fix Fix conflicts Fix flags Fix Fix Fix test Revise Cleanup Refine Deal with replaying Fix * Add non delegated listeners unconditionally * Add media events * Fix a previously ignored test * Address feedback Co-authored-by: Dan Abramov <dan.abramov@me.com>
1 parent 1dcee86 commit 356c171

10 files changed

+277
-218
lines changed

‎packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -349,13 +349,10 @@ describe('ReactDOMEventListener', () => {
349349
}),
350350
);
351351
// As of the modern event system refactor, we now support
352-
// this on <img>. The reason for this, is because we now
353-
// attach all media events to the "root" or "portal" in the
354-
// capture phase, rather than the bubble phase. This allows
355-
// us to assign less event listeners to individual elements,
356-
// which also nicely allows us to support more without needing
357-
// to add more individual code paths to support various
358-
// events that do not bubble.
352+
// this on <img>. The reason for this, is because we allow
353+
// events to be attached to nodes regardless of if they
354+
// necessary support them. This is a strange test, as this
355+
// would never occur from normal browser behavior.
359356
expect(handleImgLoadStart).toHaveBeenCalledTimes(1);
360357

361358
videoRef.current.dispatchEvent(
@@ -374,7 +371,9 @@ describe('ReactDOMEventListener', () => {
374371
document.body.appendChild(container);
375372

376373
const videoRef = React.createRef();
377-
const handleVideoPlay = jest.fn(); // We'll test this one.
374+
// We'll test this event alone.
375+
const handleVideoPlay = jest.fn();
376+
const handleVideoPlayDelegated = jest.fn();
378377
const mediaEvents = {
379378
onAbort() {},
380379
onCanPlay() {},
@@ -401,23 +400,32 @@ describe('ReactDOMEventListener', () => {
401400
onWaiting() {},
402401
};
403402

404-
const originalAddEventListener = document.addEventListener;
403+
const originalDocAddEventListener = document.addEventListener;
404+
const originalRootAddEventListener = container.addEventListener;
405405
document.addEventListener = function(type) {
406406
throw new Error(
407-
`Did not expect to add a top-level listener for the "${type}" event.`,
407+
`Did not expect to add a document-level listener for the "${type}" event.`,
408+
);
409+
};
410+
container.addEventListener = function(type) {
411+
if (type === 'mouseout' || type === 'mouseover') {
412+
// We currently listen to it unconditionally.
413+
return;
414+
}
415+
throw new Error(
416+
`Did not expect to add a root-level listener for the "${type}" event.`,
408417
);
409418
};
410419

411420
try {
412421
// We expect that mounting this tree will
413422
// *not* attach handlers for any top-level events.
414423
ReactDOM.render(
415-
<div>
424+
<div onPlay={handleVideoPlayDelegated}>
416425
<video ref={videoRef} {...mediaEvents} onPlay={handleVideoPlay} />
417426
<audio {...mediaEvents}>
418427
<source {...mediaEvents} />
419428
</audio>
420-
<form onReset={() => {}} onSubmit={() => {}} />
421429
</div>,
422430
container,
423431
);
@@ -429,8 +437,12 @@ describe('ReactDOMEventListener', () => {
429437
}),
430438
);
431439
expect(handleVideoPlay).toHaveBeenCalledTimes(1);
440+
// Unlike browsers, we delegate media events.
441+
// (This doesn't make a lot of sense but it would be a breaking change not to.)
442+
expect(handleVideoPlayDelegated).toHaveBeenCalledTimes(1);
432443
} finally {
433-
document.addEventListener = originalAddEventListener;
444+
document.addEventListener = originalDocAddEventListener;
445+
container.addEventListener = originalRootAddEventListener;
434446
document.body.removeChild(container);
435447
}
436448
});

‎packages/react-dom/src/client/ReactDOMComponent.js

Lines changed: 95 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,18 @@ import {
8585
enableDeprecatedFlareAPI,
8686
enableTrustedTypesIntegration,
8787
} from 'shared/ReactFeatureFlags';
88-
import {listenToReactEvent} from '../events/DOMModernPluginEventSystem';
88+
import {
89+
listenToReactEvent,
90+
mediaEventTypes,
91+
listenToNonDelegatedEvent,
92+
} from '../events/DOMModernPluginEventSystem';
8993
import {getEventListenerMap} from './ReactDOMComponentTree';
94+
import {
95+
TOP_LOAD,
96+
TOP_ERROR,
97+
TOP_TOGGLE,
98+
TOP_INVALID,
99+
} from '../events/DOMTopLevelEventTypes';
90100

91101
let didWarnInvalidHydration = false;
92102
let didWarnScriptTags = false;
@@ -266,6 +276,7 @@ if (__DEV__) {
266276
export function ensureListeningTo(
267277
rootContainerInstance: Element | Node,
268278
reactPropEvent: string,
279+
targetElement: Element | null,
269280
): void {
270281
// If we have a comment node, then use the parent node,
271282
// which should be an element.
@@ -282,7 +293,11 @@ export function ensureListeningTo(
282293
'ensureListeningTo(): received a container that was not an element node. ' +
283294
'This is likely a bug in React.',
284295
);
285-
listenToReactEvent(reactPropEvent, ((rootContainerElement: any): Element));
296+
listenToReactEvent(
297+
reactPropEvent,
298+
((rootContainerElement: any): Element),
299+
targetElement,
300+
);
286301
}
287302

288303
function getOwnerDocumentFromRootContainer(
@@ -364,7 +379,7 @@ function setInitialDOMProperties(
364379
if (__DEV__ && typeof nextProp !== 'function') {
365380
warnForInvalidEventListener(propKey, nextProp);
366381
}
367-
ensureListeningTo(rootContainerElement, propKey);
382+
ensureListeningTo(rootContainerElement, propKey, domElement);
368383
}
369384
} else if (nextProp != null) {
370385
setValueForProperty(domElement, propKey, nextProp, isCustomComponentTag);
@@ -527,32 +542,50 @@ export function setInitialProperties(
527542
case 'iframe':
528543
case 'object':
529544
case 'embed':
545+
// We listen to this event in case to ensure emulated bubble
546+
// listeners still fire for the load event.
547+
listenToNonDelegatedEvent(TOP_LOAD, domElement);
530548
props = rawProps;
531549
break;
532550
case 'video':
533551
case 'audio':
552+
// We listen to these events in case to ensure emulated bubble
553+
// listeners still fire for all the media events.
554+
for (let i = 0; i < mediaEventTypes.length; i++) {
555+
listenToNonDelegatedEvent(mediaEventTypes[i], domElement);
556+
}
534557
props = rawProps;
535558
break;
536559
case 'source':
560+
// We listen to this event in case to ensure emulated bubble
561+
// listeners still fire for the error event.
562+
listenToNonDelegatedEvent(TOP_ERROR, domElement);
537563
props = rawProps;
538564
break;
539565
case 'img':
540566
case 'image':
541567
case 'link':
542-
props = rawProps;
543-
break;
544-
case 'form':
568+
// We listen to these events in case to ensure emulated bubble
569+
// listeners still fire for error and load events.
570+
listenToNonDelegatedEvent(TOP_ERROR, domElement);
571+
listenToNonDelegatedEvent(TOP_LOAD, domElement);
545572
props = rawProps;
546573
break;
547574
case 'details':
575+
// We listen to this event in case to ensure emulated bubble
576+
// listeners still fire for the toggle event.
577+
listenToNonDelegatedEvent(TOP_TOGGLE, domElement);
548578
props = rawProps;
549579
break;
550580
case 'input':
551581
ReactDOMInputInitWrapperState(domElement, rawProps);
552582
props = ReactDOMInputGetHostProps(domElement, rawProps);
583+
// We listen to this event in case to ensure emulated bubble
584+
// listeners still fire for the invalid event.
585+
listenToNonDelegatedEvent(TOP_INVALID, domElement);
553586
// For controlled components we always need to ensure we're listening
554587
// to onChange. Even if there is no listener.
555-
ensureListeningTo(rootContainerElement, 'onChange');
588+
ensureListeningTo(rootContainerElement, 'onChange', domElement);
556589
break;
557590
case 'option':
558591
ReactDOMOptionValidateProps(domElement, rawProps);
@@ -561,16 +594,22 @@ export function setInitialProperties(
561594
case 'select':
562595
ReactDOMSelectInitWrapperState(domElement, rawProps);
563596
props = ReactDOMSelectGetHostProps(domElement, rawProps);
597+
// We listen to this event in case to ensure emulated bubble
598+
// listeners still fire for the invalid event.
599+
listenToNonDelegatedEvent(TOP_INVALID, domElement);
564600
// For controlled components we always need to ensure we're listening
565601
// to onChange. Even if there is no listener.
566-
ensureListeningTo(rootContainerElement, 'onChange');
602+
ensureListeningTo(rootContainerElement, 'onChange', domElement);
567603
break;
568604
case 'textarea':
569605
ReactDOMTextareaInitWrapperState(domElement, rawProps);
570606
props = ReactDOMTextareaGetHostProps(domElement, rawProps);
607+
// We listen to this event in case to ensure emulated bubble
608+
// listeners still fire for the invalid event.
609+
listenToNonDelegatedEvent(TOP_INVALID, domElement);
571610
// For controlled components we always need to ensure we're listening
572611
// to onChange. Even if there is no listener.
573-
ensureListeningTo(rootContainerElement, 'onChange');
612+
ensureListeningTo(rootContainerElement, 'onChange', domElement);
574613
break;
575614
default:
576615
props = rawProps;
@@ -790,7 +829,7 @@ export function diffProperties(
790829
if (__DEV__ && typeof nextProp !== 'function') {
791830
warnForInvalidEventListener(propKey, nextProp);
792831
}
793-
ensureListeningTo(rootContainerElement, propKey);
832+
ensureListeningTo(rootContainerElement, propKey, domElement);
794833
}
795834
if (!updatePayload && lastProp !== nextProp) {
796835
// This is a special case. If any listener updates we need to ensure
@@ -900,26 +939,68 @@ export function diffHydratedProperties(
900939

901940
// TODO: Make sure that we check isMounted before firing any of these events.
902941
switch (tag) {
942+
case 'iframe':
943+
case 'object':
944+
case 'embed':
945+
// We listen to this event in case to ensure emulated bubble
946+
// listeners still fire for the load event.
947+
listenToNonDelegatedEvent(TOP_LOAD, domElement);
948+
break;
949+
case 'video':
950+
case 'audio':
951+
// We listen to these events in case to ensure emulated bubble
952+
// listeners still fire for all the media events.
953+
for (let i = 0; i < mediaEventTypes.length; i++) {
954+
listenToNonDelegatedEvent(mediaEventTypes[i], domElement);
955+
}
956+
break;
957+
case 'source':
958+
// We listen to this event in case to ensure emulated bubble
959+
// listeners still fire for the error event.
960+
listenToNonDelegatedEvent(TOP_ERROR, domElement);
961+
break;
962+
case 'img':
963+
case 'image':
964+
case 'link':
965+
// We listen to these events in case to ensure emulated bubble
966+
// listeners still fire for error and load events.
967+
listenToNonDelegatedEvent(TOP_ERROR, domElement);
968+
listenToNonDelegatedEvent(TOP_LOAD, domElement);
969+
break;
970+
case 'details':
971+
// We listen to this event in case to ensure emulated bubble
972+
// listeners still fire for the toggle event.
973+
listenToNonDelegatedEvent(TOP_TOGGLE, domElement);
974+
break;
903975
case 'input':
904976
ReactDOMInputInitWrapperState(domElement, rawProps);
977+
// We listen to this event in case to ensure emulated bubble
978+
// listeners still fire for the invalid event.
979+
listenToNonDelegatedEvent(TOP_INVALID, domElement);
905980
// For controlled components we always need to ensure we're listening
906981
// to onChange. Even if there is no listener.
907-
ensureListeningTo(rootContainerElement, 'onChange');
982+
ensureListeningTo(rootContainerElement, 'onChange', domElement);
908983
break;
909984
case 'option':
910985
ReactDOMOptionValidateProps(domElement, rawProps);
911986
break;
912987
case 'select':
913988
ReactDOMSelectInitWrapperState(domElement, rawProps);
989+
// We listen to this event in case to ensure emulated bubble
990+
// listeners still fire for the invalid event.
991+
listenToNonDelegatedEvent(TOP_INVALID, domElement);
914992
// For controlled components we always need to ensure we're listening
915993
// to onChange. Even if there is no listener.
916-
ensureListeningTo(rootContainerElement, 'onChange');
994+
ensureListeningTo(rootContainerElement, 'onChange', domElement);
917995
break;
918996
case 'textarea':
919997
ReactDOMTextareaInitWrapperState(domElement, rawProps);
998+
// We listen to this event in case to ensure emulated bubble
999+
// listeners still fire for the invalid event.
1000+
listenToNonDelegatedEvent(TOP_INVALID, domElement);
9201001
// For controlled components we always need to ensure we're listening
9211002
// to onChange. Even if there is no listener.
922-
ensureListeningTo(rootContainerElement, 'onChange');
1003+
ensureListeningTo(rootContainerElement, 'onChange', domElement);
9231004
break;
9241005
}
9251006

@@ -986,7 +1067,7 @@ export function diffHydratedProperties(
9861067
if (__DEV__ && typeof nextProp !== 'function') {
9871068
warnForInvalidEventListener(propKey, nextProp);
9881069
}
989-
ensureListeningTo(rootContainerElement, propKey);
1070+
ensureListeningTo(rootContainerElement, propKey, domElement);
9901071
}
9911072
} else if (
9921073
__DEV__ &&

‎packages/react-dom/src/client/ReactDOMEventHandle.js

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import {
1919
getClosestInstanceFromNode,
2020
getEventHandlerListeners,
2121
setEventHandlerListeners,
22-
getEventListenerMap,
2322
getFiberFromScopeInstance,
2423
} from './ReactDOMComponentTree';
2524
import {ELEMENT_NODE, COMMENT_NODE} from '../shared/HTMLNodeType';
@@ -87,6 +86,7 @@ function registerEventOnNearestTargetContainer(
8786
isPassiveListener: boolean | void,
8887
listenerPriority: EventPriority | void,
8988
isCapturePhaseListener: boolean,
89+
targetElement: Element | null,
9090
): void {
9191
// If it is, find the nearest root or portal and make it
9292
// our event handle target container.
@@ -101,13 +101,11 @@ function registerEventOnNearestTargetContainer(
101101
if (targetContainer.nodeType === COMMENT_NODE) {
102102
targetContainer = ((targetContainer.parentNode: any): Element);
103103
}
104-
const listenerMap = getEventListenerMap(targetContainer);
105104
listenToNativeEvent(
106105
topLevelType,
107-
targetContainer,
108-
listenerMap,
109-
PLUGIN_EVENT_SYSTEM,
110106
isCapturePhaseListener,
107+
targetContainer,
108+
targetElement,
111109
isPassiveListener,
112110
listenerPriority,
113111
);
@@ -138,6 +136,7 @@ function registerReactDOMEvent(
138136
isPassiveListener,
139137
listenerPriority,
140138
isCapturePhaseListener,
139+
targetElement,
141140
);
142141
} else if (enableScopeAPI && isReactScope(target)) {
143142
const scopeTarget = ((target: any): ReactScopeInstance);
@@ -152,18 +151,20 @@ function registerReactDOMEvent(
152151
isPassiveListener,
153152
listenerPriority,
154153
isCapturePhaseListener,
154+
null,
155155
);
156156
} else if (isValidEventTarget(target)) {
157157
const eventTarget = ((target: any): EventTarget);
158-
const listenerMap = getEventListenerMap(eventTarget);
158+
// These are valid event targets, but they are also
159+
// non-managed React nodes.
159160
listenToNativeEvent(
160161
topLevelType,
161-
eventTarget,
162-
listenerMap,
163-
PLUGIN_EVENT_SYSTEM | IS_EVENT_HANDLE_NON_MANAGED_NODE,
164162
isCapturePhaseListener,
163+
eventTarget,
164+
null,
165165
isPassiveListener,
166166
listenerPriority,
167+
PLUGIN_EVENT_SYSTEM | IS_EVENT_HANDLE_NON_MANAGED_NODE,
167168
);
168169
} else {
169170
invariant(

‎packages/react-dom/src/client/ReactDOMHostConfig.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1111,7 +1111,7 @@ export function makeOpaqueHydratingObject(
11111111
}
11121112

11131113
export function preparePortalMount(portalInstance: Instance): void {
1114-
listenToReactEvent('onMouseEnter', portalInstance);
1114+
listenToReactEvent('onMouseEnter', portalInstance, null);
11151115
}
11161116

11171117
export function prepareScopeUpdate(

‎packages/react-dom/src/client/ReactDOMRoot.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ function createRootImpl(
145145
containerNodeType !== DOCUMENT_FRAGMENT_NODE &&
146146
containerNodeType !== DOCUMENT_NODE
147147
) {
148-
ensureListeningTo(container, 'onMouseEnter');
148+
ensureListeningTo(container, 'onMouseEnter', null);
149149
}
150150

151151
if (mutableSources) {

0 commit comments

Comments
 (0)