From e8857918422b5ce8505ba5ce4a2d153e509c17a1 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 22 May 2018 19:50:36 +0100 Subject: [PATCH] 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 . And since we don't special case it in the event attachment logic when we create it, we never supported 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 . 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) --- .../__tests__/ReactDOMEventListener-test.js | 161 ++++++++++++++++++ .../src/events/DOMTopLevelEventTypes.js | 3 + .../src/events/ReactBrowserEventEmitter.js | 58 ++++--- 3 files changed, 200 insertions(+), 22 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index 8f11847aac..5c2ff8c591 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -217,4 +217,165 @@ describe('ReactDOMEventListener', () => { expect(mouseOut.mock.calls[0][0]).toEqual(instance.getInner()); document.body.removeChild(container); }); + + // Regression test for https://github.com/facebook/react/pull/12877 + it('should not fire form events twice', () => { + const container = document.createElement('div'); + document.body.appendChild(container); + + const formRef = React.createRef(); + const inputRef = React.createRef(); + + const handleInvalid = jest.fn(); + const handleReset = jest.fn(); + const handleSubmit = jest.fn(); + ReactDOM.render( +
+ +
, + container, + ); + + inputRef.current.dispatchEvent( + new Event('invalid', { + // https://developer.mozilla.org/en-US/docs/Web/Events/invalid + bubbles: false, + }), + ); + expect(handleInvalid).toHaveBeenCalledTimes(1); + + formRef.current.dispatchEvent( + new Event('reset', { + // https://developer.mozilla.org/en-US/docs/Web/Events/reset + bubbles: true, + }), + ); + expect(handleReset).toHaveBeenCalledTimes(1); + + formRef.current.dispatchEvent( + new Event('submit', { + // https://developer.mozilla.org/en-US/docs/Web/Events/submit + bubbles: true, + }), + ); + expect(handleSubmit).toHaveBeenCalledTimes(1); + + formRef.current.dispatchEvent( + new Event('submit', { + // Might happen on older browsers. + bubbles: true, + }), + ); + expect(handleSubmit).toHaveBeenCalledTimes(2); // It already fired in this test. + + document.body.removeChild(container); + }); + + it('should dispatch loadstart only for media elements', () => { + const container = document.createElement('div'); + document.body.appendChild(container); + + const imgRef = React.createRef(); + const videoRef = React.createRef(); + + const handleImgLoadStart = jest.fn(); + const handleVideoLoadStart = jest.fn(); + ReactDOM.render( +
+ +
, + container, + ); + + // Note for debugging: loadstart currently doesn't fire in Chrome. + // https://bugs.chromium.org/p/chromium/issues/detail?id=458851 + imgRef.current.dispatchEvent( + new ProgressEvent('loadstart', { + bubbles: false, + }), + ); + // Historically, we happened to not support onLoadStart + // on , and this test documents that lack of support. + // If we decide to support it in the future, we should change + // this line to expect 1 call. Note that fixing this would + // be simple but would require attaching a handler to each + // . So far nobody asked us for it. + expect(handleImgLoadStart).toHaveBeenCalledTimes(0); + + videoRef.current.dispatchEvent( + new ProgressEvent('loadstart', { + bubbles: false, + }), + ); + expect(handleVideoLoadStart).toHaveBeenCalledTimes(1); + + document.body.removeChild(container); + }); + + it('should not attempt to listen to unnecessary events on the top level', () => { + const container = document.createElement('div'); + document.body.appendChild(container); + + const videoRef = React.createRef(); + const handleVideoPlay = jest.fn(); // We'll test this one. + const mediaEvents = { + onAbort() {}, + onCanPlay() {}, + onCanPlayThrough() {}, + onDurationChange() {}, + onEmptied() {}, + onEncrypted() {}, + onEnded() {}, + onError() {}, + onLoadedData() {}, + onLoadedMetadata() {}, + onLoadStart() {}, + onPause() {}, + onPlay() {}, + onPlaying() {}, + onProgress() {}, + onRateChange() {}, + onSeeked() {}, + onSeeking() {}, + onStalled() {}, + onSuspend() {}, + onTimeUpdate() {}, + onVolumeChange() {}, + onWaiting() {}, + }; + + const originalAddEventListener = document.addEventListener; + document.addEventListener = function(type) { + throw new Error( + `Did not expect to add a top-level listener for the "${type}" event.`, + ); + }; + + try { + // We expect that mounting this tree will + // *not* attach handlers for any top-level events. + ReactDOM.render( +
+
, + container, + ); + + // Also verify dispatching one of them works + videoRef.current.dispatchEvent( + new Event('play', { + bubbles: false, + }), + ); + expect(handleVideoPlay).toHaveBeenCalledTimes(1); + } finally { + document.addEventListener = originalAddEventListener; + document.body.removeChild(container); + } + }); }); diff --git a/packages/react-dom/src/events/DOMTopLevelEventTypes.js b/packages/react-dom/src/events/DOMTopLevelEventTypes.js index 298afec9c9..dc526a0ef0 100644 --- a/packages/react-dom/src/events/DOMTopLevelEventTypes.js +++ b/packages/react-dom/src/events/DOMTopLevelEventTypes.js @@ -148,6 +148,9 @@ export const TOP_VOLUME_CHANGE = unsafeCastStringToDOMTopLevelType( export const TOP_WAITING = unsafeCastStringToDOMTopLevelType('waiting'); export const TOP_WHEEL = unsafeCastStringToDOMTopLevelType('wheel'); +// List of events that need to be individually attached to media elements. +// Note that events in this list will *not* be listened to at the top level +// unless they're explicitly whitelisted in `ReactBrowserEventEmitter.listenTo`. export const mediaEventTypes = [ TOP_ABORT, TOP_CAN_PLAY, diff --git a/packages/react-dom/src/events/ReactBrowserEventEmitter.js b/packages/react-dom/src/events/ReactBrowserEventEmitter.js index d3291263ec..3dd2e452f5 100644 --- a/packages/react-dom/src/events/ReactBrowserEventEmitter.js +++ b/packages/react-dom/src/events/ReactBrowserEventEmitter.js @@ -13,9 +13,12 @@ import { TOP_CANCEL, TOP_CLOSE, TOP_FOCUS, + TOP_INVALID, TOP_RESET, TOP_SCROLL, TOP_SUBMIT, + getRawEventName, + mediaEventTypes, } from './DOMTopLevelEventTypes'; import { setEnabled, @@ -130,29 +133,40 @@ export function listenTo( for (let i = 0; i < dependencies.length; i++) { const dependency = dependencies[i]; if (!(isListening.hasOwnProperty(dependency) && isListening[dependency])) { - if (dependency === TOP_SCROLL) { - trapCapturedEvent(TOP_SCROLL, mountAt); - } else if (dependency === TOP_FOCUS || dependency === TOP_BLUR) { - trapCapturedEvent(TOP_FOCUS, mountAt); - trapCapturedEvent(TOP_BLUR, mountAt); - - // to make sure blur and focus event listeners are only attached once - isListening[TOP_BLUR] = true; - isListening[TOP_FOCUS] = true; - } else if (dependency === TOP_CANCEL) { - if (isEventSupported('cancel', true)) { - trapCapturedEvent(TOP_CANCEL, mountAt); - } - isListening[TOP_CANCEL] = true; - } else if (dependency === TOP_CLOSE) { - if (isEventSupported('close', true)) { - trapCapturedEvent(TOP_CLOSE, mountAt); - } - isListening[TOP_CLOSE] = true; - } else if (dependency !== TOP_SUBMIT && dependency !== TOP_RESET) { - trapBubbledEvent(dependency, mountAt); + switch (dependency) { + case TOP_SCROLL: + trapCapturedEvent(TOP_SCROLL, mountAt); + break; + case TOP_FOCUS: + case TOP_BLUR: + trapCapturedEvent(TOP_FOCUS, mountAt); + trapCapturedEvent(TOP_BLUR, mountAt); + // We set the flag for a single dependency later in this function, + // but this ensures we mark both as attached rather than just one. + isListening[TOP_BLUR] = true; + isListening[TOP_FOCUS] = true; + break; + case TOP_CANCEL: + case TOP_CLOSE: + if (isEventSupported(getRawEventName(dependency), true)) { + trapCapturedEvent(dependency, mountAt); + } + break; + case TOP_INVALID: + case TOP_SUBMIT: + case TOP_RESET: + // We listen to them on the target DOM elements. + // Some of them bubble so we don't want them to fire twice. + break; + default: + // By default, listen on the top level to all non-media events. + // Media events don't bubble so adding the listener wouldn't do anything. + const isMediaEvent = mediaEventTypes.indexOf(dependency) !== -1; + if (!isMediaEvent) { + trapBubbledEvent(dependency, mountAt); + } + break; } - isListening[dependency] = true; } }