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

[devongovett]

React version: 17.0.0-rc.1

Steps To Reproduce

1. 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.
2. Quickly tap or click one checkbox or radio followed by another one.
3. 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.

[eps1lon]

Would be nice to also include native change handlers so that we can confirm whether this is a bug in React or rather a quirk with macOS/iOS. Not saying that React shouldn't fix these but narrowing the origin would help a lot.

Does this also reproduce if you associate the <label> with the input using htmlFor instead of nesting the input inside the label? Due to the nesting and the way label click works I could also imagine the issue being caused due to nesting.

[devongovett]

Happy to add those, but this did not occur in previous versions of React and does occur across browsers so I don't think it's a browser bug.

[devongovett]

Ok here's a version with native events as well as labels associated by ids rather than nesting. It appears to have the exact same behavior. https://codesandbox.io/s/lingering-star-vdnxi?file=/src/App.js

What's interesting is that the native "change" event listener is also fired on the incorrect element in react-dom 17.0.0-rc.1 but not in react-dom 17.0.0-rc.0. 🤯

Still digging. Is there a way to view a diff of changes between those versions? I couldn't find tags on GitHub for either of them.

[devongovett]

Update: I've built react locally and linked it into the same example as in the code sandbox. I was able to bisect the issue down to this commit: https://github.com/facebook/react/commit/b754caaaf23a070de281dcd0a9d32846470e1907. So it seems like something about enabling the eager listeners flag caused this issue to start occurring. Still not exactly sure what that enabled that would be causing this though.

[devongovett]

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

[gaearon]

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.

🤯 Thank you so much for tracking it down.

[gaearon]

Registering all listeners up front, no matter whether an app actually uses them might trigger other unexpected behaviors.

My thinking here is that if there's any unexpected behavior as a result of eager listening, it will also be unexpected when it triggers lazily because some leaf eventually registers the same event. So if anything, eager listening helps find these problems. We'll need to solve this somehow in either case.

[gaearon]

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]

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

[devongovett]

I have verified that it fixes the issue for me! 👍

[gaearon]

Do you know if the original issue happens in iPadOS 13?

[gaearon]

@devongovett How do you feel about filing an issue on the WebKit tracker for UI Events? It seems like a pretty invasive change but so far I could only repro it on iOS and it seems the worst there. But it seems like it's iOS 13+ so it might be a recent mistake and maybe they could fix it relatively fast. This isn't to deflect but we'd like to better understand what's going on on the iOS side and how involved the fix is there.

[devongovett]

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]

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

[gaearon]

I’ve spent the whole day trying different strategies and they all have some flaws. My conclusion is that:

1. This is a browser bug. It's not new in 17 (and it would appear any time someone uses onDoubleClick in their app, even in a transitive dependency). So it should not be considered a blocker for React 17, although we recognize it’s unfortunate we are triggering it more often now.

2. For iOS, we don't know if the behavior was intentional. Regardless, a Radar has been created for https://bugs.webkit.org/show_bug.cgi?id=216681, so I think we can expect to see updates there.

3. If the issue is affecting you, it is possible to work around it at the app level.

Where to Track Progress

• iOS: https://bugs.webkit.org/show_bug.cgi?id=216681
• Firefox: If you create a report please comment and we'll link it here

How to Work Around It?

If you don't care about onDoubleClick listeners, you can disable them altogether at the DOM level:

let oldAddEventListener = Node.prototype.addEventListener;
Node.prototype.addEventListener = function (type) {
  if (type === "dblclick") {
    // ignore
    return;
  }
  return oldAddEventListener.apply(this, arguments);
};

Here is a demo.

If you run this code before any code in your app, you will not be affected by this issue since it disables all doubleclick handlers. You may put this code behind something that detects iOS if you rely on doubleclicks on desktop. You may then use a ref with a native dblclick event as an escape hatch where necessary.

I expect that in longer term (maybe even soon) we’d see the iOS issue fixed and then we don’t need the hack. I’m going to close this because I don’t think the issue is actionable for us, and any workarounds we might add will lead to more frustration further on and wouldn’t solve the root problem.

[devongovett]

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?

[gaearon]

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.

[saschanaz]

although we were also able to reproduce in Firefox on macOS.

Do you mean Firefox also unchecks a wrong checkbox? So far I can only confirm that tapping two checkboxes fast enough via touchscreen fires dblclick at the last checkbox that was tapped. (But I'm on Windows)

[devongovett]

Do you mean Firefox also unchecks a wrong checkbox?

We thought so at first, but after testing further we could still get it to happen even in React 16, so I don't think it's the same issue. Definitely have to be much quicker in FF than iOS. I'm less concerned about that issue, and I haven't done too much debugging to find the root cause there.