From 45578e6dad14d9e205b3b8021569fc3fe9530c19 Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Fri, 5 Oct 2018 01:34:02 +0200 Subject: [PATCH 1/6] =?UTF-8?q?Don=E2=80=99t=20add=20onclick=20listener=20?= =?UTF-8?q?to=20React=20root?= 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 this fix indeed worked by checkout out our DOM fixtures and added regression tests as well. 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 being bigger than the viewport: ![](http://cl.ly/3f18f8b85e91/Screen%20Recording%202018-10-05%20at%2001.32%20AM.gif) An alternative fix would be to add a third parameter to `appendChildToContainer` based on the tag of the parent fiber. Although I think relying on the `_reactRootContainer` property that we set on the element is less intrusive. --- .../src/__tests__/ReactDOMComponent-test.js | 37 +++++++++++++++++++ .../src/client/ReactDOMHostConfig.js | 2 +- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js index 95809b7a1b5b4..1113694361ac6 100644 --- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js @@ -2659,4 +2659,41 @@ 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
{}} />; + } + + ReactDOM.render(, container); + expect(typeof elementRef.current.onclick === 'function').toBeTruthy(); + }); + + it('adds onclick handler to a portal root', () => { + const container = document.createElement('div'); + const portalContainer = document.createElement('div'); + + function Component() { + return ReactDOM.createPortal(
{}} />, portalContainer); + } + + ReactDOM.render(, container); + expect(typeof portalContainer.onclick === 'function').toBeTruthy(); + }); + + it('does not add onclick handler to the React root', () => { + const container = document.createElement('div'); + + function Component() { + return
{}} />; + } + + ReactDOM.render(, container); + expect(typeof container.onclick === 'function').toBeFalsy(); + }); + }); }); diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index a9dd23f2fac3c..827bb1896fde2 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -360,7 +360,7 @@ export function appendChildToContainer( // event exists. So we wouldn't see it and dispatch it. // This is why we ensure that containers have inline onclick defined. // https://github.com/facebook/react/issues/11918 - if (parentNode.onclick === null) { + if (!container._reactRootContainer && parentNode.onclick === null) { // TODO: This cast may not be sound for SVG, MathML or custom elements. trapClickOnNonInteractiveElement(((parentNode: any): HTMLElement)); } From f78da302557be4ace1f9c6a3f66de53095b26efa Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Fri, 5 Oct 2018 01:55:44 +0200 Subject: [PATCH 2/6] Make Flow happy by giving it more context --- packages/react-dom/src/client/ReactDOMHostConfig.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 827bb1896fde2..93fc9b5015685 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -341,8 +341,16 @@ export function appendChild( parentInstance.appendChild(child); } +type DOMContainer = + | (Element & { + _reactRootContainer: ?{}, + }) + | (Document & { + _reactRootContainer: ?{}, + }); + export function appendChildToContainer( - container: Container, + container: DOMContainer, child: Instance | TextInstance, ): void { let parentNode; From 965b17ce6c945c8cc7401659970599b2788c1034 Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Fri, 5 Oct 2018 02:07:31 +0200 Subject: [PATCH 3/6] =?UTF-8?q?It=E2=80=99s=20time=20to=20go=20to=20bed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/react-dom/src/__tests__/ReactDOMComponent-test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js index 1113694361ac6..c0d8e4dafe189 100644 --- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js @@ -2678,7 +2678,10 @@ describe('ReactDOMComponent', () => { const portalContainer = document.createElement('div'); function Component() { - return ReactDOM.createPortal(
{}} />, portalContainer); + return ReactDOM.createPortal( +
{}} />, + portalContainer, + ); } ReactDOM.render(, container); From f0c2702cf3c5533f76dec295acbf157fc87e328a Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Fri, 5 Oct 2018 10:25:38 +0200 Subject: [PATCH 4/6] Check for null and undefined explicitly --- packages/react-dom/src/client/ReactDOMHostConfig.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 93fc9b5015685..bd12228c129e0 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -366,9 +366,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 (!container._reactRootContainer && parentNode.onclick === null) { + const {_reactRootContainer} = container; + 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)); } From 4cf7175f55c3c6bc5d050c69abf1e7138fcadd44 Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Fri, 5 Oct 2018 20:18:55 +0200 Subject: [PATCH 5/6] We don't use destructuring --- packages/react-dom/src/client/ReactDOMHostConfig.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index bd12228c129e0..dd756a33c6866 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -369,9 +369,9 @@ export function appendChildToContainer( // This is why we ensure that non React root containers have inline onclick // defined. // https://github.com/facebook/react/issues/11918 - const {_reactRootContainer} = container; + const reactRootContainer = container._reactRootContainer; if ( - (_reactRootContainer === null || _reactRootContainer === undefined) && + (reactRootContainer === null || reactRootContainer === undefined) && parentNode.onclick === null ) { // TODO: This cast may not be sound for SVG, MathML or custom elements. From ea25fe711488adaf012f06c3af4a9e432863f4cb Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Tue, 9 Oct 2018 10:14:04 +0200 Subject: [PATCH 6/6] Cleanup DOMContainer usage and test assertions --- .../react-dom/src/__tests__/ReactDOMComponent-test.js | 6 +++--- packages/react-dom/src/client/ReactDOM.js | 2 +- packages/react-dom/src/client/ReactDOMHostConfig.js | 10 ++-------- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js index c0d8e4dafe189..e695a845ff505 100644 --- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js @@ -2670,7 +2670,7 @@ describe('ReactDOMComponent', () => { } ReactDOM.render(, container); - expect(typeof elementRef.current.onclick === 'function').toBeTruthy(); + expect(typeof elementRef.current.onclick).toBe('function'); }); it('adds onclick handler to a portal root', () => { @@ -2685,7 +2685,7 @@ describe('ReactDOMComponent', () => { } ReactDOM.render(, container); - expect(typeof portalContainer.onclick === 'function').toBeTruthy(); + expect(typeof portalContainer.onclick).toBe('function'); }); it('does not add onclick handler to the React root', () => { @@ -2696,7 +2696,7 @@ describe('ReactDOMComponent', () => { } ReactDOM.render(, container); - expect(typeof container.onclick === 'function').toBeFalsy(); + 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 f0cf8ea162795..83563ee14f9d5 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 dd756a33c6866..c5dc1f34d1267 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, @@ -341,14 +343,6 @@ export function appendChild( parentInstance.appendChild(child); } -type DOMContainer = - | (Element & { - _reactRootContainer: ?{}, - }) - | (Document & { - _reactRootContainer: ?{}, - }); - export function appendChildToContainer( container: DOMContainer, child: Instance | TextInstance,