Switching checked to null should leave the current value #26667
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We set a blank name to disconnect it from a set of radio buttons, then make the mutations, then set the name at the end to apply the new state to the set.
```
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)
```
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: d8089f2...12db62b Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
sebmarkbage
commented
Apr 19, 2023
| if (__DEV__) { | ||
| checkAttributeStringCoercion(type, 'type'); | ||
| } | ||
| node.type = type; |
There was a problem hiding this comment.
This is always setting the type instead of only if it has changed. Not sure if that would matter like resetting dirty flags.
There was a problem hiding this comment.
I think it's fine? from the spec it sounds like it would only be the value sanitization algorithm but I tested https://codesandbox.io/s/crazy-ace-rc85uh?file=/src/App.js in chrome, safari, and firefox and none of them seem to do anything when reassigning type to the same value (you can type non-numbers like the letter e and they don't go away when the timer fires).
sebmarkbage
commented
Apr 19, 2023
| // Changing the type or name as the same time as changing the checked value | ||
| // needs to be atomically applied. We can only ensure that by disconnecting | ||
| // the name while do the mutations and then reapply the name after that's done. | ||
| node.name = ''; |
There was a problem hiding this comment.
Similarly, we didn't used to do this in the update phase - only init phase. Not sure if it'll mess with some internal states.
There was a problem hiding this comment.
I think it should be good
sophiebits
approved these changes
Apr 19, 2023
| if (__DEV__) { | ||
| checkAttributeStringCoercion(type, 'type'); | ||
| } | ||
| node.type = type; |
There was a problem hiding this comment.
I think it's fine? from the spec it sounds like it would only be the value sanitization algorithm but I tested https://codesandbox.io/s/crazy-ace-rc85uh?file=/src/App.js in chrome, safari, and firefox and none of them seem to do anything when reassigning type to the same value (you can type non-numbers like the letter e and they don't go away when the timer fires).
| // Changing the type or name as the same time as changing the checked value | ||
| // needs to be atomically applied. We can only ensure that by disconnecting | ||
| // the name while do the mutations and then reapply the name after that's done. | ||
| node.name = ''; |
There was a problem hiding this comment.
I think it should be good
| } | ||
| node.name = name; | ||
| } else { | ||
| node.removeAttribute('name'); |
There was a problem hiding this comment.
is it better to removeAttribute above instead of = '' so you don't need to do it here?
| node.name = name; | ||
| } else { | ||
| node.removeAttribute('name'); |
There was a problem hiding this comment.
is it already unset? or for hydration it's not? in which case do you still need the previous clearing that was in the old code?
There was a problem hiding this comment.
Yea, I missed this one. I'll remove. It's a tricky one because it's a hydration error in DEV regardless. We don't try to patch up other things so is there a reason we'd consider this case important? Seems like not.
In fact, I have another todo to reconsider if we should even call this method at all during hydration. Seems like we should dispatch a change event if it's different and then trigger the restore mechanism but that would have to happen after commit.
We won't clean up hydration.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 19, 2023
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)
kassens
pushed a commit
that referenced
this pull request
Apr 21, 2023
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>
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
) 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>
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
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 commit 1f248bd.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I accidentally made a behavior change in the refactor. It turns out that when switching off
checkedto 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
checkedtoinitialChecked:react/packages/react-dom-bindings/src/client/ReactDOMInput.js
Line 69 in 5cbe625
Since we never changed
initialCheckedand it's not relevant if non-nullcheckedchanges value, the only way this "change" could trigger was if we move from havingcheckedto having null.This wasn't really consistent with how
valueworks, where we instead leave the current value in place regardless. So this is a "bug fix" that changescheckedto be consistent withvalueand 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.