react: Bug: React 17.0.0-rc.1 checkboxes and radio groups sometimes fire onChange incorrectly

React version: 17.0.0-rc.1

Steps To Reproduce

Open https://9sf7d.csb.app/. The issue seems most easily reproducible in iOS Safari, although we were also able to reproduce in Firefox on macOS.
Quickly tap or click one checkbox or radio followed by another one.
Notice that sometimes the first checkbox is unchecked rather than the second checkbox you tapped on becoming checked. For radios, the first radio stays selected rather than switching to the radio you tapped on.

Link to code example: https://codesandbox.io/s/optimistic-sound-9sf7d?file=/src/App.js

The current behavior

There appears to be some sort of race condition where tapping or clicking on a controlled <input type="checkbox"> or <input type="radio"> quickly after clicking on a previous input does not fire onChange on the correct element. As far as I can tell, onChange is being fired on the second checkbox rather than the first. It appears to be a timing issue - if you wait long enough between taps, the events are fired on the correct elements. This also appears to only reproduce in React 17.0.0-rc.1, not 17.0.0-rc.0 or React 16 (you can verify by changing the versions in Code Sandbox).

The expected behavior

The onChange event should fire on the correct element, and state should update to check or uncheck the checkbox or radio you tapped, not some other element.

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 19 (9 by maintainers)

Most upvoted comments

Here is an example demonstrating the same issue with React 16 as soon as you add an onDoubleClick event anywhere in the tree. Which shows it’s not 17-specific but 17 makes it visible.

https://codesandbox.io/s/xenodochial-rain-10d5e?file=/src/App.js https://10d5e.csb.app/

gaearon on Sep 17, 2020

The only part I haven’t figured out yet that would make this work in a reusable library would be a way to find the nearest react root programmatically in order to apply this automatically

Seems a bit heavy-handed as adding a component usually doesn’t affect something up the tree.

One way you could go about it is maybe have a root component that you require people to wrap the app into (eg it has some context) and then include this style as a part of its output.

Or maybe traverse in DEV only upwards and get computed style. Warn if property not found but you reach the top.

gaearon on Sep 18, 2020

This is reasonable given that it also occurred in 16 if you had a double click listener anywhere on the page.

A slightly less hacky workaround, based on my reading of the webkit patch is to set touch-action: manipulation (or anything other than auto) on the React root element (and all portal roots). This disables the double click handler from running. Unfortunately, it must be on the root element where the dblclick listener is registered, and not on the checkboxes themselves due to the way the webkit code is written. Here’s an example.

Note that this will also disable double tap to zoom, but perhaps that is ok if the app already has a mobile viewport metatag. If that’s not an option, I’ve also tested adding touch-action: manipulation on click on the checkbox/radio and removing it after a short delay. This way, double tap to zoom works elsewhere but not when tapping on a checkbox or radio. Example of that.

The only part I haven’t figured out yet that would make this work in a reusable library would be a way to find the nearest react root programmatically in order to apply this automatically. Seems a bit hacky for React itself to do this, but I could potentially see us doing this in our component library as a temporary workaround until the iOS bug is fixed. Any recommendations on how to do that in userland?

devongovett on Sep 18, 2020

Filed: https://bugs.webkit.org/show_bug.cgi?id=216681

devongovett on Sep 18, 2020

We reproduced the issue on both iOS/iPadOS 13 and 14. I don’t think we tested on anything earlier than 13. I’m happy to file a bug with them as this is definitely a browser issue that only happened to be surfaced by the eager event change.

A quick search of the webkit source revealed this line which looks potentially problematic to my untrained eyes. Traced it back to this bug and associated patch, which looks like it was fixing double tapping on a photo for instagram.com. 😉

devongovett on Sep 18, 2020

Can you give this build some testing? I think it fixes it. https://github.com/facebook/react/pull/19853

gaearon on Sep 17, 2020

Debugging further, I’ve determined that the issue is the dblclick listener that is registered on the React root as part of the eager event binding. Commenting out this line fixes the issue: https://github.com/facebook/react/blob/36df9185c53ce4e90dc1f7362f74533ecf0607db/packages/react-dom/src/events/DOMEventProperties.js#L53

My guess is the browsers are a bit lenient when it comes to hit testing during a double click. So tapping within the double click time threshold is firing the event on the same element that started the double tap, toggling the checkbox back off rather than toggling the checkbox you actually tapped on. I guess browsers only implement this behavior when a double click listener is actually registered, so that’s why enabling the eager listeners caused the issue to appear.

This also makes me wonder what other behavior browsers enable depending on whether a listener is registered. Registering all listeners up front, no matter whether an app actually uses them might trigger other unexpected behaviors.

@gaearon not totally sure how best to fix this. I understand the original issue (#19608) was with portals and #19659 was meant to fix that. Maybe the registered events could be propagated downward to portal roots? Rather than registering all possible events, only register events that are actually used in the portal or all parent roots? I imagine this would require significantly more bookkeeping though…

devongovett on Sep 16, 2020