From 76e127b4a7f8e6758b411bd2e1035f613c772488 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Wed, 20 Sep 2023 21:40:21 -0700 Subject: [PATCH] Fix checkbox and radio hydration Fixes whatever part of https://github.com/facebook/react/issues/26876 and https://github.com/vercel/next.js/issues/49499 that https://github.com/facebook/react/pull/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. --- .../src/client/ReactDOMInput.js | 10 +- .../src/__tests__/ReactDOMInput-test.js | 177 ++++++++++++++++++ 2 files changed, 181 insertions(+), 6 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMInput.js b/packages/react-dom-bindings/src/client/ReactDOMInput.js index 4411887e3cbf0..9570f25c84fee 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMInput.js +++ b/packages/react-dom-bindings/src/client/ReactDOMInput.js @@ -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; } diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 3e984b9efc18c..6c4e55fb14407 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -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; @@ -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); @@ -1235,6 +1243,175 @@ describe('ReactDOMInput', () => { assertInputTrackingIsCurrent(container); }); + it('should hydrate controlled radio buttons', async () => { + function App() { + const [current, setCurrent] = React.useState('a'); + return ( + <> + { + Scheduler.log('click a'); + setCurrent('a'); + }} + /> + { + Scheduler.log('click b'); + setCurrent('b'); + }} + /> + { + Scheduler.log('click c'); + // Let's say the user can't pick C + }} + /> + + ); + } + const html = ReactDOMServer.renderToString(); + 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, ); + }); + + // 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 ( + <> + Scheduler.log('click a')} + /> + Scheduler.log('click b')} + /> + Scheduler.log('click c')} + /> + + ); + } + const html = ReactDOMServer.renderToString(); + 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, ); + }); + + // 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 = {