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.
Sebastian Markbåge committed
Apr 9, 2023 at 22:16 UTC
9a9da7721e5b73a8af242807e463e2af842c58ee
4 files changed
+78
-14
packages/react-dom-bindings/src/client/ReactDOMInput.js
+1
-1
@@ -84,7 +84,7 @@ export function validateInputProps(element: Element, props: Object) {
84
export function updateInputChecked(element: Element, props: Object) {
85
const node: HTMLInputElement = (element: any);
86
const checked = props.checked;
87
- if (checked != null) {
87
+ if (checked != null && node.checked !== !!checked) {
88
node.checked = checked;
89
}
90
}
packages/react-dom-bindings/src/client/ReactDOMTextarea.js
+11
-8
@@ -61,12 +61,6 @@ export function validateTextareaProps(element: Element, props: Object) {
61
export function updateTextarea(element: Element, props: Object) {
62
const node: HTMLTextAreaElement = (element: any);
63
const value = getToStringValue(props.value);
64
- const defaultValue = getToStringValue(props.defaultValue);
65
- if (defaultValue != null) {
66
- node.defaultValue = toString(defaultValue);
67
- } else {
68
- node.defaultValue = '';
69
- }
64
if (value != null) {
65
// Cast `value` to a string to ensure the value is set correctly. While
66
// browsers typically do this as necessary, jsdom doesn't.
@@ -76,10 +70,19 @@ export function updateTextarea(element: Element, props: Object) {
70
node.value = newValue;
71
}
72
// TOOO: This should respect disableInputAttributeSyncing flag.
79
- if (props.defaultValue == null && node.defaultValue !== newValue) {
80
- node.defaultValue = newValue;
73
+ if (props.defaultValue == null) {
74
+ if (node.defaultValue !== newValue) {
75
+ node.defaultValue = newValue;
76
+ }
77
+ return;
78
}
79
}
80
+ const defaultValue = getToStringValue(props.defaultValue);
81
+ if (defaultValue != null) {
82
+ node.defaultValue = toString(defaultValue);
83
+ } else {
84
+ node.defaultValue = '';
85
+ }
86
}
87
88
export function initTextarea(element: Element, props: Object) {
packages/react-dom/src/__tests__/ReactDOMComponent-test.js
+65
-5
@@ -1043,6 +1043,63 @@ describe('ReactDOMComponent', () => {
1043
expect(nodeValueSetter).toHaveBeenCalledTimes(2);
1044
});
1045
1046
+ it('should not incur unnecessary DOM mutations for controlled string properties', () => {
1047
+ function onChange() {}
1048
+ const container = document.createElement('div');
1049
+ ReactDOM.render(<input value="" onChange={onChange} />, container);
1050
+
1051
+ const node = container.firstChild;
1052
+
1053
+ let nodeValue = '';
1054
+ const nodeValueSetter = jest.fn();
1055
+ Object.defineProperty(node, 'value', {
1056
+ get: function () {
1057
+ return nodeValue;
1058
+ },
1059
+ set: nodeValueSetter.mockImplementation(function (newValue) {
1060
+ nodeValue = newValue;
1061
+ }),
1062
+ });
1063
+
1064
+ ReactDOM.render(<input value="foo" onChange={onChange} />, container);
1065
+ expect(nodeValueSetter).toHaveBeenCalledTimes(1);
1066
+
1067
+ ReactDOM.render(
1068
+ <input value="foo" data-unrelated={true} onChange={onChange} />,
1069
+ container,
1070
+ );
1071
+ expect(nodeValueSetter).toHaveBeenCalledTimes(1);
1072
+
1073
+ expect(() => {
1074
+ ReactDOM.render(<input onChange={onChange} />, container);
1075
+ }).toErrorDev(
1076
+ 'A component is changing a controlled input to be uncontrolled. This is likely caused by ' +
1077
+ 'the value changing from a defined to undefined, which should not happen. Decide between ' +
1078
+ 'using a controlled or uncontrolled input element for the lifetime of the component.',
1079
+ );
1080
+ expect(nodeValueSetter).toHaveBeenCalledTimes(1);
1081
+
1082
+ expect(() => {
1083
+ ReactDOM.render(<input value={null} onChange={onChange} />, container);
1084
+ }).toErrorDev(
1085
+ 'value` prop on `input` should not be null. Consider using an empty string to clear the ' +
1086
+ 'component or `undefined` for uncontrolled components.',
1087
+ );
1088
+ expect(nodeValueSetter).toHaveBeenCalledTimes(1);
1089
+
1090
+ expect(() => {
1091
+ ReactDOM.render(<input value="" onChange={onChange} />, container);
1092
+ }).toErrorDev(
1093
+ ' A component is changing an uncontrolled input to be controlled. This is likely caused by ' +
1094
+ 'the value changing from undefined to a defined value, which should not happen. Decide between ' +
1095
+ 'using a controlled or uncontrolled input element for the lifetime of the component.',
1096
+ );
1097
+ expect(nodeValueSetter).toHaveBeenCalledTimes(2);
1098
+
1099
+ ReactDOM.render(<input onChange={onChange} />, container);
1100
+ expect(nodeValueSetter).toHaveBeenCalledTimes(2);
1101
+ });
1102
+
1103
it('should not incur unnecessary DOM mutations for boolean properties', () => {
1104
const container = document.createElement('div');
1105
function onChange() {
@@ -1066,7 +1123,12 @@ describe('ReactDOMComponent', () => {
1123
});
1124
1125
ReactDOM.render(
1069
- <input type="checkbox" onChange={onChange} checked={true} />,
1126
+ <input
1127
+ type="checkbox"
1128
+ onChange={onChange}
1129
+ checked={true}
1130
+ data-unrelated={true}
1131
+ />,
1132
container,
1133
);
1134
expect(nodeValueSetter).toHaveBeenCalledTimes(0);
@@ -1094,15 +1156,13 @@ describe('ReactDOMComponent', () => {
1156
'using a controlled or uncontrolled input element for the lifetime of the component.',
1157
);
1158
1097
- // TODO: Non-null values are updated twice on inputs. This is should ideally be fixed.
1098
- expect(nodeValueSetter).toHaveBeenCalledTimes(3);
1159
+ expect(nodeValueSetter).toHaveBeenCalledTimes(2);
1160
1161
ReactDOM.render(
1162
<input type="checkbox" onChange={onChange} checked={true} />,
1163
container,
1164
);
1104
- // TODO: Non-null values are updated twice on inputs. This is should ideally be fixed.
1105
- expect(nodeValueSetter).toHaveBeenCalledTimes(5);
1165
+ expect(nodeValueSetter).toHaveBeenCalledTimes(3);
1166
});
1167
1168
it('should ignore attribute list for elements with the "is" attribute', () => {
packages/react-dom/src/__tests__/ReactDOMTextarea-test.js
+1
@@ -603,6 +603,7 @@ describe('ReactDOMTextarea', () => {
603
ref={n => (node = n)}
604
value="foo"
605
onChange={emptyFunction}
606
+ data-count={this.state.count}
607
/>
608
</div>
609
);