Skip to content

Commit

Permalink
Don’t add onclick listener to React root (facebook#13778)
Browse files Browse the repository at this point in the history
Fixes facebook#13777

As part of facebook#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
facebook#12989 (comment)).

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.
  • Loading branch information
philipp-spiess authored and linjiajian999 committed Oct 22, 2018
1 parent 478eee3 commit e34b86e
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 4 deletions.
40 changes: 40 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
});
2 changes: 1 addition & 1 deletion packages/react-dom/src/client/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ if (__DEV__) {

ReactControlledComponent.setRestoreImplementation(restoreControlledState);

type DOMContainer =
export type DOMContainer =
| (Element & {
_reactRootContainer: ?Root,
})
Expand Down
13 changes: 10 additions & 3 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -342,7 +344,7 @@ export function appendChild(
}

export function appendChildToContainer(
container: Container,
container: DOMContainer,
child: Instance | TextInstance,
): void {
let parentNode;
Expand All @@ -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));
}
Expand Down

0 comments on commit e34b86e

Please sign in to comment.