Failing test for radio checked with changing names #26588

sophiebits · 2023-04-10T21:33:31Z

 FAIL  packages/react-dom/src/__tests__/ReactDOMInput-test.js (8.01 s)
  ● ReactDOMInput › shouldn't get tricked by changing radio names, part 2

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      1209 |       container
      1210 |     );
    > 1211 |     expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(true);
           |                                                                               ^
      1212 |     expect(container.querySelector('input[name="b"][value="2"]').checked).toBe(true);
      1213 |   });
      1214 |

      at Object.<anonymous> (packages/react-dom/src/__tests__/ReactDOMInput-test.js:1211:79)

``` FAIL packages/react-dom/src/__tests__/ReactDOMInput-test.js (8.01 s) ● ReactDOMInput › shouldn't get tricked by changing radio names, part 2 expect(received).toBe(expected) // Object.is equality Expected: true Received: false 1209 | container 1210 | ); > 1211 | expect(container.querySelector('input[name="a"][value="1"]').checked).toBe(true); | ^ 1212 | expect(container.querySelector('input[name="b"][value="2"]').checked).toBe(true); 1213 | }); 1214 | at Object.<anonymous> (packages/react-dom/src/__tests__/ReactDOMInput-test.js:1211:79) ```

react-sizebot · 2023-04-10T21:37:53Z

Comparing: 451736b...89508f2

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	=	164.58 kB	164.58 kB	=	51.69 kB	51.69 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	166.15 kB	166.15 kB	=	52.20 kB	52.20 kB
facebook-www/ReactDOM-prod.classic.js	=	554.05 kB	554.05 kB	=	98.17 kB	98.17 kB
facebook-www/ReactDOM-prod.modern.js	=	537.89 kB	537.89 kB	=	95.51 kB	95.51 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 89508f2

sophiebits · 2023-04-10T22:25:12Z

(just confirmed this fails on 18.2.0)

sophiebits · 2023-04-10T23:22:29Z

I think this could work except maybe it's wasteful and I'm not sure if it mucks with dirty checkedness incorrectly:

diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js
index e9398cf07..a29aa4f21 100644
--- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js
+++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js
@@ -1243,11 +1243,14 @@ export function updateProperties(
       break;
     }
     case 'input': {
-      // Update checked *before* name.
-      // In the middle of an update, it is possible to have multiple checked.
-      // When a checked radio tries to change name, browser makes another radio's checked false.
-      if (nextProps.type === 'radio' && nextProps.name != null) {
-        updateInputChecked(domElement, nextProps);
+      // If a radio button's name or type change, then we need to be careful
+      // about the order of property assignment to ensure other radio buttons in
+      // the same (previous or next) group don't get incorrectly unchecked.
+      // Easiest way to do this is to unset checked and then restore it after.
+      let restoreChecked = false;
+      if ((lastProps.type === 'radio' || nextProps.type === 'radio') && domElement.checked) {
+        domElement.checked = false;
+        restoreChecked = true;
       }
       for (const propKey in lastProps) {
         const lastProp = lastProps[propKey];
@@ -1264,6 +1267,7 @@ export function updateProperties(
                 !!checked &&
                 typeof checked !== 'function' &&
                 checked !== 'symbol';
+              restoreChecked = false;
               break;
             }
             case 'value': {
@@ -1294,6 +1298,7 @@ export function updateProperties(
                 !!checked &&
                 typeof checked !== 'function' &&
                 checked !== 'symbol';
+              restoreChecked = false;
               break;
             }
             case 'value': {
@@ -1317,6 +1322,9 @@ export function updateProperties(
           }
         }
       }
+      if (restoreChecked) {
+        domElement.checked = true;
+      }
 
       if (__DEV__) {
         const wasControlled =
@@ -1654,11 +1662,14 @@ export function updatePropertiesWithDiff(
       break;
     }
     case 'input': {
-      // Update checked *before* name.
-      // In the middle of an update, it is possible to have multiple checked.
-      // When a checked radio tries to change name, browser makes another radio's checked false.
-      if (nextProps.type === 'radio' && nextProps.name != null) {
-        updateInputChecked(domElement, nextProps);
+      // If a radio button's name or type change, then we need to be careful
+      // about the order of property assignment to ensure other radio buttons in
+      // the same (previous or next) group don't get incorrectly unchecked.
+      // Easiest way to do this is to unset checked and then restore it after.
+      let restoreChecked = false;
+      if ((lastProps.type === 'radio' || nextProps.type === 'radio') && domElement.checked) {
+        domElement.checked = false;
+        restoreChecked = true;
       }
       for (let i = 0; i < updatePayload.length; i += 2) {
         const propKey = updatePayload[i];
@@ -1672,6 +1683,7 @@ export function updatePropertiesWithDiff(
               !!checked &&
               typeof checked !== 'function' &&
               checked !== 'symbol';
+            restoreChecked = false;
             break;
           }
           case 'value': {
@@ -1694,6 +1706,9 @@ export function updatePropertiesWithDiff(
           }
         }
       }
+      if (restoreChecked) {
+        domElement.checked = true;
+      }
 
       if (__DEV__) {
         const wasControlled =

I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in #26596 (comment) and #26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com>

I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in #26596 (comment) and #26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com> DiffTrain build for [1f248bd](1f248bd)

sophiebits · 2023-04-20T00:40:30Z

merged as part of #26667

I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in #26596 (comment) and #26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com>

I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in facebook/react#26596 (comment) and facebook/react#26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com> DiffTrain build for [1f248bdd7199979b050e4040ceecfe72dd977fd1](facebook/react@1f248bd)

) I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in facebook#26596 (comment) and facebook#26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com>

I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in facebook/react#26596 (comment) and facebook/react#26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com> DiffTrain build for commit facebook/react@1f248bd.

sophiebits requested a review from sebmarkbage April 10, 2023 21:33

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

sophiebits mentioned this pull request Apr 10, 2023

Diff properties in the commit phase instead of generating an update payload #26583

Merged

sebmarkbage mentioned this pull request Apr 19, 2023

Switching checked to null should leave the current value #26667

Merged

sophiebits closed this Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Failing test for radio checked with changing names #26588

Failing test for radio checked with changing names #26588

Uh oh!

sophiebits commented Apr 10, 2023 •

edited

Loading

Uh oh!

react-sizebot commented Apr 10, 2023

Uh oh!

sophiebits commented Apr 10, 2023

Uh oh!

sophiebits commented Apr 10, 2023

Uh oh!

sophiebits commented Apr 20, 2023

Uh oh!

Uh oh!

Failing test for radio checked with changing names #26588

Failing test for radio checked with changing names #26588

Uh oh!

Conversation

sophiebits commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

react-sizebot commented Apr 10, 2023

Critical size changes

Significant size changes

Uh oh!

sophiebits commented Apr 10, 2023

Uh oh!

sophiebits commented Apr 10, 2023

Uh oh!

sophiebits commented Apr 20, 2023

Uh oh!

Uh oh!

sophiebits commented Apr 10, 2023 •

edited

Loading