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

Make on/off, yes/no boolean attributes work #10589

Closed
gaearon opened this issue Sep 1, 2017 · 6 comments
Closed

Make on/off, yes/no boolean attributes work #10589

gaearon opened this issue Sep 1, 2017 · 6 comments
Labels
Component: DOM React Core Team Opened by a member of the React Core Team Type: Feature Request

Comments

@gaearon
Copy link
Collaborator

gaearon commented Sep 1, 2017

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 gaearon added this to the 16.0 milestone Sep 1, 2017
@nhunzaker
Copy link
Contributor

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:

  1. A flag that assigns any string value, true as "", and false as nothing. Maybe this should just be how HAS_BOOLEAN_VALUE works, or the default behavior for unknown attributes.
  2. A flag that toggles yes/no on a boolean value. But how many of these are no by default and an empty string makes them "yes"?
  3. A flag that toggles on/off on a boolean value (I still need to figure out which attributes are these, besides what was deprecated for autocomplete)

@aweary
Copy link
Contributor

aweary commented Sep 1, 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 <script crossOrigin /> be valid and turn into <script crossOrigin="anonymous" />. 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"

@syranide
Copy link
Contributor

syranide commented Sep 1, 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:

  1. null and false removes the attribute
  2. Strings are passed through as-is
  3. Boolean true becomes 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.

@aweary
Copy link
Contributor

aweary commented Sep 1, 2017

@syranide check out #10521 for more context I realize now that you already commented there 😄

Does the approach I outlined in #10521 (comment) sound like what you're thinking?

@syranide
Copy link
Contributor

syranide commented Sep 6, 2017

@aweary Oh sorry, yeah it seems you already had it figured out. IMHO, that seems like a sensible approach.

@gaearon gaearon removed this from the 16.0 milestone Sep 13, 2017
@gaearon
Copy link
Collaborator Author

gaearon commented Sep 13, 2017

Taking off 16 milestone but let’s come back to this some time during 16.x to decide on the strategy for 17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: DOM React Core Team Opened by a member of the React Core Team Type: Feature Request
Projects
None yet
Development

No branches or pull requests

5 participants