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)
Dan Abramov committed
May 22, 2018 at 19:50 UTC
e8857918422b5ce8505ba5ce4a2d153e509c17a1
3 files changed
+200
-22
packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
+161
@@ -217,4 +217,165 @@ describe('ReactDOMEventListener', () => {
217
expect(mouseOut.mock.calls[0][0]).toEqual(instance.getInner());
218
document.body.removeChild(container);
219
});
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
+ });
381
});
packages/react-dom/src/events/DOMTopLevelEventTypes.js
+3
@@ -148,6 +148,9 @@ export const TOP_VOLUME_CHANGE = unsafeCastStringToDOMTopLevelType(
148
export const TOP_WAITING = unsafeCastStringToDOMTopLevelType('waiting');
149
export const TOP_WHEEL = unsafeCastStringToDOMTopLevelType('wheel');
150
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,
packages/react-dom/src/events/ReactBrowserEventEmitter.js
+36
-22
@@ -13,9 +13,12 @@ import {
13
TOP_CANCEL,
14
TOP_CLOSE,
15
TOP_FOCUS,
16
+ TOP_INVALID,
17
TOP_RESET,
18
TOP_SCROLL,
19
TOP_SUBMIT,
20
+ getRawEventName,
21
+ mediaEventTypes,
22
} from './DOMTopLevelEventTypes';
23
import {
24
setEnabled,
@@ -130,29 +133,40 @@ export function listenTo(
133
for (let i = 0; i < dependencies.length; i++) {
134
const dependency = dependencies[i];
135
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;
169
}
155
-
170
isListening[dependency] = true;
171
}
172
}