Skip to content

Commit 1c6289f

Browse files
committed
Always use event capturing. Remove media events.
This commit changes event subscription such that all events are attached with capturing. This prevents the need to eagerly attach media event listeners to video, audio, and source tags, and simplifies the event subscription process. I think we can probably extend this to more event types; requiring much more testing. Until that time, this commit lays the foundation for that work by demonstrating that capturing is applicable in cases where bubbling does not occur.
1 parent 61777a7 commit 1c6289f

7 files changed

+105
-187
lines changed

packages/react-dom/src/__tests__/ReactDOMComponent-test.js

+11-8
Original file line numberDiff line numberDiff line change
@@ -966,26 +966,29 @@ describe('ReactDOMComponent', () => {
966966
});
967967

968968
it('should work error event on <source> element', () => {
969-
spyOnDevAndProd(console, 'log');
969+
const onError = jest.fn();
970970
const container = document.createElement('div');
971971
ReactDOM.render(
972972
<video>
973973
<source
974974
src="http://example.org/video"
975975
type="video/mp4"
976-
onError={e => console.log('onError called')}
976+
onError={onError}
977977
/>
978978
</video>,
979979
container,
980980
);
981981

982-
const errorEvent = document.createEvent('Event');
983-
errorEvent.initEvent('error', false, false);
984-
container.getElementsByTagName('source')[0].dispatchEvent(errorEvent);
982+
try {
983+
document.body.appendChild(container);
985984

986-
if (__DEV__) {
987-
expect(console.log.calls.count()).toBe(1);
988-
expect(console.log.calls.argsFor(0)[0]).toContain('onError called');
985+
const errorEvent = document.createEvent('Event');
986+
errorEvent.initEvent('error', false, false);
987+
container.getElementsByTagName('source')[0].dispatchEvent(errorEvent);
988+
989+
expect(onError).toHaveBeenCalledTimes(1);
990+
} finally {
991+
container.remove();
989992
}
990993
});
991994

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

+65-51
Original file line numberDiff line numberDiff line change
@@ -271,49 +271,7 @@ describe('ReactDOMEventListener', () => {
271271
document.body.removeChild(container);
272272
});
273273

274-
it('should dispatch loadstart only for media elements', () => {
275-
const container = document.createElement('div');
276-
document.body.appendChild(container);
277-
278-
const imgRef = React.createRef();
279-
const videoRef = React.createRef();
280-
281-
const handleImgLoadStart = jest.fn();
282-
const handleVideoLoadStart = jest.fn();
283-
ReactDOM.render(
284-
<div>
285-
<img ref={imgRef} onLoadStart={handleImgLoadStart} />
286-
<video ref={videoRef} onLoadStart={handleVideoLoadStart} />
287-
</div>,
288-
container,
289-
);
290-
291-
// Note for debugging: loadstart currently doesn't fire in Chrome.
292-
// https://bugs.chromium.org/p/chromium/issues/detail?id=458851
293-
imgRef.current.dispatchEvent(
294-
new ProgressEvent('loadstart', {
295-
bubbles: false,
296-
}),
297-
);
298-
// Historically, we happened to not support onLoadStart
299-
// on <img>, and this test documents that lack of support.
300-
// If we decide to support it in the future, we should change
301-
// this line to expect 1 call. Note that fixing this would
302-
// be simple but would require attaching a handler to each
303-
// <img>. So far nobody asked us for it.
304-
expect(handleImgLoadStart).toHaveBeenCalledTimes(0);
305-
306-
videoRef.current.dispatchEvent(
307-
new ProgressEvent('loadstart', {
308-
bubbles: false,
309-
}),
310-
);
311-
expect(handleVideoLoadStart).toHaveBeenCalledTimes(1);
312-
313-
document.body.removeChild(container);
314-
});
315-
316-
it('should not attempt to listen to unnecessary events on the top level', () => {
274+
it('should dispatch media events', () => {
317275
const container = document.createElement('div');
318276
document.body.appendChild(container);
319277

@@ -345,13 +303,6 @@ describe('ReactDOMEventListener', () => {
345303
onWaiting() {},
346304
};
347305

348-
const originalAddEventListener = document.addEventListener;
349-
document.addEventListener = function(type) {
350-
throw new Error(
351-
`Did not expect to add a top-level listener for the "${type}" event.`,
352-
);
353-
};
354-
355306
try {
356307
// We expect that mounting this tree will
357308
// *not* attach handlers for any top-level events.
@@ -374,8 +325,71 @@ describe('ReactDOMEventListener', () => {
374325
);
375326
expect(handleVideoPlay).toHaveBeenCalledTimes(1);
376327
} finally {
377-
document.addEventListener = originalAddEventListener;
378328
document.body.removeChild(container);
379329
}
380330
});
331+
332+
it('does not double dispatch scroll events at the deepest leaf', () => {
333+
const top = jest.fn();
334+
const middle = jest.fn();
335+
const bottom = jest.fn();
336+
const container = document.createElement('div');
337+
338+
ReactDOM.render(
339+
<div id="top" onScroll={top}>
340+
<div id="middle" onScroll={middle}>
341+
<div id="bottom" onScroll={bottom} />
342+
</div>
343+
</div>,
344+
container,
345+
);
346+
347+
const target = container.querySelector('#bottom');
348+
const event = document.createEvent('Event');
349+
350+
try {
351+
document.body.appendChild(container);
352+
353+
event.initEvent('scroll', true, true);
354+
target.dispatchEvent(event);
355+
356+
expect(top).toHaveBeenCalledTimes(1);
357+
expect(middle).toHaveBeenCalledTimes(1);
358+
expect(bottom).toHaveBeenCalledTimes(1);
359+
} finally {
360+
container.remove();
361+
}
362+
});
363+
364+
it('does not double dispatch scroll events at the middle leaf', () => {
365+
const top = jest.fn();
366+
const middle = jest.fn();
367+
const bottom = jest.fn();
368+
const container = document.createElement('div');
369+
370+
ReactDOM.render(
371+
<div id="top" onScroll={top}>
372+
<div id="middle" onScroll={middle}>
373+
<div id="bottom" onScroll={bottom} />
374+
</div>
375+
</div>,
376+
container,
377+
);
378+
379+
const target = container.querySelector('#middle');
380+
const event = document.createEvent('Event');
381+
382+
try {
383+
document.body.appendChild(container);
384+
385+
event.initEvent('scroll', true, true);
386+
target.dispatchEvent(event);
387+
388+
expect(top).toHaveBeenCalledTimes(1);
389+
expect(middle).toHaveBeenCalledTimes(1);
390+
expect(bottom).toHaveBeenCalledTimes(0);
391+
} finally {
392+
container.remove();
393+
}
394+
});
381395
});

packages/react-dom/src/client/ReactDOMFiberComponent.js

+20-40
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,7 @@ import {
2929
TOP_SUBMIT,
3030
TOP_TOGGLE,
3131
} from '../events/DOMTopLevelEventTypes';
32-
import {listenTo, trapBubbledEvent} from '../events/ReactBrowserEventEmitter';
33-
import {mediaEventTypes} from '../events/DOMTopLevelEventTypes';
32+
import {listenTo, trapEvent} from '../events/ReactBrowserEventEmitter';
3433
import * as CSSPropertyOperations from '../shared/CSSPropertyOperations';
3534
import {Namespaces, getIntrinsicNamespace} from '../shared/DOMNamespaces';
3635
import {
@@ -452,41 +451,29 @@ export function setInitialProperties(
452451
switch (tag) {
453452
case 'iframe':
454453
case 'object':
455-
trapBubbledEvent(TOP_LOAD, domElement);
456-
props = rawProps;
457-
break;
458-
case 'video':
459-
case 'audio':
460-
// Create listener for each media event
461-
for (let i = 0; i < mediaEventTypes.length; i++) {
462-
trapBubbledEvent(mediaEventTypes[i], domElement);
463-
}
464-
props = rawProps;
465-
break;
466-
case 'source':
467-
trapBubbledEvent(TOP_ERROR, domElement);
454+
trapEvent(TOP_LOAD, domElement);
468455
props = rawProps;
469456
break;
470457
case 'img':
471458
case 'image':
472459
case 'link':
473-
trapBubbledEvent(TOP_ERROR, domElement);
474-
trapBubbledEvent(TOP_LOAD, domElement);
460+
trapEvent(TOP_ERROR, domElement);
461+
trapEvent(TOP_LOAD, domElement);
475462
props = rawProps;
476463
break;
477464
case 'form':
478-
trapBubbledEvent(TOP_RESET, domElement);
479-
trapBubbledEvent(TOP_SUBMIT, domElement);
465+
trapEvent(TOP_RESET, domElement);
466+
trapEvent(TOP_SUBMIT, domElement);
480467
props = rawProps;
481468
break;
482469
case 'details':
483-
trapBubbledEvent(TOP_TOGGLE, domElement);
470+
trapEvent(TOP_TOGGLE, domElement);
484471
props = rawProps;
485472
break;
486473
case 'input':
487474
ReactDOMFiberInput.initWrapperState(domElement, rawProps);
488475
props = ReactDOMFiberInput.getHostProps(domElement, rawProps);
489-
trapBubbledEvent(TOP_INVALID, domElement);
476+
trapEvent(TOP_INVALID, domElement);
490477
// For controlled components we always need to ensure we're listening
491478
// to onChange. Even if there is no listener.
492479
ensureListeningTo(rootContainerElement, 'onChange');
@@ -498,15 +485,15 @@ export function setInitialProperties(
498485
case 'select':
499486
ReactDOMFiberSelect.initWrapperState(domElement, rawProps);
500487
props = ReactDOMFiberSelect.getHostProps(domElement, rawProps);
501-
trapBubbledEvent(TOP_INVALID, domElement);
488+
trapEvent(TOP_INVALID, domElement);
502489
// For controlled components we always need to ensure we're listening
503490
// to onChange. Even if there is no listener.
504491
ensureListeningTo(rootContainerElement, 'onChange');
505492
break;
506493
case 'textarea':
507494
ReactDOMFiberTextarea.initWrapperState(domElement, rawProps);
508495
props = ReactDOMFiberTextarea.getHostProps(domElement, rawProps);
509-
trapBubbledEvent(TOP_INVALID, domElement);
496+
trapEvent(TOP_INVALID, domElement);
510497
// For controlled components we always need to ensure we're listening
511498
// to onChange. Even if there is no listener.
512499
ensureListeningTo(rootContainerElement, 'onChange');
@@ -843,34 +830,27 @@ export function diffHydratedProperties(
843830
switch (tag) {
844831
case 'iframe':
845832
case 'object':
846-
trapBubbledEvent(TOP_LOAD, domElement);
847-
break;
848-
case 'video':
849-
case 'audio':
850-
// Create listener for each media event
851-
for (let i = 0; i < mediaEventTypes.length; i++) {
852-
trapBubbledEvent(mediaEventTypes[i], domElement);
853-
}
833+
trapEvent(TOP_LOAD, domElement);
854834
break;
855835
case 'source':
856-
trapBubbledEvent(TOP_ERROR, domElement);
836+
trapEvent(TOP_ERROR, domElement);
857837
break;
858838
case 'img':
859839
case 'image':
860840
case 'link':
861-
trapBubbledEvent(TOP_ERROR, domElement);
862-
trapBubbledEvent(TOP_LOAD, domElement);
841+
trapEvent(TOP_ERROR, domElement);
842+
trapEvent(TOP_LOAD, domElement);
863843
break;
864844
case 'form':
865-
trapBubbledEvent(TOP_RESET, domElement);
866-
trapBubbledEvent(TOP_SUBMIT, domElement);
845+
trapEvent(TOP_RESET, domElement);
846+
trapEvent(TOP_SUBMIT, domElement);
867847
break;
868848
case 'details':
869-
trapBubbledEvent(TOP_TOGGLE, domElement);
849+
trapEvent(TOP_TOGGLE, domElement);
870850
break;
871851
case 'input':
872852
ReactDOMFiberInput.initWrapperState(domElement, rawProps);
873-
trapBubbledEvent(TOP_INVALID, domElement);
853+
trapEvent(TOP_INVALID, domElement);
874854
// For controlled components we always need to ensure we're listening
875855
// to onChange. Even if there is no listener.
876856
ensureListeningTo(rootContainerElement, 'onChange');
@@ -880,14 +860,14 @@ export function diffHydratedProperties(
880860
break;
881861
case 'select':
882862
ReactDOMFiberSelect.initWrapperState(domElement, rawProps);
883-
trapBubbledEvent(TOP_INVALID, domElement);
863+
trapEvent(TOP_INVALID, domElement);
884864
// For controlled components we always need to ensure we're listening
885865
// to onChange. Even if there is no listener.
886866
ensureListeningTo(rootContainerElement, 'onChange');
887867
break;
888868
case 'textarea':
889869
ReactDOMFiberTextarea.initWrapperState(domElement, rawProps);
890-
trapBubbledEvent(TOP_INVALID, domElement);
870+
trapEvent(TOP_INVALID, domElement);
891871
// For controlled components we always need to ensure we're listening
892872
// to onChange. Even if there is no listener.
893873
ensureListeningTo(rootContainerElement, 'onChange');

packages/react-dom/src/events/DOMTopLevelEventTypes.js

-29
Original file line numberDiff line numberDiff line change
@@ -148,35 +148,6 @@ export const TOP_VOLUME_CHANGE = unsafeCastStringToDOMTopLevelType(
148148
export const TOP_WAITING = unsafeCastStringToDOMTopLevelType('waiting');
149149
export const TOP_WHEEL = unsafeCastStringToDOMTopLevelType('wheel');
150150

151-
// List of events that need to be individually attached to media elements.
152-
// Note that events in this list will *not* be listened to at the top level
153-
// unless they're explicitly whitelisted in `ReactBrowserEventEmitter.listenTo`.
154-
export const mediaEventTypes = [
155-
TOP_ABORT,
156-
TOP_CAN_PLAY,
157-
TOP_CAN_PLAY_THROUGH,
158-
TOP_DURATION_CHANGE,
159-
TOP_EMPTIED,
160-
TOP_ENCRYPTED,
161-
TOP_ENDED,
162-
TOP_ERROR,
163-
TOP_LOADED_DATA,
164-
TOP_LOADED_METADATA,
165-
TOP_LOAD_START,
166-
TOP_PAUSE,
167-
TOP_PLAY,
168-
TOP_PLAYING,
169-
TOP_PROGRESS,
170-
TOP_RATE_CHANGE,
171-
TOP_SEEKED,
172-
TOP_SEEKING,
173-
TOP_STALLED,
174-
TOP_SUSPEND,
175-
TOP_TIME_UPDATE,
176-
TOP_VOLUME_CHANGE,
177-
TOP_WAITING,
178-
];
179-
180151
export function getRawEventName(topLevelType: DOMTopLevelEventType): string {
181152
return unsafeCastDOMTopLevelTypeToString(topLevelType);
182153
}

packages/react-dom/src/events/EventListener.js

-8
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,6 @@
77
* @flow
88
*/
99

10-
export function addEventBubbleListener(
11-
element: Document | Element,
12-
eventType: string,
13-
listener: Function,
14-
): void {
15-
element.addEventListener(eventType, listener, false);
16-
}
17-
1810
export function addEventCaptureListener(
1911
element: Document | Element,
2012
eventType: string,

0 commit comments

Comments
 (0)