Don’t add onclick listener to React root #13778
Conversation
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 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:  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.
c1cba56
to
45578e6
I think this is reasonable |
So the issue only appears when React root is bigger? Or when it is smaller? I didn't see this in testing. But I don't remember what I used for testing. |
Details of bundled changes.Comparing: d836010...ea25fe7 scheduler
Generated by |
Only when the React root is smaller than the viewport. In the gif you see the red flashing as long as the text does not cause the viewport to overflow. This (kinda) explains my strange observations here as well: #12717 (comment) |
@@ -341,8 +341,16 @@ export function appendChild( | |||
parentInstance.appendChild(child); | |||
} | |||
|
|||
type DOMContainer = |
philipp-spiess
Oct 4, 2018
Author
Member
I can also export this type from ReactDOM
if that makes more sense.
I can also export this type from ReactDOM
if that makes more sense.
gaearon
Oct 9, 2018
Member
Would make more sense than duplicating to me.
Would make more sense than duplicating to me.
@@ -360,7 +368,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) { |
acdlite
Oct 5, 2018
Member
Compare to null and undefined explicitly, plz
Compare to null and undefined explicitly, plz
@@ -360,7 +368,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. |
NE-SmallTown
Oct 5, 2018
Contributor
Should we modify the comments together?
Should we modify the comments together?
// https://github.com/facebook/react/issues/11918 | ||
if (!container._reactRootContainer && parentNode.onclick === null) { | ||
const {_reactRootContainer} = container; |
acdlite
Oct 5, 2018
Member
Another style nit: we don't use destructuring, with some exceptions like module-style objects or function args (component props).
Another style nit: we don't use destructuring, with some exceptions like module-style objects or function args (component props).
} | ||
|
||
ReactDOM.render(<Component />, container); | ||
expect(typeof portalContainer.onclick === 'function').toBeTruthy(); |
gaearon
Oct 9, 2018
Member
You can just do expect(typeof ...).toBe('function')
. Not that it matters.
You can just do expect(typeof ...).toBe('function')
. Not that it matters.
Feel free to merge |
(see nit in #13778 (comment)) |
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:  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.
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:  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.
Fixes facebook#14535 Alternative to facebook#13778 As @gaearon already noted, we can not rely on a container node having a `_reactRootContainer` to detect a React Root since the `createRoot()` API will not set it. Furthermore, the `createRoot()` API is currently only setting a property on the container in DEV mode. We could: 1. Set a property in prod as well. 2. Pass in more information into the `appendChildToContainer()` config. This PR is an attempt to implement 2. to visualize the outcome. It feels bad to do this though since non of the other renderers need that property and I’m not sure how stable the reconciler API is (i.e if we can just add properties like this). Let me know if you prefer 1. or have another idea.🙂
Fixes facebook#14535 Alternative to facebook#13778 As @gaearon already noted, we can not rely on a container node having a `_reactRootContainer` to detect a React Root since the `createRoot()` API will not set it. Furthermore, the `createRoot()` API is currently only setting a property on the container in DEV mode. We could: 1. Set a property in prod as well. 2. Pass in more information into the `appendChildToContainer()` config. This PR is an attempt to implement 2. to visualize the outcome. It feels bad to do this though since non of the other renderers need that property and I’m not sure how stable the reconciler API is (i.e if we can just add properties like this). Let me know if you prefer 1. or have another idea.🙂
Fixes facebook#14535 Related to facebook#13778 Alternative to facebook#14681 As @gaearon already noted, we can not rely on a container node having a `_reactRootContainer` to detect a React Root since the `createRoot()` API will not set it. Furthermore, the `createRoot()` API is currently only setting a property on the container in DEV mode. We could: 1. Set a property in prod as well. 2. Pass in more information into the `appendChildToContainer()` config. This PR is an attempt to implement 1. It feels better than [the other approach](facebook#14681) since we don't need to change the reconciler API.
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:
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.