From 70fce26ac4e27fe68e2fedd090b780f438ec8dd5 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Thu, 13 Apr 2023 21:52:41 -0700 Subject: [PATCH] Fix input tracking bug 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. --- .../src/client/ReactDOMComponent.js | 31 +++++++----- .../src/client/ReactDOMInput.js | 49 ++++++++++--------- .../__tests__/DOMPropertyOperations-test.js | 6 ++- .../src/__tests__/ReactDOMInput-test.js | 34 ++++++++++++- 4 files changed, 82 insertions(+), 38 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 693aeffd7c4fe..e40d186770993 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -1297,32 +1297,36 @@ export function updateProperties( let type = null; let value = null; let defaultValue = null; + let oldDefaultValue = null; let checked = null; let defaultChecked = null; for (const propKey in lastProps) { const lastProp = lastProps[propKey]; - if ( - lastProps.hasOwnProperty(propKey) && - lastProp != null && - !nextProps.hasOwnProperty(propKey) - ) { + if (lastProps.hasOwnProperty(propKey) && lastProp != null) { switch (propKey) { case 'checked': { - const checkedValue = nextProps.defaultChecked; - const inputElement: HTMLInputElement = (domElement: any); - inputElement.checked = - !!checkedValue && - typeof checkedValue !== 'function' && - checkedValue !== 'symbol'; + if (!nextProps.hasOwnProperty(propKey)) { + const checkedValue = nextProps.defaultChecked; + const inputElement: HTMLInputElement = (domElement: any); + inputElement.checked = + !!checkedValue && + typeof checkedValue !== 'function' && + checkedValue !== 'symbol'; + } break; } case 'value': { // This is handled by updateWrapper below. break; } + case 'defaultValue': { + oldDefaultValue = lastProp; + } // defaultChecked and defaultValue are ignored by setProp + // Fallthrough default: { - setProp(domElement, tag, propKey, null, nextProps, lastProp); + if (!nextProps.hasOwnProperty(propKey)) + setProp(domElement, tag, propKey, null, nextProps, lastProp); } } } @@ -1473,6 +1477,7 @@ export function updateProperties( domElement, value, defaultValue, + oldDefaultValue, checked, defaultChecked, type, @@ -1809,6 +1814,7 @@ export function updatePropertiesWithDiff( const type = nextProps.type; const value = nextProps.value; const defaultValue = nextProps.defaultValue; + const oldDefaultValue = lastProps.defaultValue; const checked = nextProps.checked; const defaultChecked = nextProps.defaultChecked; for (let i = 0; i < updatePayload.length; i += 2) { @@ -1934,6 +1940,7 @@ export function updatePropertiesWithDiff( domElement, value, defaultValue, + oldDefaultValue, checked, defaultChecked, type, diff --git a/packages/react-dom-bindings/src/client/ReactDOMInput.js b/packages/react-dom-bindings/src/client/ReactDOMInput.js index 9bacf8e046314..69b646484a34b 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMInput.js +++ b/packages/react-dom-bindings/src/client/ReactDOMInput.js @@ -85,19 +85,41 @@ export function updateInput( element: Element, value: ?string, defaultValue: ?string, + oldDefaultValue: ?string, checked: ?boolean, defaultChecked: ?boolean, type: ?string, ) { const node: HTMLInputElement = (element: any); + if (value != null) { + if (type === 'number') { + if ( + // $FlowFixMe[incompatible-type] + (value === 0 && node.value === '') || + // We explicitly want to coerce to number here if possible. + // eslint-disable-next-line + node.value != (value: any) + ) { + node.value = toString(getToStringValue(value)); + } + } else if (node.value !== toString(getToStringValue(value))) { + node.value = toString(getToStringValue(value)); + } + } else if (type === 'submit' || type === 'reset') { + // Submit/reset inputs need the attribute removed completely to avoid + // blank-text buttons. + node.removeAttribute('value'); + return; + } + if (disableInputAttributeSyncing) { // When not syncing the value attribute, React only assigns a new value // whenever the defaultValue React prop has changed. When not present, // React does nothing if (defaultValue != null) { setDefaultValue(node, type, getToStringValue(defaultValue)); - } else { + } else if (oldDefaultValue != null) { node.removeAttribute('value'); } } else { @@ -110,7 +132,7 @@ export function updateInput( setDefaultValue(node, type, getToStringValue(value)); } else if (defaultValue != null) { setDefaultValue(node, type, getToStringValue(defaultValue)); - } else { + } else if (oldDefaultValue != null) { node.removeAttribute('value'); } } @@ -135,27 +157,6 @@ export function updateInput( if (checked != null && node.checked !== !!checked) { node.checked = checked; } - - if (value != null) { - if (type === 'number') { - if ( - // $FlowFixMe[incompatible-type] - (value === 0 && node.value === '') || - // We explicitly want to coerce to number here if possible. - // eslint-disable-next-line - node.value != (value: any) - ) { - node.value = toString(getToStringValue(value)); - } - } else if (node.value !== toString(getToStringValue(value))) { - node.value = toString(getToStringValue(value)); - } - } else if (type === 'submit' || type === 'reset') { - // Submit/reset inputs need the attribute removed completely to avoid - // blank-text buttons. - node.removeAttribute('value'); - return; - } } export function initInput( @@ -286,6 +287,7 @@ export function restoreControlledInputState(element: Element, props: Object) { rootNode, props.value, props.defaultValue, + props.defaultValue, props.checked, props.defaultChecked, props.type, @@ -341,6 +343,7 @@ export function restoreControlledInputState(element: Element, props: Object) { otherNode, otherProps.value, otherProps.defaultValue, + otherProps.defaultValue, otherProps.checked, otherProps.defaultChecked, otherProps.type, diff --git a/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js b/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js index 67d6f7bd465ab..bf523c4494e81 100644 --- a/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js +++ b/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js @@ -1166,7 +1166,11 @@ describe('DOMPropertyOperations', () => { ).toErrorDev( 'A component is changing a controlled input to be uncontrolled', ); - expect(container.firstChild.hasAttribute('value')).toBe(false); + if (disableInputAttributeSyncing) { + expect(container.firstChild.hasAttribute('value')).toBe(false); + } else { + expect(container.firstChild.getAttribute('value')).toBe('foo'); + } expect(container.firstChild.value).toBe('foo'); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 47fca9522fac8..17a4223b6271e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1952,7 +1952,11 @@ describe('ReactDOMInput', () => { expect(renderInputWithStringThenWithUndefined).toErrorDev( 'A component is changing a controlled input to be uncontrolled.', ); - expect(input.getAttribute('value')).toBe(null); + if (disableInputAttributeSyncing) { + expect(input.getAttribute('value')).toBe(null); + } else { + expect(input.getAttribute('value')).toBe('latest'); + } }); it('preserves the value property', () => { @@ -1998,7 +2002,11 @@ describe('ReactDOMInput', () => { 'or `undefined` for uncontrolled components.', 'A component is changing a controlled input to be uncontrolled.', ]); - expect(input.hasAttribute('value')).toBe(false); + if (disableInputAttributeSyncing) { + expect(input.getAttribute('value')).toBe(null); + } else { + expect(input.getAttribute('value')).toBe('latest'); + } }); it('preserves the value property', () => { @@ -2183,4 +2191,26 @@ describe('ReactDOMInput', () => { ReactDOM.render(, container); expect(node.defaultValue).toBe(''); }); + + it('should notice input changes when reverting back to original value', () => { + const log = []; + function onChange(e) { + log.push(e.target.value); + } + ReactDOM.render( + , + container, + ); + ReactDOM.render( + , + container, + ); + + const node = container.firstChild; + setUntrackedValue.call(node, ''); + dispatchEventOnNode(node, 'input'); + + expect(log.splice(0, log.length)).toEqual(['']); + expect(node.value).toBe('a'); + }); });