Skip to content

Commit

Permalink
Fix checkbox and radio hydration
Browse files Browse the repository at this point in the history
Fixes whatever part of #26876 and vercel/next.js#49499 that #27394 didn't fix, probably.

From manual tests I believe this behavior brings us back to parity with latest stable release (18.2.0). It's awkward that we keep the user's state even for controlled inputs.

Previously the .defaultChecked assignment done in updateInput() was changing the actual checkedness because the dirty flag wasn't getting set, meaning that hydrating could change which radio button is checked, even in the absence of user interaction! Now we go back to always detaching again.
  • Loading branch information
sophiebits committed Sep 21, 2023
1 parent 26daa54 commit aadbb8a
Show file tree
Hide file tree
Showing 2 changed files with 167 additions and 6 deletions.
10 changes: 4 additions & 6 deletions packages/react-dom-bindings/src/client/ReactDOMInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,12 +292,10 @@ export function initInput(
typeof checkedOrDefault !== 'symbol' &&
!!checkedOrDefault;

// The checked property never gets assigned. It must be manually set.
// We don't want to do this when hydrating so that existing user input isn't
// modified
// TODO: I'm pretty sure this is a bug because initialValueTracking won't be
// correct for the hydration case then.
if (!isHydrating) {
if (isHydrating) {
// Detach .checked from .defaultChecked but leave user input alone
node.checked = node.checked;
} else {
node.checked = !!initialChecked;
}

Expand Down
163 changes: 163 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ function emptyFunction() {}
describe('ReactDOMInput', () => {
let React;
let ReactDOM;
let ReactDOMClient;
let ReactDOMServer;
let Scheduler;
let act;
let assertLog;
let setUntrackedValue;
let setUntrackedChecked;
let container;
Expand Down Expand Up @@ -87,7 +91,11 @@ describe('ReactDOMInput', () => {

React = require('react');
ReactDOM = require('react-dom');
ReactDOMClient = require('react-dom/client');
ReactDOMServer = require('react-dom/server');
Scheduler = require('scheduler');
act = require('internal-test-utils').act;
assertLog = require('internal-test-utils').assertLog;

container = document.createElement('div');
document.body.appendChild(container);
Expand Down Expand Up @@ -1235,6 +1243,161 @@ describe('ReactDOMInput', () => {
assertInputTrackingIsCurrent(container);
});

it('should hydrate controlled radio buttons', async () => {
function App() {
let [current, setCurrent] = React.useState('a');
return (
<>
<input
type="radio"
name="fruit"
checked={current === 'a'}
onChange={() => {
Scheduler.log('click a');
setCurrent('a');
}}
/>
<input
type="radio"
name="fruit"
checked={current === 'b'}
onChange={() => {
Scheduler.log('click b');
setCurrent('b');
}}
/>
<input
type="radio"
name="fruit"
checked={current === 'c'}
onChange={() => {
Scheduler.log('click c');
// Let's say the user can't pick C
}}
/>
</>
);
}
const html = ReactDOMServer.renderToString(<App />);
container.innerHTML = html;
const [a, b, c] = container.querySelectorAll('input');
expect(a.checked).toBe(true);
expect(b.checked).toBe(false);
expect(c.checked).toBe(false);
expect(isCheckedDirty(a)).toBe(false);
expect(isCheckedDirty(b)).toBe(false);
expect(isCheckedDirty(c)).toBe(false);

// Click on B before hydrating
b.checked = true;
expect(isCheckedDirty(a)).toBe(true);
expect(isCheckedDirty(b)).toBe(true);
expect(isCheckedDirty(c)).toBe(false);

await act(async () => {
ReactDOMClient.hydrateRoot(container, <App />);
});

// Currently, we don't fire onChange when hydrating
assertLog([]);
// Strangely, we leave `b` checked even though we rendered A with
// checked={true} and B with checked={false}. Arguably this is a bug.
expect(a.checked).toBe(false);
expect(b.checked).toBe(true);
expect(c.checked).toBe(false);
expect(isCheckedDirty(a)).toBe(true);
expect(isCheckedDirty(b)).toBe(true);
expect(isCheckedDirty(c)).toBe(true);
assertInputTrackingIsCurrent(container);

// If we click on C now though...
await act(async () => {
setUntrackedChecked.call(c, true);
dispatchEventOnNode(c, 'click');
});

// then since C's onClick doesn't set state, A becomes rechecked.
assertLog(['click c']);
expect(a.checked).toBe(true);
expect(b.checked).toBe(false);
expect(c.checked).toBe(false);
expect(isCheckedDirty(a)).toBe(true);
expect(isCheckedDirty(b)).toBe(true);
expect(isCheckedDirty(c)).toBe(true);
assertInputTrackingIsCurrent(container);
});

it('should hydrate uncontrolled radio buttons', async () => {
function App() {
return (
<>
<input
type="radio"
name="fruit"
defaultChecked={true}
onChange={() => Scheduler.log('click a')}
/>
<input
type="radio"
name="fruit"
defaultChecked={false}
onChange={() => Scheduler.log('click b')}
/>
<input
type="radio"
name="fruit"
defaultChecked={false}
onChange={() => Scheduler.log('click c')}
/>
</>
);
}
const html = ReactDOMServer.renderToString(<App />);
container.innerHTML = html;
const [a, b, c] = container.querySelectorAll('input');
expect(a.checked).toBe(true);
expect(b.checked).toBe(false);
expect(c.checked).toBe(false);
expect(isCheckedDirty(a)).toBe(false);
expect(isCheckedDirty(b)).toBe(false);
expect(isCheckedDirty(c)).toBe(false);

// Click on B before hydrating
b.checked = true;
expect(isCheckedDirty(a)).toBe(true);
expect(isCheckedDirty(b)).toBe(true);
expect(isCheckedDirty(c)).toBe(false);

await act(async () => {
ReactDOMClient.hydrateRoot(container, <App />);
});

// Currently, we don't fire onChange when hydrating
assertLog([]);
expect(a.checked).toBe(false);
expect(b.checked).toBe(true);
expect(c.checked).toBe(false);
expect(isCheckedDirty(a)).toBe(true);
expect(isCheckedDirty(b)).toBe(true);
expect(isCheckedDirty(c)).toBe(true);
assertInputTrackingIsCurrent(container);

// Click back to A
await act(async () => {
setUntrackedChecked.call(a, true);
dispatchEventOnNode(a, 'click');
});

assertLog(['click a']);
expect(a.checked).toBe(true);
expect(b.checked).toBe(false);
expect(c.checked).toBe(false);
expect(isCheckedDirty(a)).toBe(true);
expect(isCheckedDirty(b)).toBe(true);
expect(isCheckedDirty(c)).toBe(true);
assertInputTrackingIsCurrent(container);
});

it('should check the correct radio when the selected name moves', () => {
class App extends React.Component {
state = {
Expand Down

0 comments on commit aadbb8a

Please sign in to comment.