Fix input tracking bug (#26627)
In https://github.com/facebook/react/pull/26573/commits/2019ddc75f448292ffa6429d7625514af192631b, we changed to set .defaultValue before .value on updates. In some cases, setting .defaultValue causes .value to change, and since we only set .value if it has the wrong value, this resulted in us not assigning to .value, which resulted in inputValueTracking not knowing the right value. See new test added. My fix here is to (a) move the value setting back up first and (b) narrowing the fix in the aforementioned PR to newly remove the value attribute only if it defaultValue was previously present in props. The second half is necessary because for types where the value property and attribute are indelibly linked (hidden checkbox radio submit image reset button, i.e. spec modes default or default/on from https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default), we can't remove the value attribute after setting .value, because that will undo the assignment we just did! That is, not having (b) makes all of those types fail to handle updating props.value. This code is incredibly hard to think about but I think this is right (or at least, as right as the old code was) because we set .value here only if the nextProps.value != null, and we now remove defaultValue only if lastProps.defaultValue != null. These can't happen at the same time because we have long warned if value and defaultValue are simultaneously specified, and also if a component switches between controlled and uncontrolled. Also, it fixes the test in https://github.com/facebook/react/pull/26626.
Sophie Alpert committed
Apr 18, 2023 at 10:49 UTC
b433c379d55d9684945217c7d375de1082a1abb8
4 files changed
+82
-38
packages/react-dom-bindings/src/client/ReactDOMComponent.js
+19
-12
@@ -1297,32 +1297,36 @@ export function updateProperties(
1297
let type = null;
1298
let value = null;
1299
let defaultValue = null;
1300
+ let lastDefaultValue = null;
1301
let checked = null;
1302
let defaultChecked = null;
1303
for (const propKey in lastProps) {
1304
const lastProp = lastProps[propKey];
1304
- if (
1305
- lastProps.hasOwnProperty(propKey) &&
1306
- lastProp != null &&
1307
- !nextProps.hasOwnProperty(propKey)
1308
- ) {
1305
+ if (lastProps.hasOwnProperty(propKey) && lastProp != null) {
1306
switch (propKey) {
1307
case 'checked': {
1311
- const checkedValue = nextProps.defaultChecked;
1312
- const inputElement: HTMLInputElement = (domElement: any);
1313
- inputElement.checked =
1314
- !!checkedValue &&
1315
- typeof checkedValue !== 'function' &&
1316
- checkedValue !== 'symbol';
1308
+ if (!nextProps.hasOwnProperty(propKey)) {
1309
+ const checkedValue = nextProps.defaultChecked;
1310
+ const inputElement: HTMLInputElement = (domElement: any);
1311
+ inputElement.checked =
1312
+ !!checkedValue &&
1313
+ typeof checkedValue !== 'function' &&
1314
+ checkedValue !== 'symbol';
1315
+ }
1316
break;
1317
}
1318
case 'value': {
1319
// This is handled by updateWrapper below.
1320
break;
1321
}
1322
+ case 'defaultValue': {
1323
+ lastDefaultValue = lastProp;
1324
+ }
1325
// defaultChecked and defaultValue are ignored by setProp
1326
+ // Fallthrough
1327
default: {
1325
- setProp(domElement, tag, propKey, null, nextProps, lastProp);
1328
+ if (!nextProps.hasOwnProperty(propKey))
1329
+ setProp(domElement, tag, propKey, null, nextProps, lastProp);
1330
}
1331
}
1332
}
@@ -1473,6 +1477,7 @@ export function updateProperties(
1477
domElement,
1478
value,
1479
defaultValue,
1480
+ lastDefaultValue,
1481
checked,
1482
defaultChecked,
1483
type,
@@ -1809,6 +1814,7 @@ export function updatePropertiesWithDiff(
1814
const type = nextProps.type;
1815
const value = nextProps.value;
1816
const defaultValue = nextProps.defaultValue;
1817
+ const lastDefaultValue = lastProps.defaultValue;
1818
const checked = nextProps.checked;
1819
const defaultChecked = nextProps.defaultChecked;
1820
for (let i = 0; i < updatePayload.length; i += 2) {
@@ -1934,6 +1940,7 @@ export function updatePropertiesWithDiff(
1940
domElement,
1941
value,
1942
defaultValue,
1943
+ lastDefaultValue,
1944
checked,
1945
defaultChecked,
1946
type,
packages/react-dom-bindings/src/client/ReactDOMInput.js
+26
-23
@@ -85,19 +85,41 @@ export function updateInput(
85
element: Element,
86
value: ?string,
87
defaultValue: ?string,
88
+ lastDefaultValue: ?string,
89
checked: ?boolean,
90
defaultChecked: ?boolean,
91
type: ?string,
92
) {
93
const node: HTMLInputElement = (element: any);
94
95
+ if (value != null) {
96
+ if (type === 'number') {
97
+ if (
98
+ // $FlowFixMe[incompatible-type]
99
+ (value === 0 && node.value === '') ||
100
+ // We explicitly want to coerce to number here if possible.
101
+ // eslint-disable-next-line
102
+ node.value != (value: any)
103
+ ) {
104
+ node.value = toString(getToStringValue(value));
105
+ }
106
+ } else if (node.value !== toString(getToStringValue(value))) {
107
+ node.value = toString(getToStringValue(value));
108
+ }
109
+ } else if (type === 'submit' || type === 'reset') {
110
+ // Submit/reset inputs need the attribute removed completely to avoid
111
+ // blank-text buttons.
112
+ node.removeAttribute('value');
113
+ return;
114
+ }
115
+
116
if (disableInputAttributeSyncing) {
117
// When not syncing the value attribute, React only assigns a new value
118
// whenever the defaultValue React prop has changed. When not present,
119
// React does nothing
120
if (defaultValue != null) {
121
setDefaultValue(node, type, getToStringValue(defaultValue));
100
- } else {
122
+ } else if (lastDefaultValue != null) {
123
node.removeAttribute('value');
124
}
125
} else {
@@ -110,7 +132,7 @@ export function updateInput(
132
setDefaultValue(node, type, getToStringValue(value));
133
} else if (defaultValue != null) {
134
setDefaultValue(node, type, getToStringValue(defaultValue));
113
- } else {
135
+ } else if (lastDefaultValue != null) {
136
node.removeAttribute('value');
137
}
138
}
@@ -135,27 +157,6 @@ export function updateInput(
157
if (checked != null && node.checked !== !!checked) {
158
node.checked = checked;
159
}
138
-
139
- if (value != null) {
140
- if (type === 'number') {
141
- if (
142
- // $FlowFixMe[incompatible-type]
143
- (value === 0 && node.value === '') ||
144
- // We explicitly want to coerce to number here if possible.
145
- // eslint-disable-next-line
146
- node.value != (value: any)
147
- ) {
148
- node.value = toString(getToStringValue(value));
149
- }
150
- } else if (node.value !== toString(getToStringValue(value))) {
151
- node.value = toString(getToStringValue(value));
152
- }
153
- } else if (type === 'submit' || type === 'reset') {
154
- // Submit/reset inputs need the attribute removed completely to avoid
155
- // blank-text buttons.
156
- node.removeAttribute('value');
157
- return;
158
- }
160
}
161
162
export function initInput(
@@ -286,6 +287,7 @@ export function restoreControlledInputState(element: Element, props: Object) {
287
rootNode,
288
props.value,
289
props.defaultValue,
290
+ props.defaultValue,
291
props.checked,
292
props.defaultChecked,
293
props.type,
@@ -341,6 +343,7 @@ export function restoreControlledInputState(element: Element, props: Object) {
343
otherNode,
344
otherProps.value,
345
otherProps.defaultValue,
346
+ otherProps.defaultValue,
347
otherProps.checked,
348
otherProps.defaultChecked,
349
otherProps.type,
packages/react-dom/src/__tests__/DOMPropertyOperations-test.js
+5
-1
@@ -1166,7 +1166,11 @@ describe('DOMPropertyOperations', () => {
1166
).toErrorDev(
1167
'A component is changing a controlled input to be uncontrolled',
1168
);
1169
- expect(container.firstChild.hasAttribute('value')).toBe(false);
1169
+ if (disableInputAttributeSyncing) {
1170
+ expect(container.firstChild.hasAttribute('value')).toBe(false);
1171
+ } else {
1172
+ expect(container.firstChild.getAttribute('value')).toBe('foo');
1173
+ }
1174
expect(container.firstChild.value).toBe('foo');
1175
});
1176
packages/react-dom/src/__tests__/ReactDOMInput-test.js
+32
-2
@@ -1952,7 +1952,11 @@ describe('ReactDOMInput', () => {
1952
expect(renderInputWithStringThenWithUndefined).toErrorDev(
1953
'A component is changing a controlled input to be uncontrolled.',
1954
);
1955
- expect(input.getAttribute('value')).toBe(null);
1955
+ if (disableInputAttributeSyncing) {
1956
+ expect(input.getAttribute('value')).toBe(null);
1957
+ } else {
1958
+ expect(input.getAttribute('value')).toBe('latest');
1959
+ }
1960
});
1961
1962
it('preserves the value property', () => {
@@ -1998,7 +2002,11 @@ describe('ReactDOMInput', () => {
2002
'or `undefined` for uncontrolled components.',
2003
'A component is changing a controlled input to be uncontrolled.',
2004
]);
2001
- expect(input.hasAttribute('value')).toBe(false);
2005
+ if (disableInputAttributeSyncing) {
2006
+ expect(input.getAttribute('value')).toBe(null);
2007
+ } else {
2008
+ expect(input.getAttribute('value')).toBe('latest');
2009
+ }
2010
});
2011
2012
it('preserves the value property', () => {
@@ -2183,4 +2191,26 @@ describe('ReactDOMInput', () => {
2191
ReactDOM.render(<input type="text" defaultValue={null} />, container);
2192
expect(node.defaultValue).toBe('');
2193
});
2194
+
2195
+ it('should notice input changes when reverting back to original value', () => {
2196
+ const log = [];
2197
+ function onChange(e) {
2198
+ log.push(e.target.value);
2199
+ }
2200
+ ReactDOM.render(
2201
+ <input type="text" value="" onChange={onChange} />,
2202
+ container,
2203
+ );
2204
+ ReactDOM.render(
2205
+ <input type="text" value="a" onChange={onChange} />,
2206
+ container,
2207
+ );
2208
+
2209
+ const node = container.firstChild;
2210
+ setUntrackedValue.call(node, '');
2211
+ dispatchEventOnNode(node, 'input');
2212
+
2213
+ expect(log).toEqual(['']);
2214
+ expect(node.value).toBe('a');
2215
+ });
2216
});