Don’t add onclick listener to React root (#13778)

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.
This commit is contained in:
Philipp
2018-10-09 10:27:06 +02:00
committed by GitHub
parent b2cea9078d
commit f47a958ea8
3 changed files with 51 additions and 4 deletions
@@ -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');
});
});
});
+1 -1
View File
@@ -126,7 +126,7 @@ if (__DEV__) {
ReactControlledComponent.setRestoreImplementation(restoreControlledState);
type DOMContainer =
export type DOMContainer =
| (Element & {
_reactRootContainer: ?Root,
})
+10 -3
View File
@@ -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));
}