Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The gray overlay when tap the react root container #13777

Closed
amikitevich opened this issue Oct 4, 2018 · 2 comments · Fixed by #13778
Closed

The gray overlay when tap the react root container #13777

amikitevich opened this issue Oct 4, 2018 · 2 comments · Fixed by #13778

Comments

@amikitevich
Copy link

Do you want to request a feature or report a bug?
bug

What is the current behavior?

Since [email protected] seems that root element has onclick event handler with noop function.
In ios Safari/Chrome browsers html element has gray overlay when tap on it.

The current behavior is that the whole root is blinking when we tap on it.

The simple example to reproduce:
https://codesandbox.io/s/wyo8rlj1zk
The issue is reproducing only in fullscreen mode https://wyo8rlj1zk.codesandbox.io/
(root container is steel blue)

I suppose changes came from here:
#11927
https://github.com/facebook/react/pull/11927/files

and the issue can be related with:
mui/material-ui#11154
#12717

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
React 16.5
ios 11.3/12 Safari/Chrome

philipp-spiess added a commit to philipp-spiess/react that referenced this issue Oct 4, 2018
Fixes facebook#13777

As part of facebook#11927 we introduced a regression by adding onclick handler
to the React root as well. 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.
philipp-spiess added a commit to philipp-spiess/react that referenced this issue Oct 4, 2018
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.
@philipp-spiess
Copy link
Contributor

@amikitevich Thanks for reporting this. I agree that we shouldn't add onclick handler to React roots. Seems that we did not spot this regression in manual tests because of Safari hiding tap highlights when an element is bigger than the viewport 🤔. Check out #13778.

@amikitevich
Copy link
Author

@philipp-spiess very appreciated for your explanation, help and the fix. Thank you! 👍

philipp-spiess added a commit that referenced this issue Oct 9, 2018
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
#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.
linjiajian999 pushed a commit to linjiajian999/react that referenced this issue Oct 22, 2018
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.
jetoneza pushed a commit to jetoneza/react that referenced this issue Jan 23, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants