React: Make on/off, yes/no boolean attributes work
When you pass a boolean to some attributes (e.g. autoSave, autoCorrect) in 15, they don't work correctly because they actually want a specific string (yes and no). I think there were also some attributes that want on and off.
Let's just “make them work”? Could use a special flag/whitelist for that. There should be very few of these.
Similarly we should probably make <script crossOrigin /> be valid and turn into <script crossOrigin="anonymous" />. Currently I don’t think this works on master.
gaearon
All 6 comments
This could be a can of worms.
autosave
I believe autosave expects a string name. We do not want yes/no behavior for it.
crossorigin
I'm okay with aliasing on/off, my only concern is that we do not neglect types that have string values, where an empty string represents a default. For example:
var img = new Image()
console.log(img.crossOrigin) // null
a.setAttribute('crossorigin', '')
console.log(a.crossOrigin) // anonymous
For the cross origin case, I think we need to assign "" for true and omit the value if it is false. If they pass a string, we assign that value.
autocapitalize
It's harder for autocapitalize. on/off was deprecated in iOS5.
The trouble is figuring out what false does for this case. true should be an empty string, using the browser default behavior and also inheriting from a related form tag. false should technically be... "none"? I'm not sure.
Maybe, I'd say:
- A flag that assigns any string value,
trueas"", andfalseas nothing. Maybe this should just be how HAS_BOOLEAN_VALUE works, or the default behavior for unknown attributes. - A flag that toggles
yes/noon a boolean value. But how many of these arenoby default and an empty string makes them "yes"? - A flag that toggles
on/offon a boolean value (I still need to figure out which attributes are these, besides what was deprecated forautocomplete)
nhunzaker
on 1 Sep 2017
I think it makes sense to do this only for attributes that accept boolean options like "yes"/"no" or "on"/"off". Attributes like autosave or autocapitalize that accept a set of values should require that the explicit value be set.
Similarly we should probably make be valid and turn into . Currently I don’t think this works on master.
There are other attributes with canonical cases. For example, preload will default to "auto", method will default to "get", etc. Would we normalize those as well? For these cases, I feel like this approach would require users to have some knowledge of the spec'd behavior to know what value true would represent, and it's confusing that true represents one enumerated value but false represents none of them.
If we did normalize it I think we should only do it in cases where users already expect it to work, and add warnings about it so we could eventually stop.
The attributes with "true" boolean enumerations that we could normalize would be:
autocomplete-"on"/"off"contenteditable-"true"/"false"draggable-"true"/"false"spellcheck-"true"/"false"translate-"yes"/"no"
aweary
on 1 Sep 2017
@nhunzaker: This could be a can of worms.
IMHO this. Perhaps I've missed too much of the discussion, but AFAIK it seems like there's a good reason why they aren't straight up boolean attributes, some of them default to true, some of them default to neither where not being set is special (like autocomplete) or mapped to another value (like contenteditable). Mapping boolean values to some implied string seems more likely to cause confusion for our users, there's also the possibility that more enum values will be added in the future (like autocapitalize), so then you're expected to feed null, false, true, "foobar", etc into it. It seems far preferable to just require strings for string-attributes (because then you're expected to be read up on how they work) and true boolean attributes are either just gone or empty string.
But I'm curious, from what I understand you want to avoid the whitelist and special-behavior. Why not adopt these rules:
nullandfalseremoves the attribute- Strings are passed through as-is
- Boolean
truebecomes empty string (and optionally valueless attributes for SSR)
These are simple rules to understand and you would know exactly what to expect from ReactDOM at all times, it wouldn't depend on some "maybe updated whitelist" and it wouldn't change between versions as the whitelist is updated and it would work for unsupported attributes too. Obviously you would need a temporary workaround where ReactDOM emit warnings in cases where we previously handled it okay-ish.
syranide
on 1 Sep 2017
~@syranide check out https://github.com/facebook/react/issues/10521 for more context~ I realize now that you already commented there 😄
Does the approach I outlined in https://github.com/facebook/react/issues/10521#issuecomment-324773506 sound like what you're thinking?
aweary
on 1 Sep 2017
@aweary Oh sorry, yeah it seems you already had it figured out. IMHO, that seems like a sensible approach.
syranide
on 6 Sep 2017
Taking off 16 milestone but let’s come back to this some time during 16.x to decide on the strategy for 17.
gaearon
on 13 Sep 2017
Related issues
zpao
·
3Comments
kocokolo
·
3Comments
trusktr
·
3Comments
jmullo
·
3Comments
UnbearableBear
·
3Comments
Most helpful comment
IMHO this. Perhaps I've missed too much of the discussion, but AFAIK it seems like there's a good reason why they aren't straight up boolean attributes, some of them default to true, some of them default to neither where not being set is special (like
autocomplete) or mapped to another value (likecontenteditable). Mapping boolean values to some implied string seems more likely to cause confusion for our users, there's also the possibility that more enum values will be added in the future (likeautocapitalize), so then you're expected to feednull,false,true,"foobar", etc into it. It seems far preferable to just require strings for string-attributes (because then you're expected to be read up on how they work) and true boolean attributes are either just gone or empty string.But I'm curious, from what I understand you want to avoid the whitelist and special-behavior. Why not adopt these rules:
nullandfalseremoves the attributetruebecomes empty string (and optionally valueless attributes for SSR)These are simple rules to understand and you would know exactly what to expect from ReactDOM at all times, it wouldn't depend on some "maybe updated whitelist" and it wouldn't change between versions as the whitelist is updated and it would work for unsupported attributes too. Obviously you would need a temporary workaround where ReactDOM emit warnings in cases where we previously handled it okay-ish.