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

Consider removing Mobile Safari empty onclick hack #12989

Open
gaearon opened this issue Jun 7, 2018 · 18 comments
Open

Consider removing Mobile Safari empty onclick hack #12989

gaearon opened this issue Jun 7, 2018 · 18 comments
Labels
Component: DOM React Core Team Opened by a member of the React Core Team Type: Needs Investigation

Comments

@gaearon
Copy link
Collaborator

gaearon commented Jun 7, 2018

See #238 and #1536 for historical context.

Is that still relevant? The code is here:

// TODO: Only do this for the relevant Safaris maybe?
node.onclick = emptyFunction;

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).

@nhunzaker
Copy link
Contributor

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:
https://codepen.io/nhunzaker/pen/yEMpJO?editors=1010

  • This definitely is still an issue on iOS 11
  • It looks like this isn't a problem in desktop Safari (7.1, 8, 11.1)

I wonder what is the best way to detect Safari (or generally when to apply this fix)

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 11, 2018

Can we literally feature-test it? Like dispatching an event and seeing if it bubbles, or something?

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 11, 2018

The problem with relying on useragent etc is that those can get messed up in webviews.

@nhunzaker
Copy link
Contributor

nhunzaker commented Jun 11, 2018

Ah totally. I agree that we should feature detect this. I can check later today, but I wonder if it could be something like:

let noBubble = true

// where should document come from?

let parent = document.createElement('div')
let child = document.createElement('div')
let event = document.createEvent('Event')

parent.appendChild(child)

event.initEvent('click', true, false)

parent.addEventListener('click', () => {
  noBubble = false
})

child.dispatchEvent(event);

@philipp-spiess
Copy link
Contributor

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 elementFromPoint() but that also behaved correctly. 🤔

@hectorcoronado
Copy link

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?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 2, 2018

@hectorcoronado Feel free to dig in. This will need some research — it's great if you can do it!

@hectorcoronado
Copy link

I'll look into it in the coming days and update accordingly -- thanks!

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 17, 2018

I did a few quick checks using:
https://codepen.io/nhunzaker/pen/yEMpJO?editors=1010
This definitely is still an issue on iOS 11

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.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 17, 2018

Oh...

The target element, or any of its ancestors up to but not including the , has an explicit event handler set for any of the mouse events. This event handler may be an empty function.

This explains why #11927 works. But it also means we might get away with only setting onclick on the root if the click delay / grey overlay issue is solved in version we support.

@philipp-spiess
Copy link
Contributor

But it also means we might get away with only setting onclick on the root if the click delay / grey overlay issue is solved in version we support.

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 onclick handler will also make non-interactive elements (e.g div) tapable.

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:

(Source code)


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 -webkit-tap-highlight-color CSS property is supported.

@philipp-spiess
Copy link
Contributor

@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)

@hectorcoronado
Copy link

@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!

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 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.
@necolas necolas added the React Core Team Opened by a member of the React Core Team label Jan 8, 2020
@binhpv
Copy link

binhpv commented Mar 30, 2021

@gaearon
I tested several iOS simulator and found out that from iOS 13 and higher, the click event bubbling issue is not an issue anymore. This test was done following test cases in https://www.quirksmode.org/blog/archives/2010/09/click_event_del.html

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?

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 30, 2021

Do we have a good way to detect iOS <= 13? I was just thinking about getting rid of this too a few days ago.

@binhpv
Copy link

binhpv commented Apr 3, 2021

@gaearon
Using user agent detection like https://stackoverflow.com/questions/57599945/how-to-detect-ios-13-on-javascript, if we ignore those webview modified user agent :D

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 3, 2021

I don’t think we can ignore webviews. The cost of the mistake is pretty high — things literally won’t be clickable.

@eps1lon
Copy link
Collaborator

eps1lon commented Nov 30, 2021

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 -webkit-tap-highlight-color CSS property is supported.

FYI: Even desktop browsers implement -webkit-tap-highlight-color property and actually have values set for this property (window.getComputedStyle(document.body).webkitTapHighlightColor). So a naive webkitTapHighlightColor in document.body.style and even window.getComputedStyle(document.body).webkitTapHighlightColor !== '' would not tell you whether this color is applied when tapping.

Just setting -webkit-tap-highlight-color: transparent on React containers would also not work since that CSS property is unfortunately inherited i.e. #react-root button would also have a transparent tap highlight color. Note that MDN says it's not inherited but that's not how Chrome 96 implements it: https://codesandbox.io/s/wizardly-vaughan-2cbh5?file=/src/App.js. Though we can prevent inheritance with #react-root > * { -webkit-tap-highlight-color: initial; }.

So for clickable elements (elements with a click handler) we have to set an explicit node.onclick unless we find a robust way to implement browser heuristics.

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:
The event delegation quirk only applies when only document.body has the click handler. If you have a click handler on any other element then iOS will properly bubble click events up even when non-interactive elements are targeted. Tested on Safari iOS 9 (iPhone 6s): https://csb-sel0s-31qhjklet-eps1lon.vercel.app/ Note how only a click on the last fake button doesn't register. Every other click is registered because its (fake) react root has a click handler attached.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: DOM React Core Team Opened by a member of the React Core Team Type: Needs Investigation
Projects
None yet
Development

No branches or pull requests

9 participants
@necolas @philipp-spiess @nhunzaker @gaearon @binhpv @eps1lon @hectorcoronado and others