Skip to content

Commit 9a9da77

Browse files
authored
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.
1 parent e5146cb commit 9a9da77

File tree

4 files changed

+78
-14
lines changed

4 files changed

+78
-14
lines changed

packages/react-dom-bindings/src/client/ReactDOMInput.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ export function validateInputProps(element: Element, props: Object) {
8484
export function updateInputChecked(element: Element, props: Object) {
8585
const node: HTMLInputElement = (element: any);
8686
const checked = props.checked;
87-
if (checked != null) {
87+
if (checked != null && node.checked !== !!checked) {
8888
node.checked = checked;
8989
}
9090
}

packages/react-dom-bindings/src/client/ReactDOMTextarea.js

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,6 @@ export function validateTextareaProps(element: Element, props: Object) {
6161
export function updateTextarea(element: Element, props: Object) {
6262
const node: HTMLTextAreaElement = (element: any);
6363
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-
}
7064
if (value != null) {
7165
// Cast `value` to a string to ensure the value is set correctly. While
7266
// browsers typically do this as necessary, jsdom doesn't.
@@ -76,10 +70,19 @@ export function updateTextarea(element: Element, props: Object) {
7670
node.value = newValue;
7771
}
7872
// 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;
8178
}
8279
}
80+
const defaultValue = getToStringValue(props.defaultValue);
81+
if (defaultValue != null) {
82+
node.defaultValue = toString(defaultValue);
83+
} else {
84+
node.defaultValue = '';
85+
}
8386
}
8487

8588
export function initTextarea(element: Element, props: Object) {

packages/react-dom/src/__tests__/ReactDOMComponent-test.js

Lines changed: 65 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1043,6 +1043,63 @@ describe('ReactDOMComponent', () => {
10431043
expect(nodeValueSetter).toHaveBeenCalledTimes(2);
10441044
});
10451045

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+
10461103
it('should not incur unnecessary DOM mutations for boolean properties', () => {
10471104
const container = document.createElement('div');
10481105
function onChange() {
@@ -1066,7 +1123,12 @@ describe('ReactDOMComponent', () => {
10661123
});
10671124

10681125
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+
/>,
10701132
container,
10711133
);
10721134
expect(nodeValueSetter).toHaveBeenCalledTimes(0);
@@ -1094,15 +1156,13 @@ describe('ReactDOMComponent', () => {
10941156
'using a controlled or uncontrolled input element for the lifetime of the component.',
10951157
);
10961158

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);
10991160

11001161
ReactDOM.render(
11011162
<input type="checkbox" onChange={onChange} checked={true} />,
11021163
container,
11031164
);
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);
11061166
});
11071167

11081168
it('should ignore attribute list for elements with the "is" attribute', () => {

packages/react-dom/src/__tests__/ReactDOMTextarea-test.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,7 @@ describe('ReactDOMTextarea', () => {
603603
ref={n => (node = n)}
604604
value="foo"
605605
onChange={emptyFunction}
606+
data-count={this.state.count}
606607
/>
607608
</div>
608609
);

0 commit comments

Comments
 (0)