Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix checkbox and radio hydration #27401

Merged
merged 1 commit into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tempted to change this to isHydrating && checked == null, so you can interactions before hydration work for uncontrolled but make sure they get lost for controlled so we're in sync

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I did it. this seems like the most sensible behavior to me without going as far as firing onChange during hydration

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, I take it back. per

describe('user interaction with inputs before client render', function () {

we leave user interactions on input value and select despite DOM and React being out of sync so I guess we should for radio and checkbox too.

please review this PR as I had it originally. thanks <3

// Detach .checked from .defaultChecked but leave user input alone
node.checked = node.checked;
} else {
node.checked = !!initialChecked;
}

Expand Down
177 changes: 177 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,175 @@ describe('ReactDOMInput', () => {
assertInputTrackingIsCurrent(container);
});

it('should hydrate controlled radio buttons', async () => {
function App() {
const [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);

// And we can also change to B properly after hydration.
await act(async () => {
setUntrackedChecked.call(b, true);
dispatchEventOnNode(b, 'click');
});
assertLog(['click b']);
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);
});

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