From 4f4c52a3c8f9c8a2d8133c654841fee257c37249 Mon Sep 17 00:00:00 2001
From: Sophie Alpert <git@sophiebits.com>
Date: Mon, 2 Oct 2023 11:38:13 -0700
Subject: [PATCH] Fix controlled radios, maybe for real this time (#27443)

Fixes #26876 for real?

In 18.2.0 (last stable), we set .checked unconditionally:


https://github.com/facebook/react/blob/v18.2.0/packages/react-dom/src/client/ReactDOMInput.js#L129-L135

This is important because if we are updating two radios' checkedness
from (false, true) to (true, false), we need to make sure that
input2.checked is explicitly set to false, even though setting
`input1.checked = true` already unchecks input2.

I think this fix is not complete because there is no guarantee that all
the inputs rerender at the same time? Hence the TODO. But in practice
they usually would and I _think_ this is comparable to what we had
before.

Also treating function and symbol as false like we used to and like we
do on initial mount.
---
 .../src/client/ReactDOMInput.js               |  9 ++-
 .../src/__tests__/ReactDOMComponent-test.js   | 51 +-----------
 .../src/__tests__/ReactDOMInput-test.js       | 77 +++++++++++++++++++
 3 files changed, 88 insertions(+), 49 deletions(-)

diff --git a/packages/react-dom-bindings/src/client/ReactDOMInput.js b/packages/react-dom-bindings/src/client/ReactDOMInput.js
index 4411887e3cbf0..744a6e931d9c3 100644
--- a/packages/react-dom-bindings/src/client/ReactDOMInput.js
+++ b/packages/react-dom-bindings/src/client/ReactDOMInput.js
@@ -175,8 +175,13 @@ export function updateInput(
     }
   }
 
-  if (checked != null && node.checked !== !!checked) {
-    node.checked = checked;
+  if (checked != null) {
+    // Important to set this even if it's not a change in order to update input
+    // value tracking with radio buttons
+    // TODO: Should really update input value tracking for the whole radio
+    // button group in an effect or something (similar to #27024)
+    node.checked =
+      checked && typeof checked !== 'function' && typeof checked !== 'symbol';
   }
 
   if (
diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
index 9ba1ed9dcff93..2e903aba8abf8 100644
--- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
@@ -1094,18 +1094,12 @@ describe('ReactDOMComponent', () => {
 
     it('should not incur unnecessary DOM mutations for boolean properties', () => {
       const container = document.createElement('div');
-      function onChange() {
-        // noop
-      }
-      ReactDOM.render(
-        <input type="checkbox" onChange={onChange} checked={true} />,
-        container,
-      );
+      ReactDOM.render(<audio muted={true} />, container);
 
       const node = container.firstChild;
       let nodeValue = true;
       const nodeValueSetter = jest.fn();
-      Object.defineProperty(node, 'checked', {
+      Object.defineProperty(node, 'muted', {
         get: function () {
           return nodeValue;
         },
@@ -1114,48 +1108,11 @@ describe('ReactDOMComponent', () => {
         }),
       });
 
-      ReactDOM.render(
-        <input
-          type="checkbox"
-          onChange={onChange}
-          checked={true}
-          data-unrelated={true}
-        />,
-        container,
-      );
-      expect(nodeValueSetter).toHaveBeenCalledTimes(0);
-
-      expect(() => {
-        ReactDOM.render(
-          <input type="checkbox" onChange={onChange} />,
-          container,
-        );
-      }).toErrorDev(
-        'A component is changing a controlled input to be uncontrolled. This is likely caused by ' +
-          'the value changing from a defined to undefined, which should not happen. Decide between ' +
-          'using a controlled or uncontrolled input element for the lifetime of the component.',
-      );
-      // This leaves the current checked value in place, just like text inputs.
+      ReactDOM.render(<audio muted={true} data-unrelated="yes" />, container);
       expect(nodeValueSetter).toHaveBeenCalledTimes(0);
 
-      expect(() => {
-        ReactDOM.render(
-          <input type="checkbox" onChange={onChange} checked={false} />,
-          container,
-        );
-      }).toErrorDev(
-        ' A component is changing an uncontrolled input to be controlled. This is likely caused by ' +
-          'the value changing from undefined to a defined value, which should not happen. Decide between ' +
-          'using a controlled or uncontrolled input element for the lifetime of the component.',
-      );
-
+      ReactDOM.render(<audio muted={false} data-unrelated="ok" />, container);
       expect(nodeValueSetter).toHaveBeenCalledTimes(1);
-
-      ReactDOM.render(
-        <input type="checkbox" onChange={onChange} checked={true} />,
-        container,
-      );
-      expect(nodeValueSetter).toHaveBeenCalledTimes(2);
     });
 
     it('should ignore attribute list for elements with the "is" attribute', () => {
diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js
index 3e984b9efc18c..e52174a3bf749 100644
--- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js
@@ -1414,6 +1414,83 @@ describe('ReactDOMInput', () => {
     assertInputTrackingIsCurrent(container);
   });
 
+  it('should control radio buttons if the tree updates during render (case 2; #26876)', () => {
+    let thunk = null;
+    function App() {
+      const [disabled, setDisabled] = React.useState(false);
+      const [value, setValue] = React.useState('one');
+      function handleChange(e) {
+        setDisabled(true);
+        // Pretend this is in a setTimeout or something
+        thunk = () => {
+          setDisabled(false);
+          setValue(e.target.value);
+        };
+      }
+      return (
+        <>
+          <input
+            type="radio"
+            name="fruit"
+            value="one"
+            checked={value === 'one'}
+            onChange={handleChange}
+            disabled={disabled}
+          />
+          <input
+            type="radio"
+            name="fruit"
+            value="two"
+            checked={value === 'two'}
+            onChange={handleChange}
+            disabled={disabled}
+          />
+        </>
+      );
+    }
+    ReactDOM.render(<App />, container);
+    const [one, two] = container.querySelectorAll('input');
+    expect(one.checked).toBe(true);
+    expect(two.checked).toBe(false);
+    expect(isCheckedDirty(one)).toBe(true);
+    expect(isCheckedDirty(two)).toBe(true);
+    assertInputTrackingIsCurrent(container);
+
+    // Click two
+    setUntrackedChecked.call(two, true);
+    dispatchEventOnNode(two, 'click');
+    expect(one.checked).toBe(true);
+    expect(two.checked).toBe(false);
+    expect(isCheckedDirty(one)).toBe(true);
+    expect(isCheckedDirty(two)).toBe(true);
+    assertInputTrackingIsCurrent(container);
+
+    // After a delay...
+    ReactDOM.unstable_batchedUpdates(thunk);
+    expect(one.checked).toBe(false);
+    expect(two.checked).toBe(true);
+    expect(isCheckedDirty(one)).toBe(true);
+    expect(isCheckedDirty(two)).toBe(true);
+    assertInputTrackingIsCurrent(container);
+
+    // Click back to one
+    setUntrackedChecked.call(one, true);
+    dispatchEventOnNode(one, 'click');
+    expect(one.checked).toBe(false);
+    expect(two.checked).toBe(true);
+    expect(isCheckedDirty(one)).toBe(true);
+    expect(isCheckedDirty(two)).toBe(true);
+    assertInputTrackingIsCurrent(container);
+
+    // After a delay...
+    ReactDOM.unstable_batchedUpdates(thunk);
+    expect(one.checked).toBe(true);
+    expect(two.checked).toBe(false);
+    expect(isCheckedDirty(one)).toBe(true);
+    expect(isCheckedDirty(two)).toBe(true);
+    assertInputTrackingIsCurrent(container);
+  });
+
   it('should warn with value and no onChange handler and readOnly specified', () => {
     ReactDOM.render(
       <input type="text" value="zoink" readOnly={true} />,