Skip to content

Commit

Permalink
Fix input tracking bug (#26627)
Browse files Browse the repository at this point in the history
In
2019ddc,
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 #26626.

DiffTrain build for [b433c37](b433c37)
  • Loading branch information
sophiebits committed Apr 18, 2023
1 parent b679adf commit db190e0
Show file tree
Hide file tree
Showing 15 changed files with 807 additions and 573 deletions.
2 changes: 1 addition & 1 deletion compiled/facebook-www/REVISION
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2bfe4b246f58d1f8d357f984fba9a8aa1fa79c73
b433c379d55d9684945217c7d375de1082a1abb8
2 changes: 1 addition & 1 deletion compiled/facebook-www/React-dev.modern.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ if (
}
"use strict";

var ReactVersion = "18.3.0-www-modern-40ddfb33";
var ReactVersion = "18.3.0-www-modern-edf301ed";

// ATTENTION
// When adding new symbols to this file,
Expand Down
2 changes: 1 addition & 1 deletion compiled/facebook-www/ReactART-dev.classic.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
return self;
}

var ReactVersion = "18.3.0-www-classic-09c9197a";
var ReactVersion = "18.3.0-www-classic-145c42eb";

var LegacyRoot = 0;
var ConcurrentRoot = 1;
Expand Down
4 changes: 2 additions & 2 deletions compiled/facebook-www/ReactART-prod.modern.js
Original file line number Diff line number Diff line change
Expand Up @@ -9637,7 +9637,7 @@ var slice = Array.prototype.slice,
return null;
},
bundleType: 0,
version: "18.3.0-www-modern-342727fc",
version: "18.3.0-www-modern-294eaa58",
rendererPackageName: "react-art"
};
var internals$jscomp$inline_1317 = {
Expand Down Expand Up @@ -9668,7 +9668,7 @@ var internals$jscomp$inline_1317 = {
scheduleRoot: null,
setRefreshHandler: null,
getCurrentFiber: null,
reconcilerVersion: "18.3.0-www-modern-342727fc"
reconcilerVersion: "18.3.0-www-modern-294eaa58"
};
if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
var hook$jscomp$inline_1318 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
Expand Down
82 changes: 47 additions & 35 deletions compiled/facebook-www/ReactDOM-dev.classic.js
Original file line number Diff line number Diff line change
Expand Up @@ -3779,19 +3779,40 @@ function updateInput(
element,
value,
defaultValue,
lastDefaultValue,
checked,
defaultChecked,
type
) {
var node = element;

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
) {
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 (lastDefaultValue != null) {
node.removeAttribute("value");
}
} else {
Expand All @@ -3804,7 +3825,7 @@ function updateInput(
setDefaultValue(node, type, getToStringValue(value));
} else if (defaultValue != null) {
setDefaultValue(node, type, getToStringValue(defaultValue));
} else {
} else if (lastDefaultValue != null) {
node.removeAttribute("value");
}
}
Expand All @@ -3829,26 +3850,6 @@ 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
) {
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;
}
}
function initInput(
element,
Expand Down Expand Up @@ -3974,6 +3975,7 @@ function restoreControlledInputState(element, props) {
rootNode,
props.value,
props.defaultValue,
props.defaultValue,
props.checked,
props.defaultChecked,
props.type
Expand Down Expand Up @@ -4029,6 +4031,7 @@ function restoreControlledInputState(element, props) {
otherNode,
otherProps.value,
otherProps.defaultValue,
otherProps.defaultValue,
otherProps.checked,
otherProps.defaultChecked,
otherProps.type
Expand Down Expand Up @@ -33736,7 +33739,7 @@ function createFiberRoot(
return root;
}

var ReactVersion = "18.3.0-www-classic-ffb509af";
var ReactVersion = "18.3.0-www-classic-4a39e57d";

function createPortal$1(
children,
Expand Down Expand Up @@ -39353,36 +39356,42 @@ function updateProperties(domElement, tag, lastProps, nextProps) {
var type = null;
var value = null;
var defaultValue = null;
var lastDefaultValue = null;
var checked = null;
var defaultChecked = null;

for (var propKey in lastProps) {
var lastProp = lastProps[propKey];

if (
lastProps.hasOwnProperty(propKey) &&
lastProp != null &&
!nextProps.hasOwnProperty(propKey)
) {
if (lastProps.hasOwnProperty(propKey) && lastProp != null) {
switch (propKey) {
case "checked": {
var checkedValue = nextProps.defaultChecked;
var inputElement = domElement;
inputElement.checked =
!!checkedValue &&
typeof checkedValue !== "function" &&
checkedValue !== "symbol";
if (!nextProps.hasOwnProperty(propKey)) {
var checkedValue = nextProps.defaultChecked;
var inputElement = domElement;
inputElement.checked =
!!checkedValue &&
typeof checkedValue !== "function" &&
checkedValue !== "symbol";
}

break;
}

case "value": {
// This is handled by updateWrapper below.
break;
}

case "defaultValue": {
lastDefaultValue = 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);
}
}
}
Expand Down Expand Up @@ -39551,6 +39560,7 @@ function updateProperties(domElement, tag, lastProps, nextProps) {
domElement,
value,
defaultValue,
lastDefaultValue,
checked,
defaultChecked,
type
Expand Down Expand Up @@ -39952,6 +39962,7 @@ function updatePropertiesWithDiff(
var type = nextProps.type;
var value = nextProps.value;
var defaultValue = nextProps.defaultValue;
var lastDefaultValue = lastProps.defaultValue;
var checked = nextProps.checked;
var defaultChecked = nextProps.defaultChecked;

Expand Down Expand Up @@ -40092,6 +40103,7 @@ function updatePropertiesWithDiff(
domElement,
value,
defaultValue,
lastDefaultValue,
checked,
defaultChecked,
type
Expand Down
Loading

0 comments on commit db190e0

Please sign in to comment.