-
Notifications
You must be signed in to change notification settings - Fork 47.4k
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
Consider removing Mobile Safari empty onclick hack #12989
Comments
I'd like to limit this to Safari. Looks like we still need this for non-interactive elements. I did a few quick checks using:
I wonder what is the best way to detect Safari (or generally when to apply this fix) |
Can we literally feature-test it? Like dispatching an event and seeing if it bubbles, or something? |
The problem with relying on useragent etc is that those can get messed up in webviews. |
Ah totally. I agree that we should feature detect this. I can check later today, but I wonder if it could be something like:
|
The feature test using manual event dispatching does not seem to work (here's what I used to test it). I was thinking that this bail out maybe happens in Safari's hit-detection logic and we could feature detect using |
Hello! Would I be correct in surmising this may plausibly be a good first contribution? Haven't really dug into it sufficiently, but it looks doable. If so, and if one of you isn't already working on it, any chance you could assign to me? |
@hectorcoronado Feel free to dig in. This will need some research — it's great if you can do it! |
I'll look into it in the coming days and update accordingly -- thanks! |
This doesn't seem to check whether our particular strategy of attaching it to every node with a click handler is necessary. It only checks that the click doesn't bubble to the root. But maybe we're doing more than we have to. |
This explains why #11927 works. But it also means we might get away with only setting |
To give some additional information: The "grey overlay" is a tap highlight that is shown when an element is pressed on iOS Safari. Safari will detect all interactive elements (e.g buttons) as tapable by default and adding an It's important UX wise to respect the proper granularity for adding these listeners, otherwise the tap highlight will be shown above the whole container: Back to the original topic I just realized that this is tightly intertwined with the tap highlight color so maybe a feasible feature test would check if the browser supports that? Maybe we can check if the |
@hectorcoronado Do you still want to work on this issue? You can try to explore the CSS property based feature-detection if you want 😊 (See my comment above) |
@philipp-spiess @gaearon yes! I will spend time on it after work tomorrow and will report back if it seems beyond my capabilities at present -- thank you! |
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.
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.
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.
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.
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.
@gaearon Maybe we could be more pragmatic and add the check only for iOS 12 and lower. This will of course cause some issues, but only for very small amount of user that are on iOS 12 or lower, and the user agent is modified and therefore we can't detect iOS anymore. Currently our application is public sector application, and accessibility certificate is a must. But as in #13798, we are facing this "clickable" issue. Another solution could be to provide a way to configure this behavior in the application code? |
Do we have a good way to detect iOS <= 13? I was just thinking about getting rid of this too a few days ago. |
@gaearon |
I don’t think we can ignore webviews. The cost of the mistake is pretty high — things literally won’t be clickable. |
FYI: Even desktop browsers implement Just setting So for clickable elements (elements with a click handler) we have to set an explicit And even then it's questionable if we truly want to see the tap highlight color in case the click handler isn't even used to make that specific element interactive (e.g. tracking clicks, click-away detection) With regards to calling click handlers: It doesn't look like we can safely remove this hack without affecting visual feedback on iOS which is unfortunate considering the empty click handler negatively affects NVDA + Firefox: nvaccess/nvda#13108 |
See #238 and #1536 for historical context.
Is that still relevant? The code is here:
react/packages/react-dom/src/client/ReactDOMFiberComponent.js
Lines 244 to 245 in 52fbe76
Even if it's relevant, can we just feature test it, and not do this hack on other browsers? Seems like a waste of memory for event handlers (even though the function is the same every time).
The text was updated successfully, but these errors were encountered: