Bail-out of attaching non-delegated listeners (facebook#19466)

trueadm · web-flow · commit 144297165b5c · 2020-07-27T17:47:14.000+01:00
* Bail-out of attaching non-delegated listeners

Revise comment

* Fix tests/add tests

* Add onInvalid test
diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
@@ -348,12 +348,7 @@ describe('ReactDOMEventListener', () => {
           bubbles: false,
         }),
       );
-      // As of the modern event system refactor, we now support
-      // this on <img>. The reason for this, is because we allow
-      // events to be attached to nodes regardless of if they
-      // necessary support them. This is a strange test, as this
-      // would never occur from normal browser behavior.
-      expect(handleImgLoadStart).toHaveBeenCalledTimes(1);
+      expect(handleImgLoadStart).toHaveBeenCalledTimes(0);
 
       videoRef.current.dispatchEvent(
         new ProgressEvent('loadstart', {
@@ -535,36 +530,42 @@ describe('ReactDOMEventListener', () => {
     }
   });
 
-  it('should bubble non-native bubbling events', () => {
+  it('should bubble non-native bubbling toggle events', () => {
     const container = document.createElement('div');
     const ref = React.createRef();
-    const onPlay = jest.fn();
-    const onCancel = jest.fn();
-    const onClose = jest.fn();
     const onToggle = jest.fn();
     document.body.appendChild(container);
     try {
       ReactDOM.render(
-        <div
-          onPlay={onPlay}
-          onCancel={onCancel}
-          onClose={onClose}
-          onToggle={onToggle}>
-          <div
-            ref={ref}
-            onPlay={onPlay}
-            onCancel={onCancel}
-            onClose={onClose}
-            onToggle={onToggle}
-          />
+        <div onToggle={onToggle}>
+          <details ref={ref} onToggle={onToggle} />
         </div>,
         container,
       );
       ref.current.dispatchEvent(
-        new Event('play', {
+        new Event('toggle', {
           bubbles: false,
         }),
       );
+      expect(onToggle).toHaveBeenCalledTimes(2);
+    } finally {
+      document.body.removeChild(container);
+    }
+  });
+
+  it('should bubble non-native bubbling cancel/close events', () => {
+    const container = document.createElement('div');
+    const ref = React.createRef();
+    const onCancel = jest.fn();
+    const onClose = jest.fn();
+    document.body.appendChild(container);
+    try {
+      ReactDOM.render(
+        <div onCancel={onCancel} onClose={onClose}>
+          <dialog ref={ref} onCancel={onCancel} onClose={onClose} />
+        </div>,
+        container,
+      );
       ref.current.dispatchEvent(
         new Event('cancel', {
           bubbles: false,
@@ -575,17 +576,54 @@ describe('ReactDOMEventListener', () => {
           bubbles: false,
         }),
       );
+      expect(onCancel).toHaveBeenCalledTimes(2);
+      expect(onClose).toHaveBeenCalledTimes(2);
+    } finally {
+      document.body.removeChild(container);
+    }
+  });
+
+  it('should bubble non-native bubbling media events events', () => {
+    const container = document.createElement('div');
+    const ref = React.createRef();
+    const onPlay = jest.fn();
+    document.body.appendChild(container);
+    try {
+      ReactDOM.render(
+        <div onPlay={onPlay}>
+          <video ref={ref} onPlay={onPlay} />
+        </div>,
+        container,
+      );
       ref.current.dispatchEvent(
-        new Event('toggle', {
+        new Event('play', {
           bubbles: false,
         }),
       );
-      // Regression test: ensure we still emulate bubbling with non-bubbling
-      // media
       expect(onPlay).toHaveBeenCalledTimes(2);
-      expect(onCancel).toHaveBeenCalledTimes(2);
-      expect(onClose).toHaveBeenCalledTimes(2);
-      expect(onToggle).toHaveBeenCalledTimes(2);
+    } finally {
+      document.body.removeChild(container);
+    }
+  });
+
+  it('should bubble non-native bubbling invalid events', () => {
+    const container = document.createElement('div');
+    const ref = React.createRef();
+    const onInvalid = jest.fn();
+    document.body.appendChild(container);
+    try {
+      ReactDOM.render(
+        <form onInvalid={onInvalid}>
+          <input ref={ref} onInvalid={onInvalid} />
+        </form>,
+        container,
+      );
+      ref.current.dispatchEvent(
+        new Event('invalid', {
+          bubbles: false,
+        }),
+      );
+      expect(onInvalid).toHaveBeenCalledTimes(2);
     } finally {
       document.body.removeChild(container);
     }
diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js
@@ -384,6 +384,18 @@ export function listenToNativeEvent(
     !isCapturePhaseListener &&
     nonDelegatedEvents.has(topLevelType)
   ) {
+    // For all non-delegated events, apart from scroll, we attach
+    // their event listeners to the respective elements that their
+    // events fire on. That means we can skip this step, as event
+    // listener has already been added previously. However, we
+    // special case the scroll event because the reality is that any
+    // element can scroll.
+    // TODO: ideally, we'd eventually apply the same logic to all
+    // events from the nonDelegatedEvents list. Then we can remove
+    // this special case and use the same logic for all events.
+    if (topLevelType !== TOP_SCROLL) {
+      return;
+    }
     eventSystemFlags |= IS_NON_DELEGATED;
     target = targetElement;
   }
