Update tracked value after resetting radio group (#27394)

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?
* #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

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

@@ -388,10 +388,6 @@ export function restoreControlledInputState(element: Element, props: Object) {
         );
       }
 
-      // We need update the tracked value on the named cousin since the value
-      // was changed but the input saw no event or value set
-      updateValueIfChanged(otherNode);
-
       // If this is a controlled radio button group, forcing the input that
       // was previously checked to update will cause it to be come re-checked
       // as appropriate.
@@ -406,6 +402,16 @@ export function restoreControlledInputState(element: Element, props: Object) {
         otherProps.name,
       );
     }
+
+    // If any updateInput() call set .checked to true, an input in this group
+    // (often, `rootNode` itself) may have become unchecked
+    for (let i = 0; i < group.length; i++) {
+      const otherNode = ((group[i]: any): HTMLInputElement);
+      if (otherNode.form !== rootNode.form) {
+        continue;
+      }
+      updateValueIfChanged(otherNode);
+    }
   }
 }
 

packages/react-dom/src/__tests__/ReactDOMInput-test.js

@@ -36,6 +36,43 @@ describe('ReactDOMInput', () => {
     return copy.value === node.value;
   }
 
+  function isCheckedDirty(node) {
+    // Return the "dirty checked flag" as defined in the HTML spec.
+    if (node.checked !== node.defaultChecked) {
+      return true;
+    }
+    const copy = node.cloneNode();
+    copy.type = 'checkbox';
+    copy.defaultChecked = !copy.defaultChecked;
+    return copy.checked === node.checked;
+  }
+
+  function getTrackedAndCurrentInputValue(elem: HTMLElement): [mixed, mixed] {
+    const tracker = elem._valueTracker;
+    if (!tracker) {
+      throw new Error('No input tracker');
+    }
+    return [
+      tracker.getValue(),
+      elem.nodeName === 'INPUT' &&
+      (elem.type === 'checkbox' || elem.type === 'radio')
+        ? String(elem.checked)
+        : elem.value,
+    ];
+  }
+
+  function assertInputTrackingIsCurrent(parent) {
+    parent.querySelectorAll('input, textarea, select').forEach(input => {
+      const [trackedValue, currentValue] =
+        getTrackedAndCurrentInputValue(input);
+      if (trackedValue !== currentValue) {
+        throw new Error(
+          `Input ${input.outerHTML} is currently ${currentValue} but tracker thinks it's ${trackedValue}`,
+        );
+      }
+    });
+  }
+
   beforeEach(() => {
     jest.resetModules();
 
@@ -1119,13 +1156,15 @@ describe('ReactDOMInput', () => {
               name="fruit"
               checked={true}
               onChange={emptyFunction}
+              data-which="a"
             />
             A
             <input
               ref={this.bRef}
               type="radio"
               name="fruit"
               onChange={emptyFunction}
+              data-which="b"
             />
             B
             <form>
@@ -1135,6 +1174,7 @@ describe('ReactDOMInput', () => {
                 name="fruit"
                 defaultChecked={true}
                 onChange={emptyFunction}
+                data-which="c"
               />
             </form>
           </div>
@@ -1162,6 +1202,11 @@ describe('ReactDOMInput', () => {
       expect(cNode.hasAttribute('checked')).toBe(true);
     }
 
+    expect(isCheckedDirty(aNode)).toBe(true);
+    expect(isCheckedDirty(bNode)).toBe(true);
+    expect(isCheckedDirty(cNode)).toBe(true);
+    assertInputTrackingIsCurrent(container);
+
     setUntrackedChecked.call(bNode, true);
     expect(aNode.checked).toBe(false);
     expect(cNode.checked).toBe(true);
@@ -1183,6 +1228,11 @@ describe('ReactDOMInput', () => {
     // The original state should have been restored
     expect(aNode.checked).toBe(true);
     expect(cNode.checked).toBe(true);
+
+    expect(isCheckedDirty(aNode)).toBe(true);
+    expect(isCheckedDirty(bNode)).toBe(true);
+    expect(isCheckedDirty(cNode)).toBe(true);
+    assertInputTrackingIsCurrent(container);
   });
 
   it('should check the correct radio when the selected name moves', () => {
@@ -1219,11 +1269,15 @@ describe('ReactDOMInput', () => {
     const stub = ReactDOM.render(<App />, container);
     const buttonNode = ReactDOM.findDOMNode(stub).childNodes[0];
     const firstRadioNode = ReactDOM.findDOMNode(stub).childNodes[1];
+    expect(isCheckedDirty(firstRadioNode)).toBe(true);
     expect(firstRadioNode.checked).toBe(false);
+    assertInputTrackingIsCurrent(container);
     dispatchEventOnNode(buttonNode, 'click');
     expect(firstRadioNode.checked).toBe(true);
+    assertInputTrackingIsCurrent(container);
     dispatchEventOnNode(buttonNode, 'click');
     expect(firstRadioNode.checked).toBe(false);
+    assertInputTrackingIsCurrent(container);
   });
 
   it("shouldn't get tricked by changing radio names, part 2", () => {
@@ -1246,12 +1300,13 @@ describe('ReactDOMInput', () => {
       </div>,
       container,
     );
-    expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
-      true,
-    );
-    expect(container.querySelector('input[name="a"][value="2"]').checked).toBe(
-      false,
-    );
+    const one = container.querySelector('input[name="a"][value="1"]');
+    const two = container.querySelector('input[name="a"][value="2"]');
+    expect(one.checked).toBe(true);
+    expect(two.checked).toBe(false);
+    expect(isCheckedDirty(one)).toBe(true);
+    expect(isCheckedDirty(two)).toBe(true);
+    assertInputTrackingIsCurrent(container);
 
     ReactDOM.render(
       <div>
@@ -1272,12 +1327,11 @@ describe('ReactDOMInput', () => {
       </div>,
       container,
     );
-    expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
-      true,
-    );
-    expect(container.querySelector('input[name="b"][value="2"]').checked).toBe(
-      true,
-    );
+    expect(one.checked).toBe(true);
+    expect(two.checked).toBe(true);
+    expect(isCheckedDirty(one)).toBe(true);
+    expect(isCheckedDirty(two)).toBe(true);
+    assertInputTrackingIsCurrent(container);
   });
 
   it('should control radio buttons if the tree updates during render', () => {
@@ -1339,6 +1393,9 @@ describe('ReactDOMInput', () => {
 
     expect(aNode.checked).toBe(false);
     expect(bNode.checked).toBe(true);
+    expect(isCheckedDirty(aNode)).toBe(true);
+    expect(isCheckedDirty(bNode)).toBe(true);
+    assertInputTrackingIsCurrent(container);
 
     setUntrackedChecked.call(aNode, true);
     // This next line isn't necessary in a proper browser environment, but
@@ -1352,6 +1409,9 @@ describe('ReactDOMInput', () => {
     // The original state should have been restored
     expect(aNode.checked).toBe(false);
     expect(bNode.checked).toBe(true);
+    expect(isCheckedDirty(aNode)).toBe(true);
+    expect(isCheckedDirty(bNode)).toBe(true);
+    assertInputTrackingIsCurrent(container);
   });
 
   it('should warn with value and no onChange handler and readOnly specified', () => {
@@ -1734,6 +1794,8 @@ describe('ReactDOMInput', () => {
       <input type="radio" checked={false} onChange={() => null} />,
       container,
     );
+    const input = container.querySelector('input');
+    expect(isCheckedDirty(input)).toBe(true);
     ReactDOM.render(
       <input
         type="radio"
@@ -1744,6 +1806,8 @@ describe('ReactDOMInput', () => {
       />,
       container,
     );
+    expect(isCheckedDirty(input)).toBe(true);
+    assertInputTrackingIsCurrent(container);
   });
 
   it('should warn if radio checked false changes to become uncontrolled', () => {