Attach Listeners Eagerly to Roots and Portal Containers (#19659)

* Failing test for #19608 * Attach Listeners Eagerly to Roots and Portal Containers * Forbid createEventHandle with custom events We can't support this without adding more complexity. It's not clear that this is even desirable, as none of our existing use cases need custom events. This API primarily exists as a deprecation strategy for Flare, so I don't think it is important to expand its support beyond what Flare replacement code currently needs. We can later revisit it with a better understanding of the eager/lazy tradeoff but for now let's remove the inconsistency. * Reduce risk by changing condition only under the flag Co-authored-by: koba04 <[email protected]>
facebook · Aug 24, 2020 · 848bb24 · 848bb24
1 parent 8c9fc4e
commit 848bb24
Show file tree

Hide file tree

Showing 25 changed files with 527 additions and 233 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
@@ -398,18 +398,49 @@ describe('ReactDOMEventListener', () => {
     const originalDocAddEventListener = document.addEventListener;
     const originalRootAddEventListener = container.addEventListener;
     document.addEventListener = function(type) {
-      throw new Error(
-        `Did not expect to add a document-level listener for the "${type}" event.`,
-      );
+      switch (type) {
+        case 'selectionchange':
+          break;
+        default:
+          throw new Error(
+            `Did not expect to add a document-level listener for the "${type}" event.`,
+          );
+      }
     };
-    container.addEventListener = function(type) {
-      if (type === 'mouseout' || type === 'mouseover') {
-        // We currently listen to it unconditionally.
+    container.addEventListener = function(type, fn, options) {
+      if (options && (options === true || options.capture)) {
         return;
       }
-      throw new Error(
-        `Did not expect to add a root-level listener for the "${type}" event.`,
-      );
+      switch (type) {
+        case 'abort':
+        case 'canplay':
+        case 'canplaythrough':
+        case 'durationchange':
+        case 'emptied':
+        case 'encrypted':
+        case 'ended':
+        case 'error':
+        case 'loadeddata':
+        case 'loadedmetadata':
+        case 'loadstart':
+        case 'pause':
+        case 'play':
+        case 'playing':
+        case 'progress':
+        case 'ratechange':
+        case 'seeked':
+        case 'seeking':
+        case 'stalled':
+        case 'suspend':
+        case 'timeupdate':
+        case 'volumechange':
+        case 'waiting':
+          throw new Error(
+            `Did not expect to add a root-level listener for the "${type}" event.`,
+          );
+        default:
+          break;
+      }
     };
 
     try {

diff --git a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js
@@ -1040,6 +1040,25 @@ describe('ReactDOMFiber', () => {
     expect(ops).toEqual([]);
   });
 
+  // @gate enableEagerRootListeners
+  it('listens to events that do not exist in the Portal subtree', () => {
+    const onClick = jest.fn();
+
+    const ref = React.createRef();
+    ReactDOM.render(
+      <div onClick={onClick}>
+        {ReactDOM.createPortal(<button ref={ref}>click</button>, document.body)}
+      </div>,
+      container,
+    );
+    const event = new MouseEvent('click', {
+      bubbles: true,
+    });
+    ref.current.dispatchEvent(event);
+
+    expect(onClick).toHaveBeenCalledTimes(1);
+  });
+
   it('should throw on bad createPortal argument', () => {
     expect(() => {
       ReactDOM.createPortal(<div>portal</div>, null);

diff --git a/packages/react-dom/src/client/ReactDOMComponent.js b/packages/react-dom/src/client/ReactDOMComponent.js
@@ -74,7 +74,10 @@ import {validateProperties as validateInputProperties} from '../shared/ReactDOMN
 import {validateProperties as validateUnknownProperties} from '../shared/ReactDOMUnknownPropertyHook';
 import {REACT_OPAQUE_ID_TYPE} from 'shared/ReactSymbols';
 
-import {enableTrustedTypesIntegration} from 'shared/ReactFeatureFlags';
+import {
+  enableTrustedTypesIntegration,
+  enableEagerRootListeners,
+} from 'shared/ReactFeatureFlags';
 import {
   listenToReactEvent,
   mediaEventTypes,
@@ -260,30 +263,32 @@ export function ensureListeningTo(
   reactPropEvent: string,
   targetElement: Element | null,
 ): void {
-  // If we have a comment node, then use the parent node,
-  // which should be an element.
-  const rootContainerElement =
-    rootContainerInstance.nodeType === COMMENT_NODE
-      ? rootContainerInstance.parentNode
-      : rootContainerInstance;
-  if (__DEV__) {
-    if (
-      rootContainerElement == null ||
-      (rootContainerElement.nodeType !== ELEMENT_NODE &&
-        // This is to support rendering into a ShadowRoot:
-        rootContainerElement.nodeType !== DOCUMENT_FRAGMENT_NODE)
-    ) {
-      console.error(
-        'ensureListeningTo(): received a container that was not an element node. ' +
-          'This is likely a bug in React. Please file an issue.',
-      );
+  if (!enableEagerRootListeners) {
+    // If we have a comment node, then use the parent node,
+    // which should be an element.
+    const rootContainerElement =
+      rootContainerInstance.nodeType === COMMENT_NODE
+        ? rootContainerInstance.parentNode
+        : rootContainerInstance;
+    if (__DEV__) {
+      if (
+        rootContainerElement == null ||
+        (rootContainerElement.nodeType !== ELEMENT_NODE &&
+          // This is to support rendering into a ShadowRoot:
+          rootContainerElement.nodeType !== DOCUMENT_FRAGMENT_NODE)
+      ) {
+        console.error(
+          'ensureListeningTo(): received a container that was not an element node. ' +
+            'This is likely a bug in React. Please file an issue.',
+        );
+      }
     }
+    listenToReactEvent(
+      reactPropEvent,
+      ((rootContainerElement: any): Element),
+      targetElement,
+    );
   }
-  listenToReactEvent(
-    reactPropEvent,
-    ((rootContainerElement: any): Element),
-    targetElement,
-  );
 }
 
 function getOwnerDocumentFromRootContainer(
@@ -364,7 +369,11 @@ function setInitialDOMProperties(
         if (__DEV__ && typeof nextProp !== 'function') {
           warnForInvalidEventListener(propKey, nextProp);
         }
-        ensureListeningTo(rootContainerElement, propKey, domElement);
+        if (!enableEagerRootListeners) {
+          ensureListeningTo(rootContainerElement, propKey, domElement);
+        } else if (propKey === 'onScroll') {
+          listenToNonDelegatedEvent('scroll', domElement);
+        }
       }
     } else if (nextProp != null) {
       setValueForProperty(domElement, propKey, nextProp, isCustomComponentTag);
@@ -573,9 +582,11 @@ export function setInitialProperties(
       // We listen to this event in case to ensure emulated bubble
       // listeners still fire for the invalid event.
       listenToNonDelegatedEvent('invalid', domElement);
-      // For controlled components we always need to ensure we're listening
-      // to onChange. Even if there is no listener.
-      ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      if (!enableEagerRootListeners) {
+        // For controlled components we always need to ensure we're listening
+        // to onChange. Even if there is no listener.
+        ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      }
       break;
     case 'option':
       ReactDOMOptionValidateProps(domElement, rawProps);
@@ -587,19 +598,23 @@ export function setInitialProperties(
       // We listen to this event in case to ensure emulated bubble
       // listeners still fire for the invalid event.
       listenToNonDelegatedEvent('invalid', domElement);
-      // For controlled components we always need to ensure we're listening
-      // to onChange. Even if there is no listener.
-      ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      if (!enableEagerRootListeners) {
+        // For controlled components we always need to ensure we're listening
+        // to onChange. Even if there is no listener.
+        ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      }
       break;
     case 'textarea':
       ReactDOMTextareaInitWrapperState(domElement, rawProps);
       props = ReactDOMTextareaGetHostProps(domElement, rawProps);
       // We listen to this event in case to ensure emulated bubble
       // listeners still fire for the invalid event.
       listenToNonDelegatedEvent('invalid', domElement);
-      // For controlled components we always need to ensure we're listening
-      // to onChange. Even if there is no listener.
-      ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      if (!enableEagerRootListeners) {
+        // For controlled components we always need to ensure we're listening
+        // to onChange. Even if there is no listener.
+        ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      }
       break;
     default:
       props = rawProps;
@@ -817,7 +832,11 @@ export function diffProperties(
         if (__DEV__ && typeof nextProp !== 'function') {
           warnForInvalidEventListener(propKey, nextProp);
         }
-        ensureListeningTo(rootContainerElement, propKey, domElement);
+        if (!enableEagerRootListeners) {
+          ensureListeningTo(rootContainerElement, propKey, domElement);
+        } else if (propKey === 'onScroll') {
+          listenToNonDelegatedEvent('scroll', domElement);
+        }
       }
       if (!updatePayload && lastProp !== nextProp) {
         // This is a special case. If any listener updates we need to ensure
@@ -969,9 +988,11 @@ export function diffHydratedProperties(
       // We listen to this event in case to ensure emulated bubble
       // listeners still fire for the invalid event.
       listenToNonDelegatedEvent('invalid', domElement);
-      // For controlled components we always need to ensure we're listening
-      // to onChange. Even if there is no listener.
-      ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      if (!enableEagerRootListeners) {
+        // For controlled components we always need to ensure we're listening
+        // to onChange. Even if there is no listener.
+        ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      }
       break;
     case 'option':
       ReactDOMOptionValidateProps(domElement, rawProps);
@@ -981,18 +1002,22 @@ export function diffHydratedProperties(
       // We listen to this event in case to ensure emulated bubble
       // listeners still fire for the invalid event.
       listenToNonDelegatedEvent('invalid', domElement);
-      // For controlled components we always need to ensure we're listening
-      // to onChange. Even if there is no listener.
-      ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      if (!enableEagerRootListeners) {
+        // For controlled components we always need to ensure we're listening
+        // to onChange. Even if there is no listener.
+        ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      }
       break;
     case 'textarea':
       ReactDOMTextareaInitWrapperState(domElement, rawProps);
       // We listen to this event in case to ensure emulated bubble
       // listeners still fire for the invalid event.
       listenToNonDelegatedEvent('invalid', domElement);
-      // For controlled components we always need to ensure we're listening
-      // to onChange. Even if there is no listener.
-      ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      if (!enableEagerRootListeners) {
+        // For controlled components we always need to ensure we're listening
+        // to onChange. Even if there is no listener.
+        ensureListeningTo(rootContainerElement, 'onChange', domElement);
+      }
       break;
   }
 
@@ -1059,7 +1084,9 @@ export function diffHydratedProperties(
         if (__DEV__ && typeof nextProp !== 'function') {
           warnForInvalidEventListener(propKey, nextProp);
         }
-        ensureListeningTo(rootContainerElement, propKey, domElement);
+        if (!enableEagerRootListeners) {
+          ensureListeningTo(rootContainerElement, propKey, domElement);
+        }
       }
     } else if (
       __DEV__ &&

diff --git a/packages/react-dom/src/client/ReactDOMEventHandle.js b/packages/react-dom/src/client/ReactDOMEventHandle.js
@@ -15,6 +15,7 @@ import type {
 } from '../shared/ReactDOMTypes';
 
 import {getEventPriorityForListenerSystem} from '../events/DOMEventProperties';
+import {allNativeEvents} from '../events/EventRegistry';
 import {
   getClosestInstanceFromNode,
   getEventHandlerListeners,
@@ -35,6 +36,7 @@ import {IS_EVENT_HANDLE_NON_MANAGED_NODE} from '../events/EventSystemFlags';
 import {
   enableScopeAPI,
   enableCreateEventHandleAPI,
+  enableEagerRootListeners,
 } from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 
@@ -178,6 +180,26 @@ export function createEventHandle(
 ): ReactDOMEventHandle {
   if (enableCreateEventHandleAPI) {
     const domEventName = ((type: any): DOMEventName);
+
+    if (enableEagerRootListeners) {
+      // We cannot support arbitrary native events with eager root listeners
+      // because the eager strategy relies on knowing the whole list ahead of time.
+      // If we wanted to support this, we'd have to add code to keep track
+      // (or search) for all portal and root containers, and lazily add listeners
+      // to them whenever we see a previously unknown event. This seems like a lot
+      // of complexity for something we don't even have a particular use case for.
+      // Unfortunately, the downside of this invariant is that *removing* a native
+      // event from the list of known events has now become a breaking change for
+      // any code relying on the createEventHandle API.
+      invariant(
+        allNativeEvents.has(domEventName) ||
+          domEventName === 'beforeblur' ||
+          domEventName === 'afterblur',
+        'Cannot call unstable_createEventHandle with "%s", as it is not an event known to React.',
+        domEventName,
+      );
+    }
+
     let isCapturePhaseListener = false;
     let isPassiveListener = undefined; // Undefined means to use the browser default
     let listenerPriority;

diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js
@@ -67,9 +67,13 @@ import {
   enableFundamentalAPI,
   enableCreateEventHandleAPI,
   enableScopeAPI,
+  enableEagerRootListeners,
 } from 'shared/ReactFeatureFlags';
 import {HostComponent, HostText} from 'react-reconciler/src/ReactWorkTags';
-import {listenToReactEvent} from '../events/DOMPluginEventSystem';
+import {
+  listenToReactEvent,
+  listenToAllSupportedEvents,
+} from '../events/DOMPluginEventSystem';
 
 export type Type = string;
 export type Props = {
@@ -1069,7 +1073,11 @@ export function makeOpaqueHydratingObject(
 }
 
 export function preparePortalMount(portalInstance: Instance): void {
-  listenToReactEvent('onMouseEnter', portalInstance, null);
+  if (enableEagerRootListeners) {
+    listenToAllSupportedEvents(portalInstance);
+  } else {
+    listenToReactEvent('onMouseEnter', portalInstance, null);
+  }
 }
 
 export function prepareScopeUpdate(