Update tracked value after resetting radio group

[sophiebits]

Fixes #26876, I think. Review each commit separately (all assertions pass in main already, except the last assertInputTrackingIsClean in "should control radio buttons").

I'm actually a little confused on two things here:

• All the isCheckedDirty assertions are true. But I don't think we set .checked unconditionally? So how does this happen?
• Bug: Radio button onChange not called in current React Canary #26876 (comment) claims that d962f35...1f248bd contains the faulty change, but it doesn't appear to change the restoration logic that I've touched here. (One difference outside restoration is that updateProperties did previously set .checked when nextProp !== lastProp whereas the new logic in updateInput is to set it when node.checked !== !!checked.)

But it seems to me like we need this call here anyway, and if it fixes it then it fixes it? I think technically speaking we probably should do all the updateInput() calls and then all the updateValueIfChanged() calls—in particular I think if clicking A changed the checked radio button from B to C then the code as I have it would be incorrect, but that also seems unlikely so idk whether to care.

cc @zhengjitf @Luk-z who did some investigation on the original issue

[react-sizebot]

Comparing: 2807d78...26daa54

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	+0.03%	166.62 kB	166.66 kB	+0.02%	52.13 kB	52.14 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.03%	176.05 kB	176.10 kB	+0.01%	54.97 kB	54.97 kB
facebook-www/ReactDOM-prod.classic.js	+0.04%	571.73 kB	571.96 kB	+0.02%	100.64 kB	100.66 kB
facebook-www/ReactDOM-prod.modern.js	+0.04%	555.46 kB	555.68 kB	+0.02%	97.75 kB	97.77 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 26daa54

[eps1lon]

This also fixes #19860 😮 : https://codesandbox.io/s/react-18-radio-cannot-be-read-only-forked-vrh6v2?file=/src/App.js

I don't have much context about this type of code but the fact that it fixes the original issue and another one, is a pretty strong signal for me.

[sebmarkbage]

All the isCheckedDirty assertions are true. But I don't think we set .checked unconditionally? So how does this happen?

This is unconditionally set:

https://github.com/facebook/react/blob/main/packages/react-dom-bindings/src/client/ReactDOMInput.js#L301

I don't think we do for value which is a suspicious asymmetry that I've noted before.

[sophiebits]

Hmm it does seem like there is probably a hydration bug too based on reports at vercel/next.js#49499.

[sebmarkbage]

The isHydrating check is suspicious. I don't think that's quite right. If we do need to set it e.g. to set the dirty flag or update the input value tracking, then we'd have to do that for hydration too. The reason we don't want to do it for hydration is so that it doesn't reset. But maybe what we need to do is just element.value = element.value or element.checked = element.checked.

[sophiebits]

I think I'll update this to call updateValueIfChanged() on all the inputs after calling updateInput() on all of them. Will also rename "assertInputTrackingIsClean" to not use the word "Clean" since that could seem related to "Dirty" but it's not.

In a separate PR I'll plan to add a test for hydration. My expectation is if you interact with radios before the page hydrates then onChange will not fire for that but it will work normally following that (meaning the dirty flag needs to be set and value tracking needs to be up to date).

[sebmarkbage]

I kind of think onChange should fire after the fact on inputs after hydration if it changed. At least for controlled so that it can enter the correct state. We had the same issue that onLoad for img should probably fire if it loaded before hydration. Maybe for 19? We currently don't really have a commit effect for hydration on DOM nodes which limits stuff like this.

[Luk-z]

Sorry for late response.
Tried with 18.3.0-canary-d900fadbf-20230929 the problem still persists.
Working example with 18.3.0-next-d962f35ca-20230418
Not working example with 18.3.0-canary-d900fadbf-20230929

@sophiebits didn't know if this PR is anymore the right place to discuss about (also saw that #26876 was closed )

[sophiebits]

@Luk-z Thank you for your diligence. I have now posted a fix in #27443 and verified that it fixes this repro case:

https://codesandbox.io/s/react-canary-radio-buttons-latest-not-working-forked-t2wwyc

[Luk-z]

@sophiebits I confirm that it works now, thanks!