Boolean attributes no longer coerced to string#9779
Boolean attributes no longer coerced to string#9779klinsley wants to merge 3 commits intofacebook:masterfrom
Conversation
|
I think I would be OK with this, semantically speaking it's correct for boolean attributes, and practically speaking I believe it will maintain the same behavior. cc @spicyj what do you think? |
| } else if (value === true) { | ||
| node.setAttribute(name, ''); | ||
| } else if (value === false) { | ||
| node.removeAttribute(name); |
There was a problem hiding this comment.
This change worries me. This would be breaking behavior since we currently just stringify false and this would potentially change runtime behavior. I think we should leave this specific part as-is, where null is used to identify attributes that should be removed.
There was a problem hiding this comment.
E.g. spellcheck is not a boolean attribute and expects the value 'true' or 'false', if anyone relies on boolean being stringified today it will break for them (I assume? or is that another codepath). Also note that neither 'true' nor 'false' is the default value for it.
There was a problem hiding this comment.
Are there any other enumerated attributes like that which use stringified booleans? I would hate to special case them, but I also think it's reasonable to ask users to provide the expected stringified values themselves, e.g., spellcheck="true". We could have dev-only warnings for that maybe.
There was a problem hiding this comment.
Also, using "true" and "false" but not making it a boolean attribute seems like a really confusing decision 🤔
There was a problem hiding this comment.
I don't know if there is any cross over with #9806, but I'm curious, if we move forward with this, how value should be treated.
We have a special mutation method for value that might need to be synced up with this new logic, but as far as I see it, value={true} should stringify to "true", which is what would happen if you mutated the input directly.
| DOMPropertyOperations.setValueForProperty(stubNode, 'data-foo', false); | ||
| expect(stubNode.getAttribute('data-foo')).toBe(null); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Is this true? No matter how I assign data attributes, they always appear, regardless of boolean values:
There was a problem hiding this comment.
That is correct; DOMStringMap (the backing type for .dataset) always only holds string values.
|
I'm closing since this is stale and there was no consensus. Appreciate the PR! |
7 participants
From #9230, this abides by the HTML5 spec mentioned in the issue. This issue was left in an unsure state and I wanted to try my first contribution.