Skip to content

Commit

Permalink
Switching checked to null should leave the current value (#26667)
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@sebmarkbage @sophiebits @kassens
2 people authored and kassens committed Apr 21, 2023
1 parent e0a89a1 commit 5d9139b
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 140 deletions.
126 changes: 14 additions & 112 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,7 @@ export function setInitialProperties(
// listeners still fire for the invalid event.
listenToNonDelegatedEvent('invalid', domElement);

let name = null;
let type = null;
let value = null;
let defaultValue = null;
Expand All @@ -848,31 +849,16 @@ export function setInitialProperties(
continue;
}
switch (propKey) {
case 'name': {
name = propValue;
break;
}
case 'type': {
// Fast path since 'type' is very common on inputs
if (
propValue != null &&
typeof propValue !== 'function' &&
typeof propValue !== 'symbol' &&
typeof propValue !== 'boolean'
) {
type = propValue;
if (__DEV__) {
checkAttributeStringCoercion(propValue, propKey);
}
domElement.setAttribute(propKey, propValue);
}
type = propValue;
break;
}
case 'checked': {
checked = propValue;
const checkedValue =
propValue != null ? propValue : props.defaultChecked;
const inputElement: HTMLInputElement = (domElement: any);
inputElement.checked =
!!checkedValue &&
typeof checkedValue !== 'function' &&
checkedValue !== 'symbol';
break;
}
case 'defaultChecked': {
Expand Down Expand Up @@ -904,7 +890,6 @@ export function setInitialProperties(
}
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
track((domElement: any));
validateInputProps(domElement, props);
initInput(
domElement,
Expand All @@ -913,8 +898,10 @@ export function setInitialProperties(
checked,
defaultChecked,
type,
name,
false,
);
track((domElement: any));
return;
}
case 'select': {
Expand Down Expand Up @@ -1010,9 +997,9 @@ export function setInitialProperties(
}
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
track((domElement: any));
validateTextareaProps(domElement, props);
initTextarea(domElement, value, defaultValue, children);
track((domElement: any));
return;
}
case 'option': {
Expand Down Expand Up @@ -1305,14 +1292,6 @@ export function updateProperties(
if (lastProps.hasOwnProperty(propKey) && lastProp != null) {
switch (propKey) {
case 'checked': {
if (!nextProps.hasOwnProperty(propKey)) {
const checkedValue = nextProps.defaultChecked;
const inputElement: HTMLInputElement = (domElement: any);
inputElement.checked =
!!checkedValue &&
typeof checkedValue !== 'function' &&
checkedValue !== 'symbol';
}
break;
}
case 'value': {
Expand Down Expand Up @@ -1341,22 +1320,6 @@ export function updateProperties(
switch (propKey) {
case 'type': {
type = nextProp;
// Fast path since 'type' is very common on inputs
if (nextProp !== lastProp) {
if (
nextProp != null &&
typeof nextProp !== 'function' &&
typeof nextProp !== 'symbol' &&
typeof nextProp !== 'boolean'
) {
if (__DEV__) {
checkAttributeStringCoercion(nextProp, propKey);
}
domElement.setAttribute(propKey, nextProp);
} else {
domElement.removeAttribute(propKey);
}
}
break;
}
case 'name': {
Expand All @@ -1365,15 +1328,6 @@ export function updateProperties(
}
case 'checked': {
checked = nextProp;
if (nextProp !== lastProp) {
const checkedValue =
nextProp != null ? nextProp : nextProps.defaultChecked;
const inputElement: HTMLInputElement = (domElement: any);
inputElement.checked =
!!checkedValue &&
typeof checkedValue !== 'function' &&
checkedValue !== 'symbol';
}
break;
}
case 'defaultChecked': {
Expand Down Expand Up @@ -1453,23 +1407,6 @@ export function updateProperties(
}
}

// Update checked *before* name.
// In the middle of an update, it is possible to have multiple checked.
// When a checked radio tries to change name, browser makes another radio's checked false.
if (
name != null &&
typeof name !== 'function' &&
typeof name !== 'symbol' &&
typeof name !== 'boolean'
) {
if (__DEV__) {
checkAttributeStringCoercion(name, 'name');
}
domElement.setAttribute('name', name);
} else {
domElement.removeAttribute('name');
}

// Update the wrapper around inputs *after* updating props. This has to
// happen after updating the rest of props. Otherwise HTML5 input validations
// raise warnings and prevent the new value from being assigned.
Expand All @@ -1481,6 +1418,7 @@ export function updateProperties(
checked,
defaultChecked,
type,
name,
);
return;
}
Expand Down Expand Up @@ -1822,33 +1760,12 @@ export function updatePropertiesWithDiff(
const propValue = updatePayload[i + 1];
switch (propKey) {
case 'type': {
// Fast path since 'type' is very common on inputs
if (
propValue != null &&
typeof propValue !== 'function' &&
typeof propValue !== 'symbol' &&
typeof propValue !== 'boolean'
) {
if (__DEV__) {
checkAttributeStringCoercion(propValue, propKey);
}
domElement.setAttribute(propKey, propValue);
} else {
domElement.removeAttribute(propKey);
}
break;
}
case 'name': {
break;
}
case 'checked': {
const checkedValue =
propValue != null ? propValue : nextProps.defaultChecked;
const inputElement: HTMLInputElement = (domElement: any);
inputElement.checked =
!!checkedValue &&
typeof checkedValue !== 'function' &&
checkedValue !== 'symbol';
break;
}
case 'defaultChecked': {
Expand Down Expand Up @@ -1916,23 +1833,6 @@ export function updatePropertiesWithDiff(
}
}

// Update checked *before* name.
// In the middle of an update, it is possible to have multiple checked.
// When a checked radio tries to change name, browser makes another radio's checked false.
if (
name != null &&
typeof name !== 'function' &&
typeof name !== 'symbol' &&
typeof name !== 'boolean'
) {
if (__DEV__) {
checkAttributeStringCoercion(name, 'name');
}
domElement.setAttribute('name', name);
} else {
domElement.removeAttribute('name');
}

// Update the wrapper around inputs *after* updating props. This has to
// happen after updating the rest of props. Otherwise HTML5 input validations
// raise warnings and prevent the new value from being assigned.
Expand All @@ -1944,6 +1844,7 @@ export function updatePropertiesWithDiff(
checked,
defaultChecked,
type,
name,
);
return;
}
Expand Down Expand Up @@ -2970,7 +2871,6 @@ export function diffHydratedProperties(
listenToNonDelegatedEvent('invalid', domElement);
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
track((domElement: any));
validateInputProps(domElement, props);
// For input and textarea we current always set the value property at
// post mount to force it to diverge from attributes. However, for
Expand All @@ -2984,8 +2884,10 @@ export function diffHydratedProperties(
props.checked,
props.defaultChecked,
props.type,
props.name,
true,
);
track((domElement: any));
break;
case 'option':
validateOptionProps(domElement, props);
Expand All @@ -3008,9 +2910,9 @@ export function diffHydratedProperties(
listenToNonDelegatedEvent('invalid', domElement);
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
track((domElement: any));
validateTextareaProps(domElement, props);
initTextarea(domElement, props.value, props.defaultValue, props.children);
track((domElement: any));
break;
}

Expand Down
65 changes: 60 additions & 5 deletions packages/react-dom-bindings/src/client/ReactDOMInput.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,30 @@ export function updateInput(
checked: ?boolean,
defaultChecked: ?boolean,
type: ?string,
name: ?string,
) {
const node: HTMLInputElement = (element: any);

// Temporarily disconnect the input from any radio buttons.
// Changing the type or name as the same time as changing the checked value
// needs to be atomically applied. We can only ensure that by disconnecting
// the name while do the mutations and then reapply the name after that's done.
node.name = '';

if (
type != null &&
typeof type !== 'function' &&
typeof type !== 'symbol' &&
typeof type !== 'boolean'
) {
if (__DEV__) {
checkAttributeStringCoercion(type, 'type');
}
node.type = type;
} else {
node.removeAttribute('type');
}

if (value != null) {
if (type === 'number') {
if (
Expand Down Expand Up @@ -157,6 +178,20 @@ export function updateInput(
if (checked != null && node.checked !== !!checked) {
node.checked = checked;
}

if (
name != null &&
typeof name !== 'function' &&
typeof name !== 'symbol' &&
typeof name !== 'boolean'
) {
if (__DEV__) {
checkAttributeStringCoercion(name, 'name');
}
node.name = name;
} else {
node.removeAttribute('name');
}
}

export function initInput(
Expand All @@ -166,10 +201,23 @@ export function initInput(
checked: ?boolean,
defaultChecked: ?boolean,
type: ?string,
name: ?string,
isHydrating: boolean,
) {
const node: HTMLInputElement = (element: any);

if (
type != null &&
typeof type !== 'function' &&
typeof type !== 'symbol' &&
typeof type !== 'boolean'
) {
if (__DEV__) {
checkAttributeStringCoercion(type, 'type');
}
node.type = type;
}

if (value != null || defaultValue != null) {
const isButton = type === 'submit' || type === 'reset';

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

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

if (name !== '') {
// Name needs to be set at the end so that it applies atomically to connected radio buttons.
if (
name != null &&
typeof name !== 'function' &&
typeof name !== 'symbol' &&
typeof name !== 'boolean'
) {
if (__DEV__) {
checkAttributeStringCoercion(name, 'name');
}
node.name = name;
}
}
Expand All @@ -291,6 +344,7 @@ export function restoreControlledInputState(element: Element, props: Object) {
props.checked,
props.defaultChecked,
props.type,
props.name,
);
const name = props.name;
if (props.type === 'radio' && name != null) {
Expand Down Expand Up @@ -347,6 +401,7 @@ export function restoreControlledInputState(element: Element, props: Object) {
otherProps.checked,
otherProps.defaultChecked,
otherProps.type,
otherProps.name,
);
}
}
Expand Down
Loading

0 comments on commit 5d9139b

Please sign in to comment.