From 9a9da7721e5b73a8af242807e463e2af842c58ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Sun, 9 Apr 2023 22:16:38 -0400 Subject: [PATCH] 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. --- .../src/client/ReactDOMInput.js | 2 +- .../src/client/ReactDOMTextarea.js | 19 ++--- .../src/__tests__/ReactDOMComponent-test.js | 70 +++++++++++++++++-- .../src/__tests__/ReactDOMTextarea-test.js | 1 + 4 files changed, 78 insertions(+), 14 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMInput.js b/packages/react-dom-bindings/src/client/ReactDOMInput.js index fcede97885728..0708b984f7c95 100644 --- 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 index 544cee9952be9..c9862c9b00bef 100644 --- 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 index b8c91fb86e18b..c2dd984823ed1 100644 --- 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(, 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(, container); + expect(nodeValueSetter).toHaveBeenCalledTimes(1); + + ReactDOM.render( + , + container, + ); + expect(nodeValueSetter).toHaveBeenCalledTimes(1); + + expect(() => { + ReactDOM.render(, 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(, 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(, 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(, 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( - , + , 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( , 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 index 396eef8d6d34e..347942234063d 100644 --- 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} /> );