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

Concurrent mode iOS hover behaviour #14535

Closed
subhero24 opened this issue Jan 5, 2019 · 3 comments
Closed

Concurrent mode iOS hover behaviour #14535

subhero24 opened this issue Jan 5, 2019 · 3 comments

Comments

@subhero24
Copy link

I hope this is the place to submit issues regarding concurrent mode.
I don't know if this is a bug or intended behaviour, but I have a situation where the my react app behaves differently depending on the use of concurrent mode.

Here is a link to a codesandbox demo: https://codesandbox.io/s/5xl0vnx11p

The behaviour on mobile Safari on iOS (12.1.2) is different whether using ReactDOM.render vs ReactDOM.createRoot. When ReactDOM.render is used, a tap results in an alert and after the alert is dismissed, the hover styles are applied. When using ReactDOM.createRoot, a tap results in the hover styles being applied. A second tap is needed to invoke the click handler.

(This double tap behaviour from iOS is described here)

Shouldn't the two render methods behave the same in this regard? or is this really intended behaviour?

BTW: A setTimout arround the mouseEnter event handler body does make ReactDOM.createRoot behave the same way as ReactDOM.render

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2019

I think maybe this check doesn't pass because _reactRootContainer field is only used by ReactDOM.render:

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));
}

philipp-spiess added a commit to philipp-spiess/react that referenced this issue Jan 23, 2019
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. 🙂
philipp-spiess added a commit to philipp-spiess/react that referenced this issue Jan 23, 2019
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. 🙂
philipp-spiess added a commit to philipp-spiess/react that referenced this issue Jan 24, 2019
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.
@gaearon
Copy link
Collaborator

gaearon commented Mar 24, 2021

I can't reproduce this anymore. Did iOS change something?

@subhero24
Copy link
Author

I also can not reproduce this issue anymore. Guess an iOS update fixed it. Thanks for looking into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants