Skip to content

Commit

Permalink
Fix controlled radios, maybe for real this time
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sophiebits committed Sep 30, 2023
1 parent a6ed60a commit 20a93f7
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 2 deletions.
9 changes: 7 additions & 2 deletions packages/react-dom-bindings/src/client/ReactDOMInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,13 @@ export function updateInput(
}
}

if (checked != null && node.checked !== !!checked) {
node.checked = checked;
if (checked != null) {
// Important to set this even if it's not a change in order to update input
// value tracking with radio buttons
// TODO: Should really update input value tracking for the whole radio
// button group in an effect or something (similar to #27024)
node.checked =
checked && typeof checked !== 'function' && typeof checked !== 'symbol';
}

if (
Expand Down
77 changes: 77 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1414,6 +1414,83 @@ describe('ReactDOMInput', () => {
assertInputTrackingIsCurrent(container);
});

it('should control radio buttons if the tree updates during render (case 2; #26876)', () => {
let thunk = null;
function App() {
const [disabled, setDisabled] = React.useState(false);
const [value, setValue] = React.useState('one');
function handleChange(e) {
setDisabled(true);
// Pretend this is in a setTimeout or something
thunk = () => {
setDisabled(false);
setValue(e.target.value);
};
}
return (
<>
<input
type="radio"
name="fruit"
value="one"
checked={value === 'one'}
onChange={handleChange}
disabled={disabled}
/>
<input
type="radio"
name="fruit"
value="two"
checked={value === 'two'}
onChange={handleChange}
disabled={disabled}
/>
</>
);
}
ReactDOM.render(<App />, container);
const [one, two] = container.querySelectorAll('input');
expect(one.checked).toBe(true);
expect(two.checked).toBe(false);
expect(isCheckedDirty(one)).toBe(true);
expect(isCheckedDirty(two)).toBe(true);
assertInputTrackingIsCurrent(container);

// Click two
setUntrackedChecked.call(two, true);
dispatchEventOnNode(two, 'click');
expect(one.checked).toBe(true);
expect(two.checked).toBe(false);
expect(isCheckedDirty(one)).toBe(true);
expect(isCheckedDirty(two)).toBe(true);
assertInputTrackingIsCurrent(container);

// After a delay...
ReactDOM.unstable_batchedUpdates(thunk);
expect(one.checked).toBe(false);
expect(two.checked).toBe(true);
expect(isCheckedDirty(one)).toBe(true);
expect(isCheckedDirty(two)).toBe(true);
assertInputTrackingIsCurrent(container);

// Click back to one
setUntrackedChecked.call(one, true);
dispatchEventOnNode(one, 'click');
expect(one.checked).toBe(false);
expect(two.checked).toBe(true);
expect(isCheckedDirty(one)).toBe(true);
expect(isCheckedDirty(two)).toBe(true);
assertInputTrackingIsCurrent(container);

// After a delay...
ReactDOM.unstable_batchedUpdates(thunk);
expect(one.checked).toBe(true);
expect(two.checked).toBe(false);
expect(isCheckedDirty(one)).toBe(true);
expect(isCheckedDirty(two)).toBe(true);
assertInputTrackingIsCurrent(container);
});

it('should warn with value and no onChange handler and readOnly specified', () => {
ReactDOM.render(
<input type="text" value="zoink" readOnly={true} />,
Expand Down

0 comments on commit 20a93f7

Please sign in to comment.