Bug: Radio button onChange not called in current React Canary

[robrichard]

React version: 18.3.0-canary-a1f97589f-20230526

Steps To Reproduce

1. Create radio buttons that toggle disabled in onChange
2. After selecting each radio button, onChange is no longer called

Link to code example:

The following CodeSandbox demonstrates the issue with the current react canary version. The issue is not present when react & react-dom versions are changed to stable 18.2.0

https://codesandbox.io/s/react-canary-radio-buttons-deiqb3?file=/src/App.js

The current behavior

<input type="radio" />'s onChange prop is not called on subsequent clicks of the input

The expected behavior

<input type="radio" />'s onChange prop should be called on subsequent clicks of the input

[eps1lon]

As far as I an tell this also repros without toggling disabled: https://codesandbox.io/s/react-canary-radio-buttons-forked-ydjdlp?file=/src/App.js

So it's just about changing the controlled checked delayed.

[Luk-z]

The problem was introduced with 18.3.0-next-1f248bdd7-20230419. The previous version 18.3.0-next-d962f35ca-20230418 works.

[robrichard]

It looks like it was this change that introduced this bug: #26667

[kambleaa007]

what are ways to fix this issue?

[zhengjitf]

FYI, I find something by investigating, as below:

extractEvents:

react/packages/react-dom-bindings/src/events/plugins/ChangeEventPlugin.js

Lines 322 to 331 in 6aacd3f

	const inst = getTargetInstFunc(domEventName, targetInst);
	if (inst) {
	createAndAccumulateChangeEvent(
	dispatchQueue,
	inst,
	nativeEvent,
	nativeEventTarget,
	);
	return;
	}

onChange isn't called because inst here is falsy in that process. PS: In our case, getTargetInstFunc should be equal to getTargetInstForClickEvent.

Diving into the inner functions as the followings.

getTargetInstForClickEvent:

react/packages/react-dom-bindings/src/events/plugins/ChangeEventPlugin.js

Lines 117 to 122 in 6aacd3f

	function getInstIfValueChanged(targetInst: Object) {
	const targetNode = getNodeFromInstance(targetInst);
	if (updateValueIfChanged(((targetNode: any): HTMLInputElement))) {
	return targetInst;
	}
	}

updateValueIfChanged:

react/packages/react-dom-bindings/src/client/inputValueTracking.js

Lines 141 to 147 in 6aacd3f

	const lastValue = tracker.getValue();
	const nextValue = getValueFromNode(node);
	if (nextValue !== lastValue) {
	tracker.setValue(nextValue);
	return true;
	}
	return false;

tracker:

react/packages/react-dom-bindings/src/client/inputValueTracking.js

Lines 103 to 111 in 6aacd3f

	const tracker = {
	getValue() {
	return currentValue;
	},
	setValue(value: string) {
	if (__DEV__) {
	checkFormFieldValueStringCoercion(value);
	}
	currentValue = '' + value;

We can find the key is that the tracked value should be different from the value get from node (getValueFromNode(node)) in our case. We can believe the tracked value was not updated after the delayed state change, which cause the issue.

Analysis by reproduction steps (Please visit this demo, which can print the tracked values):

1. Initially:
   • One: checked (getValueFromNode(node) is 'true'), and the tracked value is 'true'
   • Two: unchecked, and the tracked value is 'false'
2. Click 'Two', and wait for delayed setValue: onChange is called
   • One: unchecked, and the tracked value is 'false'
   • Two: checked, and the tracked value is 'true'
3. Click 'One', and wait for the delayed setValue: onChange is called
   • One: checked, and the tracked value is 'true'
   • Two: unchecked, and the tracked value is 'true'
4. Click 'Two', won't call onChange again

PS: The tracked value can be updated at the end of the event handler (by restoreControlledInputState), so Step 2 will result in the correct tracked values.

So, I think one solution is to update the tracked value (at the end of the commit mutation phase) when the checked prop changes.

I'd like to submit a PR to fix it. Any corrections or suggestions will be appreciated, (for I'm not sure if I miss anything important).

[Luk-z]

I tried another way to understand what caused the bug.

I just checked the diff between a working commit (d962f35) and non-working commit (1f248bd) and figured out that the bug happens after removing this lines of code in updateProperties() function of packages/react-dom-bindings/src/client/ReactDOMComponent.js.

just restoring

              const checkedValue = nextProps.defaultChecked;
              const inputElement: HTMLInputElement = (domElement: any);
              inputElement.checked =
                !!checkedValue &&
                typeof checkedValue !== 'function' &&
                checkedValue !== 'symbol';

or just adding domElement.checked = checked; the bug disappear.

Maybe the problem is explained in this comment

 * If `checked` or `value` are not supplied (or null/undefined), user actions
 * that affect the checked state or value will trigger updates to the element.
 *
 * If they are supplied (and not null/undefined), the rendered element will not
 * trigger updates to the element. Instead, the props must change in order for
 * the rendered element to be updated.

inputElement.checked is not updated and will result null/undefined and the radio is no more controlled?

You can check this branch. I've also made a change to packages/react-devtools-shell/src/e2e-apps/ListApp.js to start devtools server with a simple radio example.
To run the server:

• run yarn in the root directory and in packages/react-devtools-inline directory.
• from to the root directory run yarn build-for-devtools
• from packages/react-devtools-shell folder execute yarn start (better to open another shell for this command)
• go to http://localhost:8080/e2e.html

React.b.mov