Don't update textarea defaultValue and input checked unnecessarily (#…

…26580) In #26573 I changed it so that textareas get their defaultValue reset if you don't specify one. However, the way that was implemented, it always set it for any update even if it hasn't changed. We have a test for that, but that test only works if no properties update at all so that no update was scheduled. This fixes the test so that it updates some unrelated prop. I also found a test for `checked` that needed a similar fix. Interestingly, we don't do this deduping for `defaultValue` or `defaultChecked` on inputs and there's no test for that.
facebook · Apr 10, 2023 · 9a9da77 · 9a9da77
1 parent e5146cb
commit 9a9da77
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 14 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-bindings/src/client/ReactDOMTextarea.js b/packages/react-dom-bindings/src/client/ReactDOMTextarea.js
@@ -61,12 +61,6 @@ export function validateTextareaProps(element: Element, props: Object) {
 export function updateTextarea(element: Element, props: Object) {
   const node: HTMLTextAreaElement = (element: any);
   const value = getToStringValue(props.value);
-  const defaultValue = getToStringValue(props.defaultValue);
-  if (defaultValue != null) {
-    node.defaultValue = toString(defaultValue);
-  } else {
-    node.defaultValue = '';
-  }
   if (value != null) {
     // Cast `value` to a string to ensure the value is set correctly. While
     // browsers typically do this as necessary, jsdom doesn't.
@@ -76,10 +70,19 @@ export function updateTextarea(element: Element, props: Object) {
       node.value = newValue;
     }
     // TOOO: This should respect disableInputAttributeSyncing flag.
-    if (props.defaultValue == null && node.defaultValue !== newValue) {
-      node.defaultValue = newValue;
+    if (props.defaultValue == null) {
+      if (node.defaultValue !== newValue) {
+        node.defaultValue = newValue;
+      }
+      return;
     }
   }
+  const defaultValue = getToStringValue(props.defaultValue);
+  if (defaultValue != null) {
+    node.defaultValue = toString(defaultValue);
+  } else {
+    node.defaultValue = '';
+  }
 }
 
 export function initTextarea(element: Element, props: Object) {

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', () => {

diff --git a/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js b/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js
@@ -603,6 +603,7 @@ describe('ReactDOMTextarea', () => {
               ref={n => (node = n)}
               value="foo"
               onChange={emptyFunction}
+              data-count={this.state.count}
             />
           </div>
         );