Repository: facebook/react
Pull request: Switching checked to null should leave the current value

I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state.

When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`:

https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69

Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null.

This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled.

Related to that, there was also another issue observed in https://github.com/facebook/react/pull/26596#discussion_r1162295872 and https://github.com/facebook/react/pull/26588

We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes.

Changed files:
filename: packages/react-dom-bindings/src/client/ReactDOMComponent.js
patch: @@ -834,6 +834,7 @@ export function setInitialProperties(
       // listeners still fire for the invalid event.
       listenToNonDelegatedEvent('invalid', domElement);
 
+      let name = null;
       let type = null;
       let value = null;
       let defaultValue = null;
@@ -848,31 +849,16 @@ export function setInitialProperties(
           continue;
         }
         switch (propKey) {
+          case 'name': {
+            name = propValue;
+            break;
+          }
           case 'type': {
-            // Fast path since 'type' is very common on inputs
-            if (
-              propValue != null &&
-              typeof propValue !== 'function' &&
-              typeof propValue !== 'symbol' &&
-              typeof propValue !== 'boolean'
-            ) {
-              type = propValue;
-              if (__DEV__) {
-                checkAttributeStringCoercion(propValue, propKey);
-              }
-              domElement.setAttribute(propKey, propValue);
-            }
+            type = propValue;
             break;
           }
           case 'checked': {
             checked = propValue;
-            const checkedValue =
-              propValue != null ? propValue : props.defaultChecked;
-            const inputElement: HTMLInputElement = (domElement: any);
-            inputElement.checked =
-              !!checkedValue &&
-              typeof checkedValue !== 'function' &&
-              checkedValue !== 'symbol';
             break;
           }
           case 'defaultChecked': {
@@ -904,7 +890,6 @@ export function setInitialProperties(
       }
       // TODO: Make sure we check if this is still unmounted or do any clean
       // up necessary since we never stop tracking anymore.
-      track((domElement: any));
       validateInputProps(domElement, props);
       initInput(
         domElement,
@@ -913,8 +898,10 @@ export function setInitialProperties(
         checked,
         defaultChecked,
         type,
+        name,
         false,
       );
+      track((domElement: any));
       return;
     }
     case 'select': {
@@ -1010,9 +997,9 @@ export function setInitialProperties(
       }
       // TODO: Make sure we check if this is still unmounted or do any clean
       // up necessary since we never stop tracking anymore.
-      track((domElement: any));
       validateTextareaProps(domElement, props);
       initTextarea(domElement, value, defaultValue, children);
+      track((domElement: any));
       return;
     }
     case 'option': {
@@ -1305,14 +1292,6 @@ export function updateProperties(
         if (lastProps.hasOwnProperty(propKey) && lastProp != null) {
           switch (propKey) {
             case 'checked': {
-              if (!nextProps.hasOwnProperty(propKey)) {
-                const checkedValue = nextProps.defaultChecked;
-                const inputElement: HTMLInputElement = (domElement: any);
-                inputElement.checked =
-                  !!checkedValue &&
-                  typeof checkedValue !== 'function' &&
-                  checkedValue !== 'symbol';
-              }
               break;
             }
             case 'value': {
@@ -1341,22 +1320,6 @@ export function updateProperties(
           switch (propKey) {
             case 'type': {
               type = nextProp;
-              // Fast path since 'type' is very common on inputs
-              if (nextProp !== lastProp) {
-                if (
-                  nextProp != null &&
-                  typeof nextProp !== 'function' &&
-                  typeof nextProp !== 'symbol' &&
-                  typeof nextProp !== 'boolean'
-                ) {
-                  if (__DEV__) {
-                    checkAttributeStringCoercion(nextProp, propKey);
-                  }
-                  domElement.setAttribute(propKey, nextProp);
-                } else {
-                  domElement.removeAttribute(propKey);
-                }
-              }
               break;
             }
             case 'name': {
@@ -1365,15 +1328,6 @@ export function updateProperties(
             }
             case 'checked': {
               checked = nextProp;
-              if (nextProp !== lastProp) {
-                const checkedValue =
-                  nextProp != null ? nextProp : nextProps.defaultChecked;
-                const inputElement: HTMLInputElement = (domElement: any);
-                inputElement.checked =
-                  !!checkedValue &&
-                  typeof checkedValue !== 'function' &&
-                  checkedValue !== 'symbol';
-              }
               break;
             }
             case 'defaultChecked': {
@@ -1453,23 +1407,6 @@ export function updateProperties(
         }
       }
 
-      // Update checked *before* name.
-      // In the middle of an update, it is possible to have multiple checked.
-      // When a checked radio tries to change name, browser makes another radio's checked false.
-      if (
-        name != null &&
-        typeof name !== 'function' &&
-        typeof name !== 'symbol' &&
-        typeof name !== 'boolean'
-      ) {
-        if (__DEV__) {
-          checkAttributeStringCoercion(name, 'name');
-        }
-        domElement.setAttribute('name', name);
-      } else {
-        domElement.removeAttribute('name');
-      }
-
       // Update the wrapper around inputs *after* updating props. This has to
       // happen after updating the rest of props. Otherwise HTML5 input validations
       // raise warnings and prevent the new value from being assigned.
@@ -1481,6 +1418,7 @@ export function updateProperties(
         checked,
         defaultChecked,
         type,
+        name,
       );
       return;
     }
@@ -1822,33 +1760,12 @@ export function updatePropertiesWithDiff(
         const propValue = updatePayload[i + 1];
         switch (propKey) {
           case 'type': {
-            // Fast path since 'type' is very common on inputs
-            if (
-              propValue != null &&
-              typeof propValue !== 'function' &&
-              typeof propValue !== 'symbol' &&
-              typeof propValue !== 'boolean'
-            ) {
-              if (__DEV__) {
-                checkAttributeStringCoercion(propValue, propKey);
-              }
-              domElement.setAttribute(propKey, propValue);
-            } else {
-              domElement.removeAttribute(propKey);
-            }
             break;
           }
           case 'name': {
             break;
           }
           case 'checked': {
-            const checkedValue =
-              propValue != null ? propValue : nextProps.defaultChecked;
-            const inputElement: HTMLInputElement = (domElement: any);
-            inputElement.checked =
-              !!checkedValue &&
-              typeof checkedValue !== 'function' &&
-              checkedValue !== 'symbol';
             break;
           }
           case 'defaultChecked': {
@@ -1916,23 +1833,6 @@ export function updatePropertiesWithDiff(
         }
       }
 
-      // Update checked *before* name.
-      // In the middle of an update, it is possible to have multiple checked.
-      // When a checked radio tries to change name, browser makes another radio's checked false.
-      if (
-        name != null &&
-        typeof name !== 'function' &&
-        typeof name !== 'symbol' &&
-        typeof name !== 'boolean'
-      ) {
-        if (__DEV__) {
-          checkAttributeStringCoercion(name, 'name');
-        }
-        domElement.setAttribute('name', name);
-      } else {
-        domElement.removeAttribute('name');
-      }
-
       // Update the wrapper around inputs *after* updating props. This has to
       // happen after updating the rest of props. Otherwise HTML5 input validations
       // raise warnings and prevent the new value from being assigned.
@@ -1944,6 +1844,7 @@ export function updatePropertiesWithDiff(
         checked,
         defaultChecked,
         type,
+        name,
       );
       return;
     }
@@ -2970,7 +2871,6 @@ export function diffHydratedProperties(
       listenToNonDelegatedEvent('invalid', domElement);
       // TODO: Make sure we check if this is still unmounted or do any clean
       // up necessary since we never stop tracking anymore.
-      track((domElement: any));
       validateInputProps(domElement, props);
       // For input and textarea we current always set the value property at
       // post mount to force it to diverge from attributes. However, for
@@ -2984,8 +2884,10 @@ export function diffHydratedProperties(
         props.checked,
         props.defaultChecked,
         props.type,
+        props.name,
         true,
       );
+      track((domElement: any));
       break;
     case 'option':
       validateOptionProps(domElement, props);
@@ -3008,9 +2910,9 @@ export function diffHydratedProperties(
       listenToNonDelegatedEvent('invalid', domElement);
       // TODO: Make sure we check if this is still unmounted or do any clean
       // up necessary since we never stop tracking anymore.
-      track((domElement: any));
       validateTextareaProps(domElement, props);
       initTextarea(domElement, props.value, props.defaultValue, props.children);
+      track((domElement: any));
       break;
   }
 

filename: packages/react-dom-bindings/src/client/ReactDOMInput.js
patch: @@ -89,9 +89,30 @@ export function updateInput(
   checked: ?boolean,
   defaultChecked: ?boolean,
   type: ?string,
+  name: ?string,
 ) {
   const node: HTMLInputElement = (element: any);
 
+  // Temporarily disconnect the input from any radio buttons.
+  // Changing the type or name as the same time as changing the checked value
+  // needs to be atomically applied. We can only ensure that by disconnecting
+  // the name while do the mutations and then reapply the name after that's done.
+  node.name = '';
+
+  if (
+    type != null &&
+    typeof type !== 'function' &&
+    typeof type !== 'symbol' &&
+    typeof type !== 'boolean'
+  ) {
+    if (__DEV__) {
+      checkAttributeStringCoercion(type, 'type');
+    }
+    node.type = type;
+  } else {
+    node.removeAttribute('type');
+  }
+
   if (value != null) {
     if (type === 'number') {
       if (
@@ -157,6 +178,20 @@ export function updateInput(
   if (checked != null && node.checked !== !!checked) {
     node.checked = checked;
   }
+
+  if (
+    name != null &&
+    typeof name !== 'function' &&
+    typeof name !== 'symbol' &&
+    typeof name !== 'boolean'
+  ) {
+    if (__DEV__) {
+      checkAttributeStringCoercion(name, 'name');
+    }
+    node.name = name;
+  } else {
+    node.removeAttribute('name');
+  }
 }
 
 export function initInput(
@@ -166,10 +201,23 @@ export function initInput(
   checked: ?boolean,
   defaultChecked: ?boolean,
   type: ?string,
+  name: ?string,
   isHydrating: boolean,
 ) {
   const node: HTMLInputElement = (element: any);
 
+  if (
+    type != null &&
+    typeof type !== 'function' &&
+    typeof type !== 'symbol' &&
+    typeof type !== 'boolean'
+  ) {
+    if (__DEV__) {
+      checkAttributeStringCoercion(type, 'type');
+    }
+    node.type = type;
+  }
+
   if (value != null || defaultValue != null) {
     const isButton = type === 'submit' || type === 'reset';
 
@@ -235,10 +283,6 @@ export function initInput(
   // will sometimes influence the value of checked (even after detachment).
   // Reference: https://bugs.chromium.org/p/chromium/issues/detail?id=608416
   // We need to temporarily unset name to avoid disrupting radio button groups.
-  const name = node.name;
-  if (name !== '') {
-    node.name = '';
-  }
 
   const checkedOrDefault = checked != null ? checked : defaultChecked;
   // TODO: This 'function' or 'symbol' check isn't replicated in other places
@@ -276,7 +320,16 @@ export function initInput(
     node.defaultChecked = !!initialChecked;
   }
 
-  if (name !== '') {
+  // Name needs to be set at the end so that it applies atomically to connected radio buttons.
+  if (
+    name != null &&
+    typeof name !== 'function' &&
+    typeof name !== 'symbol' &&
+    typeof name !== 'boolean'
+  ) {
+    if (__DEV__) {
+      checkAttributeStringCoercion(name, 'name');
+    }
     node.name = name;
   }
 }
@@ -291,6 +344,7 @@ export function restoreControlledInputState(element: Element, props: Object) {
     props.checked,
     props.defaultChecked,
     props.type,
+    props.name,
   );
   const name = props.name;
   if (props.type === 'radio' && name != null) {
@@ -347,6 +401,7 @@ export function restoreControlledInputState(element: Element, props: Object) {
         otherProps.checked,
         otherProps.defaultChecked,
         otherProps.type,
+        otherProps.name,
       );
     }
   }

filename: packages/react-dom/src/__tests__/ReactDOMComponent-test.js
patch: @@ -1143,7 +1143,8 @@ describe('ReactDOMComponent', () => {
           '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.',
       );
-      expect(nodeValueSetter).toHaveBeenCalledTimes(1);
+      // This leaves the current checked value in place, just like text inputs.
+      expect(nodeValueSetter).toHaveBeenCalledTimes(0);
 
       expect(() => {
         ReactDOM.render(
@@ -1156,13 +1157,13 @@ describe('ReactDOMComponent', () => {
           'using a controlled or uncontrolled input element for the lifetime of the component.',
       );
 
-      expect(nodeValueSetter).toHaveBeenCalledTimes(2);
+      expect(nodeValueSetter).toHaveBeenCalledTimes(1);
 
       ReactDOM.render(
         <input type="checkbox" onChange={onChange} checked={true} />,
         container,
       );
-      expect(nodeValueSetter).toHaveBeenCalledTimes(3);
+      expect(nodeValueSetter).toHaveBeenCalledTimes(2);
     });
 
     it('should ignore attribute list for elements with the "is" attribute', () => {

filename: packages/react-dom/src/__tests__/ReactDOMInput-test.js
patch: @@ -1191,7 +1191,7 @@ describe('ReactDOMInput', () => {
         updated: false,
       };
       onClick = () => {
-        this.setState({updated: true});
+        this.setState({updated: !this.state.updated});
       };
       render() {
         const {updated} = this.state;
@@ -1222,6 +1222,62 @@ describe('ReactDOMInput', () => {
     expect(firstRadioNode.checked).toBe(false);
     dispatchEventOnNode(buttonNode, 'click');
     expect(firstRadioNode.checked).toBe(true);
+    dispatchEventOnNode(buttonNode, 'click');
+    expect(firstRadioNode.checked).toBe(false);
+  });
+
+  it("shouldn't get tricked by changing radio names, part 2", () => {
+    ReactDOM.render(
+      <div>
+        <input
+          type="radio"
+          name="a"
+          value="1"
+          checked={true}
+          onChange={() => {}}
+        />
+        <input
+          type="radio"
+          name="a"
+          value="2"
+          checked={false}
+          onChange={() => {}}
+        />
+      </div>,
+      container,
+    );
+    expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
+      true,
+    );
+    expect(container.querySelector('input[name="a"][value="2"]').checked).toBe(
+      false,
+    );
+
+    ReactDOM.render(
+      <div>
+        <input
+          type="radio"
+          name="a"
+          value="1"
+          checked={true}
+          onChange={() => {}}
+        />
+        <input
+          type="radio"
+          name="b"
+          value="2"
+          checked={true}
+          onChange={() => {}}
+        />
+      </div>,
+      container,
+    );
+    expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
+      true,
+    );
+    expect(container.querySelector('input[name="b"][value="2"]').checked).toBe(
+      true,
+    );
   });
 
   it('should control radio buttons if the tree updates during render', () => {
@@ -1720,8 +1776,18 @@ describe('ReactDOMInput', () => {
     ) {
       const el = originalCreateElement.apply(this, arguments);
       let value = '';
+      let typeProp = '';
 
       if (type === 'input') {
+        Object.defineProperty(el, 'type', {
+          get: function () {
+            return typeProp;
+          },
+          set: function (val) {
+            typeProp = String(val);
+            log.push('set property type');
+          },
+        });
         Object.defineProperty(el, 'value', {
           get: function () {
             return value;
@@ -1751,10 +1817,10 @@ describe('ReactDOMInput', () => {
     );
 
     expect(log).toEqual([
-      'set attribute type',
       'set attribute min',
       'set attribute max',
       'set attribute step',
+      'set property type',
       'set property value',
     ]);
   });
@@ -1810,6 +1876,14 @@ describe('ReactDOMInput', () => {
         HTMLInputElement.prototype,
         'value',
       ).set;
+      const getType = Object.getOwnPropertyDescriptor(
+        HTMLInputElement.prototype,
+        'type',
+      ).get;
+      const setType = Object.getOwnPropertyDescriptor(
+        HTMLInputElement.prototype,
+        'type',
+      ).set;
       if (type === 'input') {
         Object.defineProperty(el, 'defaultValue', {
           get: function () {
@@ -1829,6 +1903,15 @@ describe('ReactDOMInput', () => {
             setValue.call(this, val);
           },
         });
+        Object.defineProperty(el, 'type', {
+          get: function () {
+            return getType.call(this);
+          },
+          set: function (val) {
+            log.push(`node.type = ${strify(val)}`);
+            setType.call(this, val);
+          },
+        });
         spyOnDevAndProd(el, 'setAttribute').mockImplementation(function (
           name,
           val,
@@ -1843,14 +1926,14 @@ describe('ReactDOMInput', () => {
 
     if (disableInputAttributeSyncing) {
       expect(log).toEqual([
-        'node.setAttribute("type", "date")',
+        'node.type = "date"',
         'node.defaultValue = "1980-01-01"',
         // TODO: it's possible this reintroduces the bug because we don't assign `value` at all.
         // Need to check this on mobile Safari and Chrome.
       ]);
     } else {
       expect(log).toEqual([
-        'node.setAttribute("type", "date")',
+        'node.type = "date"',
         // value must be assigned before defaultValue. This fixes an issue where the
         // visually displayed value of date inputs disappears on mobile Safari and Chrome:
         // https://github.com/facebook/react/issues/7233

filename: packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js
patch: @@ -12,7 +12,6 @@
 let React;
 let ReactDOM;
 let ReactDOMClient;
-let ReactFeatureFlags;
 let Scheduler;
 let act;
 let waitForAll;
@@ -39,7 +38,6 @@ describe('ChangeEventPlugin', () => {
 
   beforeEach(() => {
     jest.resetModules();
-    ReactFeatureFlags = require('shared/ReactFeatureFlags');
     // TODO pull this into helper method, reduce repetition.
     // mock the browser APIs which are used in schedule:
     // - calling 'window.postMessage' should actually fire postmessage handlers
@@ -100,13 +98,8 @@ describe('ChangeEventPlugin', () => {
     node.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));
     node.dispatchEvent(new Event('change', {bubbles: true, cancelable: true}));
 
-    if (ReactFeatureFlags.disableInputAttributeSyncing) {
-      // TODO: figure out why. This might be a bug.
-      expect(called).toBe(1);
-    } else {
-      // There should be no React change events because the value stayed the same.
-      expect(called).toBe(0);
-    }
+    // There should be no React change events because the value stayed the same.
+    expect(called).toBe(0);
   });
 
   it('should consider initial text value to be current (capture)', () => {
@@ -124,13 +117,8 @@ describe('ChangeEventPlugin', () => {
     node.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));
     node.dispatchEvent(new Event('change', {bubbles: true, cancelable: true}));
 
-    if (ReactFeatureFlags.disableInputAttributeSyncing) {
-      // TODO: figure out why. This might be a bug.
-      expect(called).toBe(1);
-    } else {
-      // There should be no React change events because the value stayed the same.
-      expect(called).toBe(0);
-    }
+    // There should be no React change events because the value stayed the same.
+    expect(called).toBe(0);
   });
 
   it('should not invoke a change event for textarea same value', () => {