Fix bug with enableLegacyFBSupport click handlers (#19378)

facebook · Jul 16, 2020 · e387c98 · e387c98
1 parent a226b9b
commit e387c98
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 32 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOM-test.js b/packages/react-dom/src/__tests__/ReactDOM-test.js
@@ -13,7 +13,6 @@ let React;
 let ReactDOM;
 let ReactDOMServer;
 let ReactTestUtils;
-const ReactFeatureFlags = require('shared/ReactFeatureFlags');
 
 describe('ReactDOM', () => {
   beforeEach(() => {
@@ -355,27 +354,11 @@ describe('ReactDOM', () => {
     document.body.appendChild(container);
     try {
       ReactDOM.render(<Wrapper />, container);
-      let expected;
-
-      if (ReactFeatureFlags.enableLegacyFBSupport) {
-        // We expect to duplicate the 2nd handler because this test is
-        // not really designed around how the legacy FB support system works.
-        // This is because the above test sync fires a click() event
-        // during that of another click event, which causes the FB support system
-        // to duplicate adding an event listener. In practice this would never
-        // happen, as we only apply the legacy FB logic for "click" events,
-        // which would never stack this way in product code.
-        expected = [
-          '1st node clicked',
-          "2nd node clicked imperatively from 1st's handler",
-          "2nd node clicked imperatively from 1st's handler",
-        ];
-      } else {
-        expected = [
-          '1st node clicked',
-          "2nd node clicked imperatively from 1st's handler",
-        ];
-      }
+
+      const expected = [
+        '1st node clicked',
+        "2nd node clicked imperatively from 1st's handler",
+      ];
 
       expect(actual).toEqual(expected);
     } finally {

diff --git a/packages/react-dom/src/events/DOMModernPluginEventSystem.js b/packages/react-dom/src/events/DOMModernPluginEventSystem.js
@@ -478,16 +478,13 @@ function addTrappedEventListener(
   if (enableLegacyFBSupport && isDeferredListenerForLegacyFBSupport) {
     const originalListener = listener;
     listener = function(...p) {
-      try {
-        return originalListener.apply(this, p);
-      } finally {
-        removeEventListener(
-          targetContainer,
-          rawEventName,
-          unsubscribeListener,
-          isCapturePhaseListener,
-        );
-      }
+      removeEventListener(
+        targetContainer,
+        rawEventName,
+        unsubscribeListener,
+        isCapturePhaseListener,
+      );
+      return originalListener.apply(this, p);
     };
   }
   if (isCapturePhaseListener) {

diff --git a/packages/react-dom/src/events/__tests__/DOMModernPluginEventSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMModernPluginEventSystem-test.internal.js
@@ -143,6 +143,39 @@ describe('DOMModernPluginEventSystem', () => {
           expect(log[5]).toEqual(['bubble', buttonElement]);
         });
 
+        it('handle propagation of click events combined with sync clicks', () => {
+          const buttonRef = React.createRef();
+          let clicks = 0;
+
+          function Test() {
+            const inputRef = React.useRef(null);
+            return (
+              <div>
+                <button
+                  ref={buttonRef}
+                  onClick={() => {
+                    // Sync click
+                    inputRef.current.click();
+                  }}
+                />
+                <input
+                  ref={inputRef}
+                  onClick={() => {
+                    clicks++;
+                  }}
+                />
+              </div>
+            );
+          }
+
+          ReactDOM.render(<Test />, container);
+
+          const buttonElement = buttonRef.current;
+          dispatchClickEvent(buttonElement);
+
+          expect(clicks).toBe(1);
+        });
+
         it('handle propagation of click events between roots', () => {
           const buttonRef = React.createRef();
           const divRef = React.createRef();