@samitouri / QOS-React-1 / commits / 4f4c52a3c8

Fix controlled radios, maybe for real this time (#27443)

Fixes #26876 for real? In 18.2.0 (last stable), we set .checked unconditionally: https://github.com/facebook/react/blob/v18.2.0/packages/react-dom/src/client/ReactDOMInput.js#L129-L135 This is important because if we are updating two radios' checkedness from (false, true) to (true, false), we need to make sure that input2.checked is explicitly set to false, even though setting `input1.checked = true` already unchecks input2. I think this fix is not complete because there is no guarantee that all the inputs rerender at the same time? Hence the TODO. But in practice they usually would and I _think_ this is comparable to what we had before. Also treating function and symbol as false like we used to and like we do on initial mount.

Sophie Alpert committed Oct 2, 2023 at 11:38 UTC 4f4c52a3c8f9c8a2d8133c654841fee257c37249

3 files changed +88 -49

packages/react-dom-bindings/src/client/ReactDOMInput.js

+7 -2

@@ -175,8 +175,13 @@ export function updateInput(

175 }

176 }

177

178 - if (checked != null && node.checked !== !!checked) {

179 - node.checked = checked;

178 + if (checked != null) {

179 + // Important to set this even if it's not a change in order to update input

180 + // value tracking with radio buttons

181 + // TODO: Should really update input value tracking for the whole radio

182 + // button group in an effect or something (similar to #27024)

183 + node.checked =

184 + checked && typeof checked !== 'function' && typeof checked !== 'symbol';

185 }

186

187 if (

packages/react-dom/src/__tests__/ReactDOMComponent-test.js

+4 -47

@@ -1094,18 +1094,12 @@ describe('ReactDOMComponent', () => {

1094

1095 it('should not incur unnecessary DOM mutations for boolean properties', () => {

1096 const container = document.createElement('div');

1097 - function onChange() {

1098 - // noop

1099 - }

1100 - ReactDOM.render(

1101 - <input type="checkbox" onChange={onChange} checked={true} />,

1102 - container,

1103 - );

1097 + ReactDOM.render(<audio muted={true} />, container);

1098

1099 const node = container.firstChild;

1100 let nodeValue = true;

1101 const nodeValueSetter = jest.fn();

1108 - Object.defineProperty(node, 'checked', {

1102 + Object.defineProperty(node, 'muted', {

1103 get: function () {

1104 return nodeValue;

1105 },

@@ -1114,48 +1108,11 @@ describe('ReactDOMComponent', () => {

1108 }),

1109 });

1110

1117 - ReactDOM.render(

1118 - <input

1119 - type="checkbox"

1120 - onChange={onChange}

1121 - checked={true}

1122 - data-unrelated={true}

1123 - />,

1124 - container,

1125 - );

1126 - expect(nodeValueSetter).toHaveBeenCalledTimes(0);

1127 -

1128 - expect(() => {

1129 - ReactDOM.render(

1130 - <input type="checkbox" onChange={onChange} />,

1131 - container,

1132 - );

1133 - }).toErrorDev(

1134 - 'A component is changing a controlled input to be uncontrolled. This is likely caused by ' +

1135 - 'the value changing from a defined to undefined, which should not happen. Decide between ' +

1136 - 'using a controlled or uncontrolled input element for the lifetime of the component.',

1137 - );

1138 - // This leaves the current checked value in place, just like text inputs.

1111 + ReactDOM.render(<audio muted={true} data-unrelated="yes" />, container);

1112 expect(nodeValueSetter).toHaveBeenCalledTimes(0);

1113

1141 - expect(() => {

1142 - ReactDOM.render(

1143 - <input type="checkbox" onChange={onChange} checked={false} />,

1144 - container,

1145 - );

1146 - }).toErrorDev(

1147 - ' A component is changing an uncontrolled input to be controlled. This is likely caused by ' +

1148 - 'the value changing from undefined to a defined value, which should not happen. Decide between ' +

1149 - 'using a controlled or uncontrolled input element for the lifetime of the component.',

1150 - );

1151 -

1114 + ReactDOM.render(<audio muted={false} data-unrelated="ok" />, container);

1115 expect(nodeValueSetter).toHaveBeenCalledTimes(1);

1153 -

1154 - ReactDOM.render(

1155 - <input type="checkbox" onChange={onChange} checked={true} />,

1156 - container,

1157 - );

1158 - expect(nodeValueSetter).toHaveBeenCalledTimes(2);

1116 });

1117

1118 it('should ignore attribute list for elements with the "is" attribute', () => {

packages/react-dom/src/__tests__/ReactDOMInput-test.js

+77

@@ -1414,6 +1414,83 @@ describe('ReactDOMInput', () => {

1414 assertInputTrackingIsCurrent(container);

1415 });

1416

1417 + it('should control radio buttons if the tree updates during render (case 2; #26876)', () => {

1418 + let thunk = null;

1419 + function App() {

1420 + const [disabled, setDisabled] = React.useState(false);

1421 + const [value, setValue] = React.useState('one');

1422 + function handleChange(e) {

1423 + setDisabled(true);

1424 + // Pretend this is in a setTimeout or something

1425 + thunk = () => {

1426 + setDisabled(false);

1427 + setValue(e.target.value);

1428 + };

1429 + }

1430 + return (

1431 + <>

1432 + <input

1433 + type="radio"

1434 + name="fruit"

1435 + value="one"

1436 + checked={value === 'one'}

1437 + onChange={handleChange}

1438 + disabled={disabled}

1439 + />

1440 + <input

1441 + type="radio"

1442 + name="fruit"

1443 + value="two"

1444 + checked={value === 'two'}

1445 + onChange={handleChange}

1446 + disabled={disabled}

1447 + />

1448 + </>

1449 + );

1450 + }

1451 + ReactDOM.render(<App />, container);

1452 + const [one, two] = container.querySelectorAll('input');

1453 + expect(one.checked).toBe(true);

1454 + expect(two.checked).toBe(false);

1455 + expect(isCheckedDirty(one)).toBe(true);

1456 + expect(isCheckedDirty(two)).toBe(true);

1457 + assertInputTrackingIsCurrent(container);

1458 +

1459 + // Click two

1460 + setUntrackedChecked.call(two, true);

1461 + dispatchEventOnNode(two, 'click');

1462 + expect(one.checked).toBe(true);

1463 + expect(two.checked).toBe(false);

1464 + expect(isCheckedDirty(one)).toBe(true);

1465 + expect(isCheckedDirty(two)).toBe(true);

1466 + assertInputTrackingIsCurrent(container);

1467 +

1468 + // After a delay...

1469 + ReactDOM.unstable_batchedUpdates(thunk);

1470 + expect(one.checked).toBe(false);

1471 + expect(two.checked).toBe(true);

1472 + expect(isCheckedDirty(one)).toBe(true);

1473 + expect(isCheckedDirty(two)).toBe(true);

1474 + assertInputTrackingIsCurrent(container);

1475 +

1476 + // Click back to one

1477 + setUntrackedChecked.call(one, true);

1478 + dispatchEventOnNode(one, 'click');

1479 + expect(one.checked).toBe(false);

1480 + expect(two.checked).toBe(true);

1481 + expect(isCheckedDirty(one)).toBe(true);

1482 + expect(isCheckedDirty(two)).toBe(true);

1483 + assertInputTrackingIsCurrent(container);

1484 +

1485 + // After a delay...

1486 + ReactDOM.unstable_batchedUpdates(thunk);

1487 + expect(one.checked).toBe(true);

1488 + expect(two.checked).toBe(false);

1489 + expect(isCheckedDirty(one)).toBe(true);

1490 + expect(isCheckedDirty(two)).toBe(true);

1491 + assertInputTrackingIsCurrent(container);

1492 + });

1493 +

1494 it('should warn with value and no onChange handler and readOnly specified', () => {

1495 ReactDOM.render(

1496 <input type="text" value="zoink" readOnly={true} />,