From f47a958ea8d328190c88bac1ad6f85eaaaefc310 Mon Sep 17 00:00:00 2001
From: Philipp <hello@philippspiess.com>
Date: Tue, 9 Oct 2018 10:27:06 +0200
Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20add=20onclick=20listener=20to?=
 =?UTF-8?q?=20React=20root=20(#13778)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes #13777

As part of #11927 we introduced a regression by adding onclick handler
to the React root. This causes the whole React tree to flash when tapped
on iOS devices (for reasons I outlined in
https://github.com/facebook/react/issues/12989#issuecomment-414266839).

To fix this, we should only apply onclick listeners to portal roots. I
verified that my proposed fix indeed works by checking out our DOM
fixtures and adding regression tests.

Strangely, I had to make changes to the DOM fixtures to see the behavior
in the first place. This seems to be caused by our normal sites (and
thus their React root) being bigger than the viewport:

![](http://cl.ly/3f18f8b85e91/Screen%20Recording%202018-10-05%20at%2001.32%20AM.gif)

An alternative approach to finding out if we're appending to a React
root would be to add a third parameter to `appendChildToContainer` based
on the tag of the parent fiber.
---
 .../src/__tests__/ReactDOMComponent-test.js   | 40 +++++++++++++++++++
 packages/react-dom/src/client/ReactDOM.js     |  2 +-
 .../src/client/ReactDOMHostConfig.js          | 13 ++++--
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
index 95809b7a1b..e695a845ff 100644
--- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
@@ -2659,4 +2659,44 @@ describe('ReactDOMComponent', () => {
       document.body.removeChild(container);
     }
   });
+
+  describe('iOS Tap Highlight', () => {
+    it('adds onclick handler to elements with onClick prop', () => {
+      const container = document.createElement('div');
+
+      const elementRef = React.createRef();
+      function Component() {
+        return <div ref={elementRef} onClick={() => {}} />;
+      }
+
+      ReactDOM.render(<Component />, container);
+      expect(typeof elementRef.current.onclick).toBe('function');
+    });
+
+    it('adds onclick handler to a portal root', () => {
+      const container = document.createElement('div');
+      const portalContainer = document.createElement('div');
+
+      function Component() {
+        return ReactDOM.createPortal(
+          <div onClick={() => {}} />,
+          portalContainer,
+        );
+      }
+
+      ReactDOM.render(<Component />, container);
+      expect(typeof portalContainer.onclick).toBe('function');
+    });
+
+    it('does not add onclick handler to the React root', () => {
+      const container = document.createElement('div');
+
+      function Component() {
+        return <div onClick={() => {}} />;
+      }
+
+      ReactDOM.render(<Component />, container);
+      expect(typeof container.onclick).not.toBe('function');
+    });
+  });
 });
diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js
index f0cf8ea162..83563ee14f 100644
--- a/packages/react-dom/src/client/ReactDOM.js
+++ b/packages/react-dom/src/client/ReactDOM.js
@@ -126,7 +126,7 @@ if (__DEV__) {
 
 ReactControlledComponent.setRestoreImplementation(restoreControlledState);
 
-type DOMContainer =
+export type DOMContainer =
   | (Element & {
       _reactRootContainer: ?Root,
     })
diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js
index a9dd23f2fa..c5dc1f34d1 100644
--- a/packages/react-dom/src/client/ReactDOMHostConfig.js
+++ b/packages/react-dom/src/client/ReactDOMHostConfig.js
@@ -36,6 +36,8 @@ import {
   DOCUMENT_FRAGMENT_NODE,
 } from '../shared/HTMLNodeType';
 
+import type {DOMContainer} from './ReactDOM';
+
 export type Type = string;
 export type Props = {
   autoFocus?: boolean,
@@ -342,7 +344,7 @@ export function appendChild(
 }
 
 export function appendChildToContainer(
-  container: Container,
+  container: DOMContainer,
   child: Instance | TextInstance,
 ): void {
   let parentNode;
@@ -358,9 +360,14 @@ export function appendChildToContainer(
   // through the React tree. However, on Mobile Safari the click would
   // never bubble through the *DOM* tree unless an ancestor with onclick
   // event exists. So we wouldn't see it and dispatch it.
-  // This is why we ensure that containers have inline onclick defined.
+  // This is why we ensure that non React root containers have inline onclick
+  // defined.
   // https://github.com/facebook/react/issues/11918
-  if (parentNode.onclick === null) {
+  const reactRootContainer = container._reactRootContainer;
+  if (
+    (reactRootContainer === null || reactRootContainer === undefined) &&
+    parentNode.onclick === null
+  ) {
     // TODO: This cast may not be sound for SVG, MathML or custom elements.
     trapClickOnNonInteractiveElement(((parentNode: any): HTMLElement));
   }