Skip to content

Commit e885791

Browse files
gaearongaearon
authored
Fix a regression that caused us to listen to extra events at the top (#12878)
* Rewrite to a switch I find it a bit easier to follow than many comparison conditions. * Remove unnecessary assignments They are being assigned below anyway. This is likely a copypasta from the FOCUS/BLUR special case (which *does* need those assignments). * Unify "cancel" and "close" cases Their logic is identical. * Don't listen to media events at the top * Add a unit test for double-invoking form events * Remove an unused case and document it in a test The case I added was wrong (just like including this event in the top level list was always wrong). In fact it never bubbles, even for <img>. And since we don't special case it in the <img> event attachment logic when we create it, we never supported <img onLoadStart> at all. We could fix it. But Chrome doesn't support it either: https://bugs.chromium.org/p/chromium/issues/detail?id=458851. Nobody asked us for it yet. And supporting it would require attaching an extra listener to every <img>. So maybe we don't need it? Let's document the existing state of things. * Add a test verifying we don't attach unnecessary listeners * Add a comment * Add a test for submit (bubbles: false)
1 parent 7c0aca2 commit e885791

File tree

3 files changed

+200
-22
lines changed

3 files changed

+200
-22
lines changed

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

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,4 +217,165 @@ describe('ReactDOMEventListener', () => {
217217
expect(mouseOut.mock.calls[0][0]).toEqual(instance.getInner());
218218
document.body.removeChild(container);
219219
});
220+
221+
// Regression test for https://github.com/facebook/react/pull/12877
222+
it('should not fire form events twice', () => {
223+
const container = document.createElement('div');
224+
document.body.appendChild(container);
225+
226+
const formRef = React.createRef();
227+
const inputRef = React.createRef();
228+
229+
const handleInvalid = jest.fn();
230+
const handleReset = jest.fn();
231+
const handleSubmit = jest.fn();
232+
ReactDOM.render(
233+
<form ref={formRef} onReset={handleReset} onSubmit={handleSubmit}>
234+
<input ref={inputRef} onInvalid={handleInvalid} />
235+
</form>,
236+
container,
237+
);
238+
239+
inputRef.current.dispatchEvent(
240+
new Event('invalid', {
241+
// https://developer.mozilla.org/en-US/docs/Web/Events/invalid
242+
bubbles: false,
243+
}),
244+
);
245+
expect(handleInvalid).toHaveBeenCalledTimes(1);
246+
247+
formRef.current.dispatchEvent(
248+
new Event('reset', {
249+
// https://developer.mozilla.org/en-US/docs/Web/Events/reset
250+
bubbles: true,
251+
}),
252+
);
253+
expect(handleReset).toHaveBeenCalledTimes(1);
254+
255+
formRef.current.dispatchEvent(
256+
new Event('submit', {
257+
// https://developer.mozilla.org/en-US/docs/Web/Events/submit
258+
bubbles: true,
259+
}),
260+
);
261+
expect(handleSubmit).toHaveBeenCalledTimes(1);
262+
263+
formRef.current.dispatchEvent(
264+
new Event('submit', {
265+
// Might happen on older browsers.
266+
bubbles: true,
267+
}),
268+
);
269+
expect(handleSubmit).toHaveBeenCalledTimes(2); // It already fired in this test.
270+
271+
document.body.removeChild(container);
272+
});
273+
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', () => {
317+
const container = document.createElement('div');
318+
document.body.appendChild(container);
319+
320+
const videoRef = React.createRef();
321+
const handleVideoPlay = jest.fn(); // We'll test this one.
322+
const mediaEvents = {
323+
onAbort() {},
324+
onCanPlay() {},
325+
onCanPlayThrough() {},
326+
onDurationChange() {},
327+
onEmptied() {},
328+
onEncrypted() {},
329+
onEnded() {},
330+
onError() {},
331+
onLoadedData() {},
332+
onLoadedMetadata() {},
333+
onLoadStart() {},
334+
onPause() {},
335+
onPlay() {},
336+
onPlaying() {},
337+
onProgress() {},
338+
onRateChange() {},
339+
onSeeked() {},
340+
onSeeking() {},
341+
onStalled() {},
342+
onSuspend() {},
343+
onTimeUpdate() {},
344+
onVolumeChange() {},
345+
onWaiting() {},
346+
};
347+
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+
355+
try {
356+
// We expect that mounting this tree will
357+
// *not* attach handlers for any top-level events.
358+
ReactDOM.render(
359+
<div>
360+
<video ref={videoRef} {...mediaEvents} onPlay={handleVideoPlay} />
361+
<audio {...mediaEvents}>
362+
<source {...mediaEvents} />
363+
</audio>
364+
<form onReset={() => {}} onSubmit={() => {}} />
365+
</div>,
366+
container,
367+
);
368+
369+
// Also verify dispatching one of them works
370+
videoRef.current.dispatchEvent(
371+
new Event('play', {
372+
bubbles: false,
373+
}),
374+
);
375+
expect(handleVideoPlay).toHaveBeenCalledTimes(1);
376+
} finally {
377+
document.addEventListener = originalAddEventListener;
378+
document.body.removeChild(container);
379+
}
380+
});
220381
});

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,9 @@ 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`.
151154
export const mediaEventTypes = [
152155
TOP_ABORT,
153156
TOP_CAN_PLAY,

‎packages/react-dom/src/events/ReactBrowserEventEmitter.js‎

Lines changed: 36 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,12 @@ import {
1313
TOP_CANCEL,
1414
TOP_CLOSE,
1515
TOP_FOCUS,
16+
TOP_INVALID,
1617
TOP_RESET,
1718
TOP_SCROLL,
1819
TOP_SUBMIT,
20+
getRawEventName,
21+
mediaEventTypes,
1922
} from './DOMTopLevelEventTypes';
2023
import {
2124
setEnabled,
@@ -130,29 +133,40 @@ export function listenTo(
130133
for (let i = 0; i < dependencies.length; i++) {
131134
const dependency = dependencies[i];
132135
if (!(isListening.hasOwnProperty(dependency) && isListening[dependency])) {
133-
if (dependency === TOP_SCROLL) {
134-
trapCapturedEvent(TOP_SCROLL, mountAt);
135-
} else if (dependency === TOP_FOCUS || dependency === TOP_BLUR) {
136-
trapCapturedEvent(TOP_FOCUS, mountAt);
137-
trapCapturedEvent(TOP_BLUR, mountAt);
138-
139-
// to make sure blur and focus event listeners are only attached once
140-
isListening[TOP_BLUR] = true;
141-
isListening[TOP_FOCUS] = true;
142-
} else if (dependency === TOP_CANCEL) {
143-
if (isEventSupported('cancel', true)) {
144-
trapCapturedEvent(TOP_CANCEL, mountAt);
145-
}
146-
isListening[TOP_CANCEL] = true;
147-
} else if (dependency === TOP_CLOSE) {
148-
if (isEventSupported('close', true)) {
149-
trapCapturedEvent(TOP_CLOSE, mountAt);
150-
}
151-
isListening[TOP_CLOSE] = true;
152-
} else if (dependency !== TOP_SUBMIT && dependency !== TOP_RESET) {
153-
trapBubbledEvent(dependency, mountAt);
136+
switch (dependency) {
137+
case TOP_SCROLL:
138+
trapCapturedEvent(TOP_SCROLL, mountAt);
139+
break;
140+
case TOP_FOCUS:
141+
case TOP_BLUR:
142+
trapCapturedEvent(TOP_FOCUS, mountAt);
143+
trapCapturedEvent(TOP_BLUR, mountAt);
144+
// We set the flag for a single dependency later in this function,
145+
// but this ensures we mark both as attached rather than just one.
146+
isListening[TOP_BLUR] = true;
147+
isListening[TOP_FOCUS] = true;
148+
break;
149+
case TOP_CANCEL:
150+
case TOP_CLOSE:
151+
if (isEventSupported(getRawEventName(dependency), true)) {
152+
trapCapturedEvent(dependency, mountAt);
153+
}
154+
break;
155+
case TOP_INVALID:
156+
case TOP_SUBMIT:
157+
case TOP_RESET:
158+
// We listen to them on the target DOM elements.
159+
// Some of them bubble so we don't want them to fire twice.
160+
break;
161+
default:
162+
// By default, listen on the top level to all non-media events.
163+
// Media events don't bubble so adding the listener wouldn't do anything.
164+
const isMediaEvent = mediaEventTypes.indexOf(dependency) !== -1;
165+
if (!isMediaEvent) {
166+
trapBubbledEvent(dependency, mountAt);
167+
}
168+
break;
154169
}
155-
156170
isListening[dependency] = true;
157171
}
158172
}

0 commit comments

Comments
 (0)