From 1f248bdd7199979b050e4040ceecfe72dd977fd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 19 Apr 2023 11:46:29 -0400 Subject: [PATCH] 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 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 --- .../src/client/ReactDOMComponent.js | 126 ++---------------- .../src/client/ReactDOMInput.js | 65 ++++++++- .../src/__tests__/ReactDOMComponent-test.js | 7 +- .../src/__tests__/ReactDOMInput-test.js | 91 ++++++++++++- .../__tests__/ChangeEventPlugin-test.js | 20 +-- 5 files changed, 169 insertions(+), 140 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 098285224c..e37c61a35d 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -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; } diff --git a/packages/react-dom-bindings/src/client/ReactDOMInput.js b/packages/react-dom-bindings/src/client/ReactDOMInput.js index 4e8f9b32aa..2096238d76 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMInput.js +++ b/packages/react-dom-bindings/src/client/ReactDOMInput.js @@ -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, ); } } diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js index a195bd67bc..f5493c059f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js @@ -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( , container, ); - expect(nodeValueSetter).toHaveBeenCalledTimes(3); + expect(nodeValueSetter).toHaveBeenCalledTimes(2); }); it('should ignore attribute list for elements with the "is" attribute', () => { diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 0c736174e4..0f02d928c7 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -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( +
+ {}} + /> + {}} + /> +
, + 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( +
+ {}} + /> + {}} + /> +
, + 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 diff --git a/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js b/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js index f4f75bfd52..ae27973a0c 100644 --- a/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js +++ b/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js @@ -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', () => {