Skip to content

Commit

Permalink
Don’t add onclick listener to React root
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 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.
  • Loading branch information
philipp-spiess committed Oct 4, 2018
1 parent d836010 commit 45578e6
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 1 deletion.
37 changes: 37 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
Expand Up @@ -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 <div ref={elementRef} onClick={() => {}} />;
}

ReactDOM.render(<Component />, 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(<div onClick={() => {}} />, portalContainer);
}

ReactDOM.render(<Component />, 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 <div onClick={() => {}} />;
}

ReactDOM.render(<Component />, container);
expect(typeof container.onclick === 'function').toBeFalsy();
});
});
});
2 changes: 1 addition & 1 deletion packages/react-dom/src/client/ReactDOMHostConfig.js
Expand Up @@ -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));
}
Expand Down

0 comments on commit 45578e6

Please sign in to comment.