Fix checkbox and radio hydration #27401

sophiebits · 2023-09-21T04:46:16Z

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, so the DOM is out of sync with React state.

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.

react-sizebot · 2023-09-21T04:48:27Z

Comparing: 3c27178...76e127b

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	166.66 kB	166.67 kB	=	52.14 kB	52.14 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	176.10 kB	176.11 kB	=	54.97 kB	54.98 kB
facebook-www/ReactDOM-prod.classic.js	=	571.96 kB	571.97 kB	=	100.66 kB	100.66 kB
facebook-www/ReactDOM-prod.modern.js	=	555.68 kB	555.70 kB	=	97.77 kB	97.77 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 76e127b

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.

sophiebits · 2023-09-21T05:32:33Z

Hmm actually testing more with 18.2.0, it seems like uncontrolled inputs get their DOM state overridden by React props upon hydration (I'm guessing because of attribute fixup logic), whereas controlled inputs do not. How's that for unintuitive.

And the dirty flag did not get consistently set during hydration (even though it did during a client render from scratch).

Mrrh.

sophiebits · 2023-09-21T20:36:07Z

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

-  // TODO: I'm pretty sure this is a bug because initialValueTracking won't be
-  // correct for the hydration case then.
-  if (!isHydrating) {
+  if (isHydrating) {


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

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

no, I take it back. per

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

Line 247 in 34de298

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

eps1lon

makes sense

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, so the DOM is out of sync with React state. 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. DiffTrain build for [db69f95](db69f95)

React upstream changes: - facebook/react#27401 - facebook/react#27443 - facebook/react#27445 - facebook/react#27364 - facebook/react#27440 - facebook/react#27436

Fixes whatever part of facebook#26876 and vercel/next.js#49499 that facebook#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, so the DOM is out of sync with React state. 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.

…experimental prefix for server action APIs (#56809) The latest React canary builds have a few changes that need to be adopted for compatability. 1. the `useFormState` and `useFormStatus` hooks in `react-dom` and the `formData` opiont in `react-dom/server` are no longer prefixed with `experimental_` 2. server content (an undocumented React feature) has been removed. Next only had trivial intenral use of this API and did not expose a coherent feature to Next users (no ability to seed context on refetches). It is still possible that some users used the React server context APIs which is why this should go into Next 14. ### React upstream changes - facebook/react#27513 - facebook/react#27514 - facebook/react#27511 - facebook/react#27508 - facebook/react#27502 - facebook/react#27474 - facebook/react#26789 - facebook/react#27500 - facebook/react#27488 - facebook/react#27458 - facebook/react#27471 - facebook/react#27470 - facebook/react#27464 - facebook/react#27456 - facebook/react#27462 - facebook/react#27461 - facebook/react#27460 - facebook/react#27459 - facebook/react#27454 - facebook/react#27457 - facebook/react#27453 - facebook/react#27401 - facebook/react#27443 - facebook/react#27445 - facebook/react#27364 - facebook/react#27440 - facebook/react#27436 --------- Co-authored-by: Zack Tanner <zacktanner@gmail.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Jiachi Liu <inbox@huozhi.im>

Fixes whatever part of facebook#26876 and vercel/next.js#49499 that facebook#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, so the DOM is out of sync with React state. 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.

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, so the DOM is out of sync with React state. 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. DiffTrain build for commit db69f95.

sophiebits requested review from sebmarkbage and eps1lon September 21, 2023 04:46

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 21, 2023

sophiebits force-pushed the gh-26876-2 branch from aadbb8a to 1a80690 Compare September 21, 2023 04:52

Base automatically changed from gh-26876 to main September 21, 2023 04:58

sophiebits force-pushed the gh-26876-2 branch from 1a80690 to 76e127b Compare September 21, 2023 04:58

sophiebits commented Sep 21, 2023

View reviewed changes

christianjuth mentioned this pull request Sep 26, 2023

Radio Button Ignores "defaultChecked" Property on First Render vercel/next.js#51932

Closed

1 task

sophiebits force-pushed the gh-26876-2 branch from e4e3432 to 76e127b Compare September 28, 2023 02:56

eps1lon approved these changes Oct 1, 2023

View reviewed changes

sophiebits merged commit db69f95 into main Oct 2, 2023
36 checks passed

acdlite mentioned this pull request Oct 4, 2023

Update React from d900fadbf to 6f1324395 vercel/next.js#56220

Closed

shuding mentioned this pull request Oct 11, 2023

Upgrade vendored React vercel/next.js#56705

Closed

gnoff mentioned this pull request Oct 13, 2023

Update React from d900fadbf to 09fbee89d. Removes server context and experimental prefix for server action APIs vercel/next.js#56809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix checkbox and radio hydration #27401

Fix checkbox and radio hydration #27401

sophiebits commented Sep 21, 2023

react-sizebot commented Sep 21, 2023 •

edited

sophiebits commented Sep 21, 2023 •

edited

sophiebits Sep 21, 2023

sophiebits Sep 28, 2023

sophiebits Sep 28, 2023

eps1lon left a comment

Fix checkbox and radio hydration #27401

Fix checkbox and radio hydration #27401

Conversation

sophiebits commented Sep 21, 2023

react-sizebot commented Sep 21, 2023 • edited

Critical size changes

Significant size changes

sophiebits commented Sep 21, 2023 • edited

sophiebits Sep 21, 2023

Choose a reason for hiding this comment

sophiebits Sep 28, 2023

Choose a reason for hiding this comment

sophiebits Sep 28, 2023

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

react-sizebot commented Sep 21, 2023 •

edited

sophiebits commented Sep 21, 2023 •

edited