Don't set checked if it's already the correct value

This is in line with value. We had a test for it but it wasn't passing if any other related prop is updated.
facebook · Apr 10, 2023 · c6e4925 · c6e4925
1 parent 00572cc
commit c6e4925
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 6 deletions.
diff --git a/packages/react-dom-bindings/src/client/ReactDOMInput.js b/packages/react-dom-bindings/src/client/ReactDOMInput.js
@@ -84,7 +84,7 @@ export function validateInputProps(element: Element, props: Object) {
 export function updateInputChecked(element: Element, props: Object) {
   const node: HTMLInputElement = (element: any);
   const checked = props.checked;
-  if (checked != null) {
+  if (checked != null && node.checked !== !!checked) {
     node.checked = checked;
   }
 }

diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
@@ -1043,6 +1043,63 @@ describe('ReactDOMComponent', () => {
       expect(nodeValueSetter).toHaveBeenCalledTimes(2);
     });
 
+    it('should not incur unnecessary DOM mutations for controlled string properties', () => {
+      function onChange() {}
+      const container = document.createElement('div');
+      ReactDOM.render(<input value="" onChange={onChange} />, container);
+
+      const node = container.firstChild;
+
+      let nodeValue = '';
+      const nodeValueSetter = jest.fn();
+      Object.defineProperty(node, 'value', {
+        get: function () {
+          return nodeValue;
+        },
+        set: nodeValueSetter.mockImplementation(function (newValue) {
+          nodeValue = newValue;
+        }),
+      });
+
+      ReactDOM.render(<input value="foo" onChange={onChange} />, container);
+      expect(nodeValueSetter).toHaveBeenCalledTimes(1);
+
+      ReactDOM.render(
+        <input value="foo" data-unrelated={true} onChange={onChange} />,
+        container,
+      );
+      expect(nodeValueSetter).toHaveBeenCalledTimes(1);
+
+      expect(() => {
+        ReactDOM.render(<input 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.',
+      );
+      expect(nodeValueSetter).toHaveBeenCalledTimes(1);
+
+      expect(() => {
+        ReactDOM.render(<input value={null} onChange={onChange} />, container);
+      }).toErrorDev(
+        'value` prop on `input` should not be null. Consider using an empty string to clear the ' +
+          'component or `undefined` for uncontrolled components.',
+      );
+      expect(nodeValueSetter).toHaveBeenCalledTimes(1);
+
+      expect(() => {
+        ReactDOM.render(<input value="" onChange={onChange} />, 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.',
+      );
+      expect(nodeValueSetter).toHaveBeenCalledTimes(2);
+
+      ReactDOM.render(<input onChange={onChange} />, container);
+      expect(nodeValueSetter).toHaveBeenCalledTimes(2);
+    });
+
     it('should not incur unnecessary DOM mutations for boolean properties', () => {
       const container = document.createElement('div');
       function onChange() {
@@ -1066,7 +1123,12 @@ describe('ReactDOMComponent', () => {
       });
 
       ReactDOM.render(
-        <input type="checkbox" onChange={onChange} checked={true} />,
+        <input
+          type="checkbox"
+          onChange={onChange}
+          checked={true}
+          data-unrelated={true}
+        />,
         container,
       );
       expect(nodeValueSetter).toHaveBeenCalledTimes(0);
@@ -1094,15 +1156,13 @@ describe('ReactDOMComponent', () => {
           'using a controlled or uncontrolled input element for the lifetime of the component.',
       );
 
-      // TODO: Non-null values are updated twice on inputs. This is should ideally be fixed.
-      expect(nodeValueSetter).toHaveBeenCalledTimes(3);
+      expect(nodeValueSetter).toHaveBeenCalledTimes(2);
 
       ReactDOM.render(
         <input type="checkbox" onChange={onChange} checked={true} />,
         container,
       );
-      // TODO: Non-null values are updated twice on inputs. This is should ideally be fixed.
-      expect(nodeValueSetter).toHaveBeenCalledTimes(5);
+      expect(nodeValueSetter).toHaveBeenCalledTimes(3);
     });
 
     it('should ignore attribute list for elements with the "is" attribute', () => {