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

Request to allow controlled checkboxes to remove checked when assigned null #7530

Closed
wants to merge 2 commits into from

Conversation

nhunzaker
Copy link
Contributor

We hit a snag on a project where a piece of state backing a checkbox was set to null instead of false, and the checkbox checked state was not reflected in the UI.

Steps to reproduce:

  1. Visit https://jsfiddle.net/69z2wepo/53315/
  2. Click "toggle" to show the checkbox
  3. Check the checkbox
  4. Click "toggle" to hide the checkbox
  5. Click "toggle" to show the checkbox (it should be checked)
  6. Click "Reset"

Expected: The checkbox is no longer checked
Actual: The checkbox is checked

Here's a recording of the behavior:

toggle

It looks like this happens because null returns a controlled input to the initial checked state (inst._wrapperState.initialChecked) it was mounted with:

https://github.com/facebook/react/blob/master/src/renderers/dom/client/wrappers/ReactDOMInput.js#L78

To me, this is inconsistent with the behavior of other attributes. If given null, the attribute is removed. Additionally, if input.checked is assigned null directly, it becomes unchecked.

This PR configures ReactDOMInput to respect null for controlled checkboxes. It will only ever fallback to initial mounted value on undefined.

If a change like this is impossible, could there be a warning or additional documentation along the lines of tips/controlled-input-null-value? I'm happy to follow up with an issue if so (I'll even write the docs, if given a formal stance).

@@ -146,7 +146,7 @@ var ReactDOMInput = {

var defaultValue = props.defaultValue;
inst._wrapperState = {
initialChecked: props.checked != null ? props.checked : props.defaultChecked,
initialChecked: props.checked !== undefined ? props.checked : props.defaultChecked,
initialValue: props.value != null ? props.value : defaultValue,
Copy link
Contributor

Choose a reason for hiding this comment

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

If accepted, wouldn't this introduce a new inconsistency between controlled text inputs and checkboxes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Would it be unreasonable to extend this to text inputs?

Copy link
Contributor

@syranide syranide Aug 22, 2016

Choose a reason for hiding this comment

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

Has been discussed before and rejected. I don't think anyone feels super strongly about the current implementation, but changing it means breaking a lot of code.

@syranide
Copy link
Contributor

Giving undefined or null for value/checked for inputs signals it to become uncontrolled, hence not updating the value. My personal recommendation is to always use an explicit wrapper, that's either uncontrolled/controlled.

Given that there is a warning for value it should reasonably also exists for checked (haven't checked if it does, but it seems it does not based on this issue).

@jimfb
Copy link
Contributor

jimfb commented Aug 23, 2016

Yeah, that should be a warning, but it appears to not be firing (https://jsfiddle.net/s4742gkk/).

I'm going to close this out, track it as #7544

@jimfb jimfb closed this Aug 23, 2016
@ghost ghost added the CLA Signed label Aug 23, 2016
@nhunzaker
Copy link
Contributor Author

nhunzaker commented Aug 26, 2016

I know I'm getting back to this late, but bummer.

I hope this isn't too much smoke blowing, but it doesn't seem like controlled/uncontrolled behavior should be regulated by the value types at all. It could be determined by the ownership, like if props.hasOwnProperty('value') or props.hasOwnProperty('checked') return true. This would signal controlled behavior through configuration, rather than a value type (and require type check warnings).

I'll have to dig into the history here.

@syranide
Copy link
Contributor

@nhunzaker I'd say it should be different components entirely. Doing it by property check isn't a good idea either, because then you can't easily have wrapper components passing along value, they would always become controlled then. Also, kind of nit-picky, but you should never be passing null to something that expects a string (especially not inputs). So even if null was allowed, I would call it bad practice and can cause problematic edge-cases, so I wouldn't mind a warning still.

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Aug 27, 2016

I'd say it should be different components entirely.

@syranide I think I follow. Do you mean that React should produce a new component instance when switching from controlled to uncontrolled? I believe that would certainly solve this case, and possibly eliminate some of the complexity around properly switching an input's "mode" as React attempts to recycle DOM inputs (do I have that right?).

Also, kind of nit-picky, but you should never be passing null to something that expects a string (especially not inputs).

True, but React's standard behavior gracefully accommodates it. If I set lang={null} in React, it will either ignore the attribute assignment or remove it. This is different than a direct call to setAttribute, which would assign a lang attribute of "null" (instead of removing it). I believe this tolerance is a wonderful feature of React.

then you can't easily have wrapper components passing along value, they would always become controlled then

Fair point; one I can't really argue against.

@syranide
Copy link
Contributor

@syranide I think I follow. Do you mean that React should produce a new component instance when switching from controlled to uncontrolled? I believe that would certainly solve this case, and possibly eliminate some of the complexity around properly switching an input's "mode" as React attempts to recycle DOM inputs (do I have that right?).

Last time we discussed this everyone agreed that basically keying by controlled/uncontrolled internally is the right way to go (hence the warning, you should key it yourself). However, my point here is that IMHO input shouldn't be able to be uncontrolled and controlled, there should be a hypothetical UncontrolledInput and ControlledInput, the two behaviors should have different types.

True, but React's standard behavior gracefully accommodates it. If I set lang={null} in React, it will either ignore the attribute assignment or remove it. This is different than a direct call to setAttribute, which would assign a lang attribute of "null" (instead of removing it). I believe this tolerance is a wonderful feature of React.

The difference is that lang represents a static option, there's an obvious behavior for what happens when it doesn't exist. value describes the value displayed to the user and which the user interacts with. null is a lack of value and strictly speaking there is no universal default value for inputs, it depends entirely on the context of where it's used. Put differently, a checkbox given no value, should it default to being true or false? Depending on context either true or false should be the default, so you should be explicit and not pass null.

This tolerance is also error-prone, because 3 values now correspond to the same visual input state (undefined, null and empty string). Which means that if you check for empty string it will fail because null/undefined. If you pass the value along then something else may fail to check for null/undefined or you may end up storing null/undefined in a database. Or it may be stringified somewhere else and become "undefined" or "null".

@nhunzaker
Copy link
Contributor Author

Thank you for the thorough response, though I apologize for taking you away
from your weekend for it. I'm curious about a dedicated ControlledInput
and UncontrolledInput. I'll dig up the old threads to get additional
context.

Thank you for walking me through this!

On Sat, Aug 27, 2016, 10:58 AM Andreas Svensson notifications@github.com
wrote:

@syranide https://github.com/syranide I think I follow. Do you mean
that React should produce a new component instance when switching from
controlled to uncontrolled? I believe that would certainly solve this case,
and possibly eliminate some of the complexity around properly switching an
input's "mode" as React attempts to recycle DOM inputs (do I have that
right?).

Last time we discussed this everyone agreed that basically keying by
controlled/uncontrolled internally is the right way to go (hence the
warning, you should key it yourself). However, my point here is that IMHO
input shouldn't be able to be uncontrolled and controlled, there should
be a hypothetical UncontrolledInput and ControlledInput, the two
behaviors should have different types.

True, but React's standard behavior gracefully accommodates it. If I set
lang={null} in React, it will either ignore the attribute assignment or
remove it. This is different than a direct call to setAttribute, which
would assign a lang attribute of "null" (instead of removing it). I believe
this tolerance is a wonderful feature of React.

The difference is that lang represents a static option, there's an
obvious behavior for what happens when it doesn't exist. value describes
the value displayed to the user and which the user interacts with. null
is a lack of value and strictly speaking there is no universal default
value for inputs, it depends entirely on the context of where it's used.
Put differently, a checkbox given no value, should it default to being true
or false? Depending on context either true or false should be the default,
so you should be explicit and not pass null.

This tolerance is also error-prone, because 3 values now correspond to the
same visual input state (undefined, null and empty string). Which means
that if you check for empty string it will fail because null/undefined. If
you pass the value along then something else may fail to check for
null/undefined or you may end up storing null/undefined in a database. Or
it may be stringified somewhere else and become "undefined" or "null".


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7530 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAkEOB_X7_wHQeteh4QabEncNN39S84fks5qkHrZgaJpZM4JoljY
.

@syranide
Copy link
Contributor

syranide commented Aug 27, 2016

@nhunzaker Not at all, this is just my opinion on it and I find it interesting to discuss :)

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.

4 participants