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: update other radios tracked value in one group when one is updated to be checked #27024

Closed
wants to merge 1 commit into from

Conversation

zhengjitf
Copy link

@zhengjitf zhengjitf commented Jun 29, 2023

Summary

Fix #26876, which is caused by not updating tracked values. Please see here for the investigation details.

The root is, when one radio is changed to be checked (by updating the checked prop), the previously checked one won't update its tracked value (will be still 'true') if it's one of the previous siblings.

How did you test this change?

Testing Steps (refer to but not exactly follow the steps of reproduction):

  • Two radios with the same name, initially, radio1 is checked, and radio2 is unchecked. (radio1 is the previous sibling of radio2)
  • Update the checked props to make radio1 unchecked, and radio2 checked.
  • Then update the checked props to make radio1 checked, and radio2 unchecked.
  • Click radio2, the onChange handler should be called after fixing the issue.

Any feedback and suggestions will be appreciated. cc @eps1lon @sebmarkbage @acdlite

@react-sizebot
Copy link

Comparing: 2153a29...7e23144

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 +0.09% 164.32 kB 164.46 kB +0.11% 51.75 kB 51.80 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.08% 171.73 kB 171.87 kB +0.11% 53.97 kB 54.02 kB
facebook-www/ReactDOM-prod.classic.js +0.06% 568.20 kB 568.55 kB +0.09% 100.14 kB 100.23 kB
facebook-www/ReactDOM-prod.modern.js +0.06% 552.00 kB 552.35 kB +0.11% 97.29 kB 97.40 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 7e23144

@@ -189,6 +216,21 @@ export function updateInput(
checkAttributeStringCoercion(name, 'name');
}
node.name = toString(getToStringValue(name));

if (needUpdateRadioGroupTrackedValue) {
const group = findRadioGroup(node);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to be a fragile fix due to it querying the DOM during the mutation where things can happen before or after depending on tree order. It would also potentially trigger thrashing of the DOM internal caches instead of all writes and then reads.

If this worked before I think the solution is probably more likely something about the timing of when the mutation should happen to replicate the previous order of operations.

@zhengjitf zhengjitf closed this Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Radio button onChange not called in current React Canary
4 participants