Skip to content

Commit 5d9139b

Browse files
sebmarkbagesophiebits
authored and
kassens
committed
Switching checked to null should leave the current value (#26667)
I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in #26596 (comment) and #26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com>
1 parent e0a89a1 commit 5d9139b

File tree

5 files changed

+169
-140
lines changed

5 files changed

+169
-140
lines changed

‎packages/react-dom-bindings/src/client/ReactDOMComponent.js‎

Lines changed: 14 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -834,6 +834,7 @@ export function setInitialProperties(
834834
// listeners still fire for the invalid event.
835835
listenToNonDelegatedEvent('invalid', domElement);
836836

837+
let name = null;
837838
let type = null;
838839
let value = null;
839840
let defaultValue = null;
@@ -848,31 +849,16 @@ export function setInitialProperties(
848849
continue;
849850
}
850851
switch (propKey) {
852+
case 'name': {
853+
name = propValue;
854+
break;
855+
}
851856
case 'type': {
852-
// Fast path since 'type' is very common on inputs
853-
if (
854-
propValue != null &&
855-
typeof propValue !== 'function' &&
856-
typeof propValue !== 'symbol' &&
857-
typeof propValue !== 'boolean'
858-
) {
859-
type = propValue;
860-
if (__DEV__) {
861-
checkAttributeStringCoercion(propValue, propKey);
862-
}
863-
domElement.setAttribute(propKey, propValue);
864-
}
857+
type = propValue;
865858
break;
866859
}
867860
case 'checked': {
868861
checked = propValue;
869-
const checkedValue =
870-
propValue != null ? propValue : props.defaultChecked;
871-
const inputElement: HTMLInputElement = (domElement: any);
872-
inputElement.checked =
873-
!!checkedValue &&
874-
typeof checkedValue !== 'function' &&
875-
checkedValue !== 'symbol';
876862
break;
877863
}
878864
case 'defaultChecked': {
@@ -904,7 +890,6 @@ export function setInitialProperties(
904890
}
905891
// TODO: Make sure we check if this is still unmounted or do any clean
906892
// up necessary since we never stop tracking anymore.
907-
track((domElement: any));
908893
validateInputProps(domElement, props);
909894
initInput(
910895
domElement,
@@ -913,8 +898,10 @@ export function setInitialProperties(
913898
checked,
914899
defaultChecked,
915900
type,
901+
name,
916902
false,
917903
);
904+
track((domElement: any));
918905
return;
919906
}
920907
case 'select': {
@@ -1010,9 +997,9 @@ export function setInitialProperties(
1010997
}
1011998
// TODO: Make sure we check if this is still unmounted or do any clean
1012999
// up necessary since we never stop tracking anymore.
1013-
track((domElement: any));
10141000
validateTextareaProps(domElement, props);
10151001
initTextarea(domElement, value, defaultValue, children);
1002+
track((domElement: any));
10161003
return;
10171004
}
10181005
case 'option': {
@@ -1305,14 +1292,6 @@ export function updateProperties(
13051292
if (lastProps.hasOwnProperty(propKey) && lastProp != null) {
13061293
switch (propKey) {
13071294
case 'checked': {
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-
}
13161295
break;
13171296
}
13181297
case 'value': {
@@ -1341,22 +1320,6 @@ export function updateProperties(
13411320
switch (propKey) {
13421321
case 'type': {
13431322
type = nextProp;
1344-
// Fast path since 'type' is very common on inputs
1345-
if (nextProp !== lastProp) {
1346-
if (
1347-
nextProp != null &&
1348-
typeof nextProp !== 'function' &&
1349-
typeof nextProp !== 'symbol' &&
1350-
typeof nextProp !== 'boolean'
1351-
) {
1352-
if (__DEV__) {
1353-
checkAttributeStringCoercion(nextProp, propKey);
1354-
}
1355-
domElement.setAttribute(propKey, nextProp);
1356-
} else {
1357-
domElement.removeAttribute(propKey);
1358-
}
1359-
}
13601323
break;
13611324
}
13621325
case 'name': {
@@ -1365,15 +1328,6 @@ export function updateProperties(
13651328
}
13661329
case 'checked': {
13671330
checked = nextProp;
1368-
if (nextProp !== lastProp) {
1369-
const checkedValue =
1370-
nextProp != null ? nextProp : nextProps.defaultChecked;
1371-
const inputElement: HTMLInputElement = (domElement: any);
1372-
inputElement.checked =
1373-
!!checkedValue &&
1374-
typeof checkedValue !== 'function' &&
1375-
checkedValue !== 'symbol';
1376-
}
13771331
break;
13781332
}
13791333
case 'defaultChecked': {
@@ -1453,23 +1407,6 @@ export function updateProperties(
14531407
}
14541408
}
14551409

1456-
// Update checked *before* name.
1457-
// In the middle of an update, it is possible to have multiple checked.
1458-
// When a checked radio tries to change name, browser makes another radio's checked false.
1459-
if (
1460-
name != null &&
1461-
typeof name !== 'function' &&
1462-
typeof name !== 'symbol' &&
1463-
typeof name !== 'boolean'
1464-
) {
1465-
if (__DEV__) {
1466-
checkAttributeStringCoercion(name, 'name');
1467-
}
1468-
domElement.setAttribute('name', name);
1469-
} else {
1470-
domElement.removeAttribute('name');
1471-
}
1472-
14731410
// Update the wrapper around inputs *after* updating props. This has to
14741411
// happen after updating the rest of props. Otherwise HTML5 input validations
14751412
// raise warnings and prevent the new value from being assigned.
@@ -1481,6 +1418,7 @@ export function updateProperties(
14811418
checked,
14821419
defaultChecked,
14831420
type,
1421+
name,
14841422
);
14851423
return;
14861424
}
@@ -1822,33 +1760,12 @@ export function updatePropertiesWithDiff(
18221760
const propValue = updatePayload[i + 1];
18231761
switch (propKey) {
18241762
case 'type': {
1825-
// Fast path since 'type' is very common on inputs
1826-
if (
1827-
propValue != null &&
1828-
typeof propValue !== 'function' &&
1829-
typeof propValue !== 'symbol' &&
1830-
typeof propValue !== 'boolean'
1831-
) {
1832-
if (__DEV__) {
1833-
checkAttributeStringCoercion(propValue, propKey);
1834-
}
1835-
domElement.setAttribute(propKey, propValue);
1836-
} else {
1837-
domElement.removeAttribute(propKey);
1838-
}
18391763
break;
18401764
}
18411765
case 'name': {
18421766
break;
18431767
}
18441768
case 'checked': {
1845-
const checkedValue =
1846-
propValue != null ? propValue : nextProps.defaultChecked;
1847-
const inputElement: HTMLInputElement = (domElement: any);
1848-
inputElement.checked =
1849-
!!checkedValue &&
1850-
typeof checkedValue !== 'function' &&
1851-
checkedValue !== 'symbol';
18521769
break;
18531770
}
18541771
case 'defaultChecked': {
@@ -1916,23 +1833,6 @@ export function updatePropertiesWithDiff(
19161833
}
19171834
}
19181835

1919-
// Update checked *before* name.
1920-
// In the middle of an update, it is possible to have multiple checked.
1921-
// When a checked radio tries to change name, browser makes another radio's checked false.
1922-
if (
1923-
name != null &&
1924-
typeof name !== 'function' &&
1925-
typeof name !== 'symbol' &&
1926-
typeof name !== 'boolean'
1927-
) {
1928-
if (__DEV__) {
1929-
checkAttributeStringCoercion(name, 'name');
1930-
}
1931-
domElement.setAttribute('name', name);
1932-
} else {
1933-
domElement.removeAttribute('name');
1934-
}
1935-
19361836
// Update the wrapper around inputs *after* updating props. This has to
19371837
// happen after updating the rest of props. Otherwise HTML5 input validations
19381838
// raise warnings and prevent the new value from being assigned.
@@ -1944,6 +1844,7 @@ export function updatePropertiesWithDiff(
19441844
checked,
19451845
defaultChecked,
19461846
type,
1847+
name,
19471848
);
19481849
return;
19491850
}
@@ -2970,7 +2871,6 @@ export function diffHydratedProperties(
29702871
listenToNonDelegatedEvent('invalid', domElement);
29712872
// TODO: Make sure we check if this is still unmounted or do any clean
29722873
// up necessary since we never stop tracking anymore.
2973-
track((domElement: any));
29742874
validateInputProps(domElement, props);
29752875
// For input and textarea we current always set the value property at
29762876
// post mount to force it to diverge from attributes. However, for
@@ -2984,8 +2884,10 @@ export function diffHydratedProperties(
29842884
props.checked,
29852885
props.defaultChecked,
29862886
props.type,
2887+
props.name,
29872888
true,
29882889
);
2890+
track((domElement: any));
29892891
break;
29902892
case 'option':
29912893
validateOptionProps(domElement, props);
@@ -3008,9 +2910,9 @@ export function diffHydratedProperties(
30082910
listenToNonDelegatedEvent('invalid', domElement);
30092911
// TODO: Make sure we check if this is still unmounted or do any clean
30102912
// up necessary since we never stop tracking anymore.
3011-
track((domElement: any));
30122913
validateTextareaProps(domElement, props);
30132914
initTextarea(domElement, props.value, props.defaultValue, props.children);
2915+
track((domElement: any));
30142916
break;
30152917
}
30162918

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

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,30 @@ export function updateInput(
8989
checked: ?boolean,
9090
defaultChecked: ?boolean,
9191
type: ?string,
92+
name: ?string,
9293
) {
9394
const node: HTMLInputElement = (element: any);
9495

96+
// Temporarily disconnect the input from any radio buttons.
97+
// Changing the type or name as the same time as changing the checked value
98+
// needs to be atomically applied. We can only ensure that by disconnecting
99+
// the name while do the mutations and then reapply the name after that's done.
100+
node.name = '';
101+
102+
if (
103+
type != null &&
104+
typeof type !== 'function' &&
105+
typeof type !== 'symbol' &&
106+
typeof type !== 'boolean'
107+
) {
108+
if (__DEV__) {
109+
checkAttributeStringCoercion(type, 'type');
110+
}
111+
node.type = type;
112+
} else {
113+
node.removeAttribute('type');
114+
}
115+
95116
if (value != null) {
96117
if (type === 'number') {
97118
if (
@@ -157,6 +178,20 @@ export function updateInput(
157178
if (checked != null && node.checked !== !!checked) {
158179
node.checked = checked;
159180
}
181+
182+
if (
183+
name != null &&
184+
typeof name !== 'function' &&
185+
typeof name !== 'symbol' &&
186+
typeof name !== 'boolean'
187+
) {
188+
if (__DEV__) {
189+
checkAttributeStringCoercion(name, 'name');
190+
}
191+
node.name = name;
192+
} else {
193+
node.removeAttribute('name');
194+
}
160195
}
161196

162197
export function initInput(
@@ -166,10 +201,23 @@ export function initInput(
166201
checked: ?boolean,
167202
defaultChecked: ?boolean,
168203
type: ?string,
204+
name: ?string,
169205
isHydrating: boolean,
170206
) {
171207
const node: HTMLInputElement = (element: any);
172208

209+
if (
210+
type != null &&
211+
typeof type !== 'function' &&
212+
typeof type !== 'symbol' &&
213+
typeof type !== 'boolean'
214+
) {
215+
if (__DEV__) {
216+
checkAttributeStringCoercion(type, 'type');
217+
}
218+
node.type = type;
219+
}
220+
173221
if (value != null || defaultValue != null) {
174222
const isButton = type === 'submit' || type === 'reset';
175223

@@ -235,10 +283,6 @@ export function initInput(
235283
// will sometimes influence the value of checked (even after detachment).
236284
// Reference: https://bugs.chromium.org/p/chromium/issues/detail?id=608416
237285
// We need to temporarily unset name to avoid disrupting radio button groups.
238-
const name = node.name;
239-
if (name !== '') {
240-
node.name = '';
241-
}
242286

243287
const checkedOrDefault = checked != null ? checked : defaultChecked;
244288
// TODO: This 'function' or 'symbol' check isn't replicated in other places
@@ -276,7 +320,16 @@ export function initInput(
276320
node.defaultChecked = !!initialChecked;
277321
}
278322

279-
if (name !== '') {
323+
// Name needs to be set at the end so that it applies atomically to connected radio buttons.
324+
if (
325+
name != null &&
326+
typeof name !== 'function' &&
327+
typeof name !== 'symbol' &&
328+
typeof name !== 'boolean'
329+
) {
330+
if (__DEV__) {
331+
checkAttributeStringCoercion(name, 'name');
332+
}
280333
node.name = name;
281334
}
282335
}
@@ -291,6 +344,7 @@ export function restoreControlledInputState(element: Element, props: Object) {
291344
props.checked,
292345
props.defaultChecked,
293346
props.type,
347+
props.name,
294348
);
295349
const name = props.name;
296350
if (props.type === 'radio' && name != null) {
@@ -347,6 +401,7 @@ export function restoreControlledInputState(element: Element, props: Object) {
347401
otherProps.checked,
348402
otherProps.defaultChecked,
349403
otherProps.type,
404+
otherProps.name,
350405
);
351406
}
352407
}

0 commit comments

Comments
 (0)