Decide on the desired behavior when a boolean value is passed to an unknown prop.

[flarnie]

Moving the discussion from comments in #10509 to this issue.

[sophiebits]

cc @aweary @nhunzaker

What are the properties that expect "true" or "false" values?

I know the ARIA boolean attributes do but from a cursory look at https://www.w3.org/TR/wai-aria/states_and_properties, all of the boolean attributes default to false (grep the page for true (default), no matches – compare to false (default)).

That means we could do ="true" for true and omit the attribute for false. I think that might be compatible for almost every case. In HTML, any string is treated as a boolean value so ="true" is valid. download="true" is the only attribute I know of whose behavior is different from download and we already have a special case for that that I guess we can just keep.

[flarnie]

Attempting to summarize the proposals and pros/cons so far:

Option 1:
Stringify and pass booleans through.

Pros:

• Consistent with the way React currently treats known attributes.
• Already implemented in Moar attribute tests #10509

Cons:

• Is probably not what the engineer wants or expects their code to do.
• Could create bugs; 'foo="false"' may get treated the same as 'foo="true"' by some browsers.

Option 2:
Render is as an implicit boolean attribute, either adding with a no value if true was passed, removing if false was passed.

Pros:

• This is consistent with the spec, and probably closest to the desired behavior.
• This would fix Boolean attributes on Web Components #9230

Cons:

• Needs to be implemented.
• We should change behavior for known attributes as well, in order to be consistent. If not, then introduces inconsistency.
• (?) Introduces another flavor of behavior for attributes in React, could be confusing. Maybe not a problem.

Option 3:
Remove booleans and warn.

Pros:

• This is consistent with how we treat other "invalid" values, like functions.
• This is the behavior on master, and could be achieved by minor updates to Moar attribute tests #10509

Cons:

• Would require minor updates to Moar attribute tests #10509.
• Isn't totally consistent with the spec for boolean attributes, might be unexpected for users.

Edit: Please correct anything I missed, thanks to everyone who has chimed in about this.

[flarnie]

Also wanted to share this:

Let's try to:

• Keep behavior consistent between "known attributes" and "unknown attributes" when possible
• Limit the number of colors used; it's simpler for users to have fewer variation how React deals with property values.

[sophiebits]

So IIUC as far as we know option 2 has no downsides and will never cause the wrong behavior that we know of. Is that right? If so, that sounds the most compelling to me.

[aweary]

That means we could do ="true" for true and omit the attribute for false.

@spicyj setting boolean attributes to "true" isn't valid according to the HTML spec section on boolean attributes.

If the attribute is present, its value must either be the empty string or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace. The values "true" and "false" are not allowed on boolean attributes.

This is especially a problem with custom elements, as outlined in #9230.

download="true" is the only attribute I know of whose behavior is different

spellcheck is another enumerable attribute that accepts "true" and "false".

What I'm suggesting is this:

1. If it's a known attribute that expects a stringified boolean, stringify it before calling setAttribute

2. If it's an unknown attribute, treat true and false as boolean attributes according to the spec (setting attribute with an empty string or a case-insensitive match of the attribute name for true and removing the attribute for false)

3. Otherwise, call setAttribute with the stringified value

[nhunzaker]

@spicyj Actually... for a prior version of the custom attributes feature, @gaearon and I dug through the spec and found all of the attributes that expected true/false strings. They are:

HTML:

    autoComplete: 0,
    // IE only true/false iFrame attribute
    // https://msdn.microsoft.com/en-us/library/ms533072(v=vs.85).aspx
    allowTransparency: 0,
    contentEditable: 0,
    draggable: 0,
    spellCheck: 0,
    // autoCapitalize and autoCorrect are supported in Mobile Safari for
    // keyboard hints.
    autoCapitalize: 0,
    autoCorrect: 0,
    // autoSave allows WebKit/Blink to persist values of input fields on page reloads
    autoSave: 0,

SVG:

  'autoReverse',
  'externalResourcesRequired',
  'preserveAlpha',

[sophiebits]

@aweary It has the correct behavior though, right? In practice, you would test .hasAttribute() since it is common to do checked="" and common to do checked="".

@nhunzaker Ah, so like autoComplete={false} would need to become autocomplete="false"? Good find.

[aweary]

@spicyj the existing behavior isn't correct because it renders <div bool={false} /> as <div bool="false" />. The spec also explicitly states that "true" is not a valid value for boolean attributes, so I suspect there may be implications extending past whether hasAttribute returns the right value (for example, with custom elements)

[nhunzaker]

I think what @aweary is suggesting makes sense, we already have the stringy boolean attributes in the list. We'd still allow all boolean values through in the filter, but change the way the boolean attribute is actually written in DOMPropertyOperations and DOMMarkupOperations.

[nhunzaker]

@aweary Still, I'm curious about the behavior here. Why does setAttribute('test', false) assign a string 'false' here, if that doesn't match the spec?

https://codepen.io/nhunzaker/pen/vJrvmY

[aweary]

@nhunzaker it's weird, I know. Boolean attributes are kind of spec'd as "they should exist, or they shouldn't" so using setAttribute to remove a boolean attribute doesn't make sense given that the API is all about setting attributes. That's why the spec states to use removeAttribute for boolean attributes with a false value.

At least, that's my interpretation. I can't speak to the motives of the spec authors 😄

[aweary]

We'd still allow all boolean values through in the filter, but change the way the boolean attribute is actually written in DOMPropertyOperations and DOMMarkupOperations.

👍 maybe we could coerce known enumerable attributes expecting booleans to strings earlier in the code path so that setValueForAttribute can just coerce true to "" and call removeAttribute for false. Or maybe pass a flag to setValueForAttribute. Either way, I think implementing this behavior shouldn't be too complex.

[flarnie]

@sebmarkbage pointed out we don't actually have a consistent behavior in 15.* for booleans - sometimes they get stringified, sometimes maybe not. I'll let him add some context on why we thought of stringifying as the default in 16.0.

[aweary]

To clarify my position, I'm mostly advocating for following the spec for unknown boolean attributes. If there are compatibility concerns with some set of known/previously-known attributes, that totally makes sense, and I can get behind special casing that behavior.

I just think the default approach for attributes we have no information about should be consistent with the spec if possible.

[sophiebits]

I was arguing that conformance to the spec in a case like this doesn't matter – as far as I can tell, attributes described by that spec behave as "if the attribute is present, it's on; if it's not, it's off". Even though the spec says true/false isn't correct, I interpret that as a warning against writing "true" in HTML because then you would be liable to think "false" would work as false. That isn't the case so writing "true" manually is a refactoring hazard in HTML, but that argument doesn't translate over to React. If "true" behaves the same as "" in all browsers, I don't see the downside of passing "true".

[aweary]

I'm arguing that it's safer to conform to the spec for unknown attributes that accept booleans, especially in the context of future boolean attributes that may depend on the behavior defined in the spec. This is also a concern with custom elements as outlined in #9230 (maybe @robdodson can provide more insight here).

More importantly, it does matter for boolean attributes when the value is false since browsers definitely don't treat "false" the same as an absent attribute. I think it would be confusing if true was stringified and false wasn't, so it makes sense to me to follow the spec in both cases.

What are the downsides? If we already have a whitelist of boolean attributes we can special case those to stringify if we have compatibility concerns until the next major and treat truly unknown attributes as the spec describes.

[sophiebits]

The (only?) advantage of "true"-vs-omit is that aria-* and checked can use the same logic.

[nhunzaker]

@spicyj and @aweary is the only different between your implementations whether to translate true to "" vs "true" for unknown attributes?

[aweary]

If we want to support enumerated attributes with boolean values (so spellcheck={true} renders as spellcheck="true") we'll need to include some branching logic which I think can just be reused for ARIA attributes and checked?

@nhunzaker that, and false removing the attribute instead of stringifying (unless we're in agreement on that?)

[sophiebits]

I was also arguing that false should omit the attribute for unknown attributes.

[nhunzaker]

But @spicyj said:

That means we could do ="true" for true and omit the attribute for false.

If we did an empty string, I think we could remove every entry from the HTMLPropertyConfig for HAS_BOOLEAN_VALUE:
https://github.com/facebook/react/blob/master/src/renderers/dom/shared/HTMLDOMPropertyConfig.js

We'd need like 7 exceptions for string values, but from a design standpoint, I like that it removes code.

[nhunzaker]

@spicyj Hah! Sorry. Spoke for you :)

[aweary]

@spicyj I somehow skimmed over that part, and I even quoted it 😄 thanks for clarifying. @nhunzaker If we want to keep stringifying boolean attributes that were already stringified in 15* we'd probably need to keep the whitelist right? I'm not sure if that point is clear or not though, how concerned are we about that?

But even then, we'd be able to remove it in the following major, which would be nice.

[aweary]

Oh wait, HAS_BOOLEAN_VALUE implements the behavior I'm advocating for, nevermind on that point 👀

[nhunzaker]

Right. I think we can safely remove it. This could be non-breaking.

[sophiebits]

Not sure if I'm following exactly what you're discussing, but I think removing HAS_BOOLEAN_VALUE is breaking since it changes what happens when you pass non-boolean values to it.

[nhunzaker]

Ah you're correct, for context this happens here:
https://github.com/facebook/react/blob/master/src/renderers/dom/shared/DOMPropertyOperations.js#L185-L190

So the breaking change would be that you could technically assign a string value to one of these attributes, where you'd need to pass a boolean to get the expected behavior (an empty string assuming we go with @aweary's approach).

But maybe this is fine? It wouldn't be spec compliant, but as @spicyj suggested earlier, maybe presence is enough. null and false would still remove the value.

Taking hidden as an example of a HAS_BOOLEAN_VALUE prop. Using the string "false" coerces to true and assigns the attribute.

So at best we're spec compliant, but at worst everything works as expected (right?).

It's a breaking change to the markup because whatever string you pass gets used, like hidden="false" instead of hidden="", but I don't think it's a breaking change functionally:

https://codepen.io/nhunzaker/pen/oeyVqM

Unless, of course, this is not consistent beyond my hidden example.

[sophiebits]

I think the meaningful difference is that today (with HAS_BOOLEAN_VALUE) '' is counted as false but it wouldn't be if we count the attribute as unknown. Everything else probably doesn't matter in practice.

[syranide]

#10521 (comment)

Note that this list is incorrect, autocomplete for example does NOT use true and false, but on and off. Same with autocorrect and probably one or two more.

I believe hidden shouldn't use true/false either but rather just attribute exists vs not exists.

[nhunzaker]

Note that this list is incorrect, autocomplete for example does NOT use true and false, but on and off. Same with autocorrect and probably one or two more.

Thank you! I've sent out a PR that addresses this (#10531)

I believe hidden shouldn't use true/false either but rather just attribute exists vs not exists.

Yep. hidden is a boolean property, assigning hidden="", or hidden="true" will both set the hidden property to true. However setting it to true/false in React and having it result in adding/removing an attribute is really convenient for boolean properties because avoid having to do things like hidden={this.props.hidden ? '' : null}.

So if we made this the default behavior, we could just add a flag that said "Always stringify this property", it would target 7 attributes: allowTransparency, contentEditable, draggable, spellCheck, autoReverse, externalResourcesRequired, and preserveAlpha.

[syranide]

Yep. hidden is a boolean property, assigning hidden="", or hidden="true"

Or hidden="anything", i.e. they should just exist. I guess my point is that some of the other properties expect "true", "false" or no attribute, but "no attribute" can be different from false/true (!). So even hidden shouldn't be conflated with e.g. spellcheck, IMHO spellcheck should really take a string, you can imagine spellcheck getting another enum value in the future to signify a different behavior. I'm assuming this is the reason why it's not considered a boolean attribute (that and being true by default).

So really, serializing true to "true" should perhaps never be done if you want to keep it consistent and prefer to avoid a whitelist. It does not seem weird to me to have to specify "true" as a value to spellcheck (as opposed to true).

[sebmarkbage]

The canonical way of expressing this is easy to find. You simply use the property instead of the attribute and then read the attribute. My preference is keeping the attribute consistent with what you'd get from using the property.

[aweary]

How does this high-level approach sound:

• We maintain the current whitelist for 16

• For enumerated attributes that rely on coercion right now, we add a warning if booleans are used. So spellCheck={true} would warn and advise to use spellCheck="true"

• For attributes currently whitelisted as HAS_BOOLEAN_VALUE we add a warning if they use a value that would be semantically different if we treated them as boolean attributes. So disabled="" would warn and advise to use disabled={false}

• For unknown boolean attributes we call setAttribute(name, "") for true and removeAttribute(name) for false. This would keep the attribute consistent with what you'd get from the assigning property. Alternatively, we can keep setting the full string value for true if there are concerns and move to using "" in 17.

• In 17+ we remove the warnings and the whitelist and assume a boolean value means a boolean attribute.

[jquense]

For enumerated attributes that rely on coercion right now, we add a warning if booleans are used. So spellCheck={true} would warn and advise to use spellCheck="true"

This makes the most sense but feels unfortunate :/ why is the DOM so dang wonky :P Would this silently fail in v17 tho?

[aweary]

Maybe we could keep that warning indefinitely? It shouldn't be too costly as a DEV-only check.

[robdodson]

The plan described by @aweary in #10521 (comment) sounds good to me.