OSSForks/react
1
0
Fork 0
mirror of https://github.com/zebrajr/react.git synced 2026-01-15 12:15:22 +00:00
Code Issues Actions 6 Packages Projects Releases Wiki Activity

Switching checked to null should leave the current value (#26667)

Browse Source 
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`:


5cbe6258bc/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
https://github.com/facebook/react/pull/26596#discussion_r1162295872 and
https://github.com/facebook/react/pull/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>
This commit is contained in:
Sebastian Markbåge
2023-04-19 11:46:29 -04:00
committed by GitHub
parent b90e8ebaa5
commit 1f248bdd71
5 changed files with 169 additions and 140 deletions
Download Patch File Download Diff File Expand all files Collapse all files

126
packages/react-dom-bindings/src/client/ReactDOMComponent.js vendored
View File

@@ -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;
@@ -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': {
@@ -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,
@@ -913,8 +898,10 @@ export function setInitialProperties(
checked,
defaultChecked,
type,
name,
false,
);
track((domElement: any));
return;
}
case 'select': {
@@ -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': {
@@ -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': {
@@ -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': {
@@ -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': {
@@ -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.
@@ -1481,6 +1418,7 @@ export function updateProperties(
checked,
defaultChecked,
type,
name,
);
return;
}
@@ -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': {
@@ -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.
@@ -1944,6 +1844,7 @@ export function updatePropertiesWithDiff(
checked,
defaultChecked,
type,
name,
);
return;
}
@@ -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
@@ -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);
@@ -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;
}

65
packages/react-dom-bindings/src/client/ReactDOMInput.js vendored
View File

@@ -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 (
@@ -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(
@@ -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';
@@ -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
@@ -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;
}
}
@@ -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) {
@@ -347,6 +401,7 @@ export function restoreControlledInputState(element: Element, props: Object) {
otherProps.checked,
otherProps.defaultChecked,
otherProps.type,
otherProps.name,
);
}
}

7
packages/react-dom/src/__tests__/ReactDOMComponent-test.js vendored
View File

@@ -1143,7 +1143,8 @@ describe('ReactDOMComponent', () => {
'the value changing from a defined to undefined, which should not happen. Decide between ' +
'using a controlled or uncontrolled input element for the lifetime of the component.',
);
expect(nodeValueSetter).toHaveBeenCalledTimes(1);
// This leaves the current checked value in place, just like text inputs.
expect(nodeValueSetter).toHaveBeenCalledTimes(0);
expect(() => {
ReactDOM.render(
@@ -1156,13 +1157,13 @@ describe('ReactDOMComponent', () => {
'using a controlled or uncontrolled input element for the lifetime of the component.',
);
expect(nodeValueSetter).toHaveBeenCalledTimes(2);
expect(nodeValueSetter).toHaveBeenCalledTimes(1);
ReactDOM.render(
<input type="checkbox" onChange={onChange} checked={true} />,
container,
);
expect(nodeValueSetter).toHaveBeenCalledTimes(3);
expect(nodeValueSetter).toHaveBeenCalledTimes(2);
});
it('should ignore attribute list for elements with the "is" attribute', () => {

91
packages/react-dom/src/__tests__/ReactDOMInput-test.js vendored
View File

@@ -1191,7 +1191,7 @@ describe('ReactDOMInput', () => {
updated: false,
};
onClick = () => {
this.setState({updated: true});
this.setState({updated: !this.state.updated});
};
render() {
const {updated} = this.state;
@@ -1222,6 +1222,62 @@ describe('ReactDOMInput', () => {
expect(firstRadioNode.checked).toBe(false);
dispatchEventOnNode(buttonNode, 'click');
expect(firstRadioNode.checked).toBe(true);
dispatchEventOnNode(buttonNode, 'click');
expect(firstRadioNode.checked).toBe(false);
});
it("shouldn't get tricked by changing radio names, part 2", () => {
ReactDOM.render(
<div>
<input
type="radio"
name="a"
value="1"
checked={true}
onChange={() => {}}
/>
<input
type="radio"
name="a"
value="2"
checked={false}
onChange={() => {}}
/>
</div>,
container,
);
expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
true,
);
expect(container.querySelector('input[name="a"][value="2"]').checked).toBe(
false,
);
ReactDOM.render(
<div>
<input
type="radio"
name="a"
value="1"
checked={true}
onChange={() => {}}
/>
<input
type="radio"
name="b"
value="2"
checked={true}
onChange={() => {}}
/>
</div>,
container,
);
expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
true,
);
expect(container.querySelector('input[name="b"][value="2"]').checked).toBe(
true,
);
});
it('should control radio buttons if the tree updates during render', () => {
@@ -1720,8 +1776,18 @@ describe('ReactDOMInput', () => {
) {
const el = originalCreateElement.apply(this, arguments);
let value = '';
let typeProp = '';
if (type === 'input') {
Object.defineProperty(el, 'type', {
get: function () {
return typeProp;
},
set: function (val) {
typeProp = String(val);
log.push('set property type');
},
});
Object.defineProperty(el, 'value', {
get: function () {
return value;
@@ -1751,10 +1817,10 @@ describe('ReactDOMInput', () => {
);
expect(log).toEqual([
'set attribute type',
'set attribute min',
'set attribute max',
'set attribute step',
'set property type',
'set property value',
]);
});
@@ -1810,6 +1876,14 @@ describe('ReactDOMInput', () => {
HTMLInputElement.prototype,
'value',
).set;
const getType = Object.getOwnPropertyDescriptor(
HTMLInputElement.prototype,
'type',
).get;
const setType = Object.getOwnPropertyDescriptor(
HTMLInputElement.prototype,
'type',
).set;
if (type === 'input') {
Object.defineProperty(el, 'defaultValue', {
get: function () {
@@ -1829,6 +1903,15 @@ describe('ReactDOMInput', () => {
setValue.call(this, val);
},
});
Object.defineProperty(el, 'type', {
get: function () {
return getType.call(this);
},
set: function (val) {
log.push(`node.type = ${strify(val)}`);
setType.call(this, val);
},
});
spyOnDevAndProd(el, 'setAttribute').mockImplementation(function (
name,
val,
@@ -1843,14 +1926,14 @@ describe('ReactDOMInput', () => {
if (disableInputAttributeSyncing) {
expect(log).toEqual([
'node.setAttribute("type", "date")',
'node.type = "date"',
'node.defaultValue = "1980-01-01"',
// TODO: it's possible this reintroduces the bug because we don't assign `value` at all.
// Need to check this on mobile Safari and Chrome.
]);
} else {
expect(log).toEqual([
'node.setAttribute("type", "date")',
'node.type = "date"',
// value must be assigned before defaultValue. This fixes an issue where the
// visually displayed value of date inputs disappears on mobile Safari and Chrome:
// https://github.com/facebook/react/issues/7233

20
packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js vendored
View File

@@ -12,7 +12,6 @@
let React;
let ReactDOM;
let ReactDOMClient;
let ReactFeatureFlags;
let Scheduler;
let act;
let waitForAll;
@@ -39,7 +38,6 @@ describe('ChangeEventPlugin', () => {
beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
// TODO pull this into helper method, reduce repetition.
// mock the browser APIs which are used in schedule:
// - calling 'window.postMessage' should actually fire postmessage handlers
@@ -100,13 +98,8 @@ describe('ChangeEventPlugin', () => {
node.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));
node.dispatchEvent(new Event('change', {bubbles: true, cancelable: true}));
if (ReactFeatureFlags.disableInputAttributeSyncing) {
// TODO: figure out why. This might be a bug.
expect(called).toBe(1);
} else {
// There should be no React change events because the value stayed the same.
expect(called).toBe(0);
}
// There should be no React change events because the value stayed the same.
expect(called).toBe(0);
});
it('should consider initial text value to be current (capture)', () => {
@@ -124,13 +117,8 @@ describe('ChangeEventPlugin', () => {
node.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));
node.dispatchEvent(new Event('change', {bubbles: true, cancelable: true}));
if (ReactFeatureFlags.disableInputAttributeSyncing) {
// TODO: figure out why. This might be a bug.
expect(called).toBe(1);
} else {
// There should be no React change events because the value stayed the same.
expect(called).toBe(0);
}
// There should be no React change events because the value stayed the same.
expect(called).toBe(0);
});
it('should not invoke a change event for textarea same value', () => {