Fix boolean element attributes as per HTML5 spec #993

jjt · 2014-01-29T22:22:39Z

No description provided.

jjt · 2014-01-29T22:23:15Z

Fixes #961

syranide · 2014-01-30T09:13:02Z

src/dom/DOMPropertyOperations.js

@@ -94,6 +94,9 @@ var DOMPropertyOperations = {
      if (shouldIgnoreValue(name, value)) {
        return '';
      }
+      if (value === true && DOMProperty.hasBooleanValue[name]) {


Shouldn't that just be:

if (value && DOMProperty.hasBooleanValue[name]) {

If it's a boolean, then it's a boolean.

I think it has to be if a value is strictly true, and not just truey. Otherwise a value of "myAttrValue" would emit a bare attribute. I guess that might be favourable to force HTML5 compliance. But it takes away the ability to target XHTML with disabled="disabled" (if that matters).

@jjt The issue I see with only strictly true values is that most things people use as booleans are often not strictly true or false, I mean even Var1 || Var2 could be anything.

As for XHTML, I'm not sure if React is even compatible with it anyway (and who even uses it, hehe), if that is something we strive to support (do we?) then I would argue that it's better to detect it and always mirror the key in the value, rather than this weird in-between.

What? People play it fast and loose with js types?!!?

I tend to agree with you though inre: no XHTML support. It would fit with the current system of passing a falsey value to omit an attribute.

I say screw XHTML support and go bare. We've already decided we aren't ignoring the value (shouldIgnoreValue), so I think we should do it for all hasBooleanValue properties.

syranide · 2014-01-30T09:14:00Z

@jjt Have you double-checked that all attributes marked as HAS_BOOLEAN_VALUE actually are and support bare attributes?

jjt · 2014-01-30T19:29:16Z

@syranide Yup, I went through the attributes and cross-referenced them with the ones in the spec.

zpao · 2014-01-31T01:38:37Z

src/dom/DOMPropertyOperations.js

@@ -94,6 +94,9 @@ var DOMPropertyOperations = {
      if (shouldIgnoreValue(name, value)) {
        return '';
      }
+      if (value === true && DOMProperty.hasBooleanValue[name]) {
+        return escapeTextForBrowser(name);


We should be using attributeName just in case they don't match up.

Right, good call.

key should never be index into an array or there are bugs. Especially in transitions. Fixes facebook#853

We've been able to use `querySelectorAll` in all the browsers that we support for some time now, but we haven't been able to test code that uses it in the older version of `jsdom` that we were using, until recently. Besides the general goal of modernizing our code, the impetus for this specific change is that I'm trying to support testing without having to render nodes into an actual document. The `.getElementsByName` method is only defined on `document` and only works if the nodes you care about are contained by the document. On the other hand, `querySelectorAll` works on any DOM node, and allows a more precise selection of just the `<input type="radio">` elements that have the appropriate name. IE8's implementation of `querySelectorAll` supports attribute-based selectors, which is all we need here.

Numeric keys will be ordered sequentially in Chrome/Firefox. Warn the user if such keys are set on a child.

We're not handling these correctly.

Number('.1') === 0.1, and react uses dot-prefixed keys for children. Whoops. Nuke the non-numeric requirement, and just check a regex. This seems performant enough in micro-benchmarks: http://jsperf.com/numericlike

Recently learned that components passed into `React.renderComponent` may not be the ones actually mounted. Also learned that it returns the mounted component. Added some documentation describing this.

Let's be consistent with the naming

jjt · 2014-01-31T22:09:26Z

Alright, it's using attributeName and truthy testing now.

syranide · 2014-01-31T22:56:01Z

@jjt Saw that you proposed using querySelectorAll instead of getElementsByName, I would guess that querySelectorAll is significantly slower than getElementsByName though.

zpao · 2014-01-31T23:06:12Z

@syranide I think that's from a rebase or something, we checked that in already. Though something funky is going on.

@jjt can you rebase so we only see your changes here?

jjt · 2014-01-31T23:19:49Z

@zpao Yeah, I don't know what happened with the commits. Will fix.

@syranide That was from mangled rebasing - I only pushed two commits, the rest are from me pulling in changes from master.

jjt · 2014-01-31T23:55:06Z

Nuking this one, see #1005

syranide · 2014-01-31T23:57:52Z

@jjt FYI, if you run into this again, git reset --hard can be used to reset the branch.

jjt · 2014-02-01T00:02:04Z

@syranide Thanks, I'll look into that. I know just enough about git workflows to get myself into trouble, so it would be good to know how to dig myself out.

syranide · 2014-02-01T00:03:42Z

@jjt #459, we've all been there! :)

jjt · 2014-02-01T00:25:39Z

@syranide Heh, thanks for the reassurance!

Fix boolean element attributes as per HTML5 spec

ec17e37

syranide reviewed Jan 30, 2014
View reviewed changes

zpao reviewed Jan 31, 2014
View reviewed changes

petehunt and others added 10 commits January 31, 2014 13:37

remove references to react-page

6ec9c62

Fix animation example code

9649d67

key should never be index into an array or there are bugs. Especially in transitions. Fixes facebook#853

Warn for numeric object properties

3cf8b26

Numeric keys will be ordered sequentially in Chrome/Firefox. Warn the user if such keys are set on a child.

fix grammar mistake

c7c62ee

Support children and ref for cloneWithProps()

d749b19

We're not handling these correctly.

Fix warning for numeric properties

6ca23ae

Number('.1') === 0.1, and react uses dot-prefixed keys for children. Whoops. Nuke the non-numeric requirement, and just check a regex. This seems performant enough in micro-benchmarks: http://jsperf.com/numericlike

Add documentation about React.renderComponent

815ac01

Recently learned that components passed into `React.renderComponent` may not be the ones actually mounted. Also learned that it returns the mounted component. Added some documentation describing this.

s/Mock DOM/Virtual DOM/

157b87e

Let's be consistent with the naming

Boolean attributes use truthy value instead of strict true

16b6ddc

jjt closed this Jan 31, 2014

jjt deleted the boolean-attributes branch April 6, 2016 17:26

invincibleJai mentioned this pull request May 14, 2019

fix(git-import): updates spec for git import talamer/console#92

Merged

ryan-ally mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.3.0 to 9.1.3 ryan-ally/react-1#238

Open

jackromo888 mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.1.0 to 9.1.3 jackromo888/react#245

Open

AlexanderGrovnes mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.1.0 to 9.1.3 AlexanderGrovnes/react#40

Open

ryan-ally mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.3.0 to 9.1.3 ryan-ally/react-1#239

Open

DevProp mentioned this pull request Jul 8, 2023

[Snyk] Security upgrade babel-loader from 8.1.0 to 9.1.3 DevProp/react#40

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix boolean element attributes as per HTML5 spec #993

Fix boolean element attributes as per HTML5 spec #993

jjt commented Jan 29, 2014

jjt commented Jan 29, 2014

syranide Jan 30, 2014

jjt Jan 30, 2014

syranide Jan 30, 2014

jjt Jan 30, 2014

zpao Jan 31, 2014

syranide commented Jan 30, 2014

jjt commented Jan 30, 2014

zpao Jan 31, 2014

jjt Jan 31, 2014

jjt commented Jan 31, 2014

syranide commented Jan 31, 2014

zpao commented Jan 31, 2014

jjt commented Jan 31, 2014

jjt commented Jan 31, 2014

syranide commented Jan 31, 2014

jjt commented Feb 1, 2014

syranide commented Feb 1, 2014

jjt commented Feb 1, 2014

Fix boolean element attributes as per HTML5 spec #993

Fix boolean element attributes as per HTML5 spec #993

Conversation

jjt commented Jan 29, 2014

jjt commented Jan 29, 2014

syranide Jan 30, 2014

Choose a reason for hiding this comment

jjt Jan 30, 2014

Choose a reason for hiding this comment

syranide Jan 30, 2014

Choose a reason for hiding this comment

jjt Jan 30, 2014

Choose a reason for hiding this comment

zpao Jan 31, 2014

Choose a reason for hiding this comment

syranide commented Jan 30, 2014

jjt commented Jan 30, 2014

zpao Jan 31, 2014

Choose a reason for hiding this comment

jjt Jan 31, 2014

Choose a reason for hiding this comment

jjt commented Jan 31, 2014

syranide commented Jan 31, 2014

zpao commented Jan 31, 2014

jjt commented Jan 31, 2014

jjt commented Jan 31, 2014

syranide commented Jan 31, 2014

jjt commented Feb 1, 2014

syranide commented Feb 1, 2014

jjt commented Feb 1, 2014