Update tracked value after resetting radio group (#27394)
Fixes #26876, I think. Review each commit separately (all assertions pass in main already, except the last assertInputTrackingIsClean in "should control radio buttons"). I'm actually a little confused on two things here: * All the isCheckedDirty assertions are true. But I don't think we set .checked unconditionally? So how does this happen? * https://github.com/facebook/react/issues/26876#issuecomment-1611662862 claims that https://github.com/facebook/react/compare/d962f35ca...1f248bdd7 contains the faulty change, but it doesn't appear to change the restoration logic that I've touched here. (One difference outside restoration is that updateProperties did previously set `.checked` when `nextProp !== lastProp` whereas the new logic in updateInput is to set it when `node.checked !== !!checked`.) But it seems to me like we need this call here anyway, and if it fixes it then it fixes it? I think technically speaking we probably should do all the updateInput() calls and then all the updateValueIfChanged() calls—in particular I think if clicking A changed the checked radio button from B to C then the code as I have it would be incorrect, but that also seems unlikely so idk whether to care. cc @zhengjitf @Luk-z who did some investigation on the original issue
Sophie Alpert committed
Sep 20, 2023 at 21:57 UTC
3c27178a2f2c74f14d90613028e3929e1f06d830
2 files changed
+86
-16
packages/react-dom-bindings/src/client/ReactDOMInput.js
+10
-4
@@ -388,10 +388,6 @@ export function restoreControlledInputState(element: Element, props: Object) {
388
);
389
}
390
391
- // We need update the tracked value on the named cousin since the value
392
- // was changed but the input saw no event or value set
393
- updateValueIfChanged(otherNode);
394
-
391
// If this is a controlled radio button group, forcing the input that
392
// was previously checked to update will cause it to be come re-checked
393
// as appropriate.
@@ -406,6 +402,16 @@ export function restoreControlledInputState(element: Element, props: Object) {
402
otherProps.name,
403
);
404
}
405
+
406
+ // If any updateInput() call set .checked to true, an input in this group
407
+ // (often, `rootNode` itself) may have become unchecked
408
+ for (let i = 0; i < group.length; i++) {
409
+ const otherNode = ((group[i]: any): HTMLInputElement);
410
+ if (otherNode.form !== rootNode.form) {
411
+ continue;
412
+ }
413
+ updateValueIfChanged(otherNode);
414
+ }
415
}
416
}
417
packages/react-dom/src/__tests__/ReactDOMInput-test.js
+76
-12
@@ -36,6 +36,43 @@ describe('ReactDOMInput', () => {
36
return copy.value === node.value;
37
}
38
39
+ function isCheckedDirty(node) {
40
+ // Return the "dirty checked flag" as defined in the HTML spec.
41
+ if (node.checked !== node.defaultChecked) {
42
+ return true;
43
+ }
44
+ const copy = node.cloneNode();
45
+ copy.type = 'checkbox';
46
+ copy.defaultChecked = !copy.defaultChecked;
47
+ return copy.checked === node.checked;
48
+ }
49
+
50
+ function getTrackedAndCurrentInputValue(elem: HTMLElement): [mixed, mixed] {
51
+ const tracker = elem._valueTracker;
52
+ if (!tracker) {
53
+ throw new Error('No input tracker');
54
+ }
55
+ return [
56
+ tracker.getValue(),
57
+ elem.nodeName === 'INPUT' &&
58
+ (elem.type === 'checkbox' || elem.type === 'radio')
59
+ ? String(elem.checked)
60
+ : elem.value,
61
+ ];
62
+ }
63
+
64
+ function assertInputTrackingIsCurrent(parent) {
65
+ parent.querySelectorAll('input, textarea, select').forEach(input => {
66
+ const [trackedValue, currentValue] =
67
+ getTrackedAndCurrentInputValue(input);
68
+ if (trackedValue !== currentValue) {
69
+ throw new Error(
70
+ `Input ${input.outerHTML} is currently ${currentValue} but tracker thinks it's ${trackedValue}`,
71
+ );
72
+ }
73
+ });
74
+ }
75
+
76
beforeEach(() => {
77
jest.resetModules();
78
@@ -1119,6 +1156,7 @@ describe('ReactDOMInput', () => {
1156
name="fruit"
1157
checked={true}
1158
onChange={emptyFunction}
1159
+ data-which="a"
1160
/>
1161
A
1162
<input
@@ -1126,6 +1164,7 @@ describe('ReactDOMInput', () => {
1164
type="radio"
1165
name="fruit"
1166
onChange={emptyFunction}
1167
+ data-which="b"
1168
/>
1169
B
1170
<form>
@@ -1135,6 +1174,7 @@ describe('ReactDOMInput', () => {
1174
name="fruit"
1175
defaultChecked={true}
1176
onChange={emptyFunction}
1177
+ data-which="c"
1178
/>
1179
</form>
1180
</div>
@@ -1162,6 +1202,11 @@ describe('ReactDOMInput', () => {
1202
expect(cNode.hasAttribute('checked')).toBe(true);
1203
}
1204
1205
+ expect(isCheckedDirty(aNode)).toBe(true);
1206
+ expect(isCheckedDirty(bNode)).toBe(true);
1207
+ expect(isCheckedDirty(cNode)).toBe(true);
1208
+ assertInputTrackingIsCurrent(container);
1209
+
1210
setUntrackedChecked.call(bNode, true);
1211
expect(aNode.checked).toBe(false);
1212
expect(cNode.checked).toBe(true);
@@ -1183,6 +1228,11 @@ describe('ReactDOMInput', () => {
1228
// The original state should have been restored
1229
expect(aNode.checked).toBe(true);
1230
expect(cNode.checked).toBe(true);
1231
+
1232
+ expect(isCheckedDirty(aNode)).toBe(true);
1233
+ expect(isCheckedDirty(bNode)).toBe(true);
1234
+ expect(isCheckedDirty(cNode)).toBe(true);
1235
+ assertInputTrackingIsCurrent(container);
1236
});
1237
1238
it('should check the correct radio when the selected name moves', () => {
@@ -1219,11 +1269,15 @@ describe('ReactDOMInput', () => {
1269
const stub = ReactDOM.render(<App />, container);
1270
const buttonNode = ReactDOM.findDOMNode(stub).childNodes[0];
1271
const firstRadioNode = ReactDOM.findDOMNode(stub).childNodes[1];
1272
+ expect(isCheckedDirty(firstRadioNode)).toBe(true);
1273
expect(firstRadioNode.checked).toBe(false);
1274
+ assertInputTrackingIsCurrent(container);
1275
dispatchEventOnNode(buttonNode, 'click');
1276
expect(firstRadioNode.checked).toBe(true);
1277
+ assertInputTrackingIsCurrent(container);
1278
dispatchEventOnNode(buttonNode, 'click');
1279
expect(firstRadioNode.checked).toBe(false);
1280
+ assertInputTrackingIsCurrent(container);
1281
});
1282
1283
it("shouldn't get tricked by changing radio names, part 2", () => {
@@ -1246,12 +1300,13 @@ describe('ReactDOMInput', () => {
1300
</div>,
1301
container,
1302
);
1249
- expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
1250
- true,
1251
- );
1252
- expect(container.querySelector('input[name="a"][value="2"]').checked).toBe(
1253
- false,
1254
- );
1303
+ const one = container.querySelector('input[name="a"][value="1"]');
1304
+ const two = container.querySelector('input[name="a"][value="2"]');
1305
+ expect(one.checked).toBe(true);
1306
+ expect(two.checked).toBe(false);
1307
+ expect(isCheckedDirty(one)).toBe(true);
1308
+ expect(isCheckedDirty(two)).toBe(true);
1309
+ assertInputTrackingIsCurrent(container);
1310
1311
ReactDOM.render(
1312
<div>
@@ -1272,12 +1327,11 @@ describe('ReactDOMInput', () => {
1327
</div>,
1328
container,
1329
);
1275
- expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(
1276
- true,
1277
- );
1278
- expect(container.querySelector('input[name="b"][value="2"]').checked).toBe(
1279
- true,
1280
- );
1330
+ expect(one.checked).toBe(true);
1331
+ expect(two.checked).toBe(true);
1332
+ expect(isCheckedDirty(one)).toBe(true);
1333
+ expect(isCheckedDirty(two)).toBe(true);
1334
+ assertInputTrackingIsCurrent(container);
1335
});
1336
1337
it('should control radio buttons if the tree updates during render', () => {
@@ -1339,6 +1393,9 @@ describe('ReactDOMInput', () => {
1393
1394
expect(aNode.checked).toBe(false);
1395
expect(bNode.checked).toBe(true);
1396
+ expect(isCheckedDirty(aNode)).toBe(true);
1397
+ expect(isCheckedDirty(bNode)).toBe(true);
1398
+ assertInputTrackingIsCurrent(container);
1399
1400
setUntrackedChecked.call(aNode, true);
1401
// This next line isn't necessary in a proper browser environment, but
@@ -1352,6 +1409,9 @@ describe('ReactDOMInput', () => {
1409
// The original state should have been restored
1410
expect(aNode.checked).toBe(false);
1411
expect(bNode.checked).toBe(true);
1412
+ expect(isCheckedDirty(aNode)).toBe(true);
1413
+ expect(isCheckedDirty(bNode)).toBe(true);
1414
+ assertInputTrackingIsCurrent(container);
1415
});
1416
1417
it('should warn with value and no onChange handler and readOnly specified', () => {
@@ -1734,6 +1794,8 @@ describe('ReactDOMInput', () => {
1794
<input type="radio" checked={false} onChange={() => null} />,
1795
container,
1796
);
1797
+ const input = container.querySelector('input');
1798
+ expect(isCheckedDirty(input)).toBe(true);
1799
ReactDOM.render(
1800
<input
1801
type="radio"
@@ -1744,6 +1806,8 @@ describe('ReactDOMInput', () => {
1806
/>,
1807
container,
1808
);
1809
+ expect(isCheckedDirty(input)).toBe(true);
1810
+ assertInputTrackingIsCurrent(container);
1811
});
1812
1813
it('should warn if radio checked false changes to become uncontrolled', () => {