Support arbitrary attributes on elements with dashes in the tag name. #3067

jimfb · 2015-02-05T20:18:56Z

Support arbitrary attributes on elements with dashes in the tag name.

sophiebits · 2015-02-05T20:26:12Z

src/browser/ui/dom/DOMPropertyOperations.js

+      if (value == null) {
+        return '';
+      }
+      return name + '=' + quoteAttributeValueForBrowser(value);


We call quoteAttributeValueForBrowser, which calls escapeTextContentForBrowser. Is that not good enough?

What are you worried about? Users controlling the property name?

Yeah – even if that's not likely to be idiomatic code, it's surprising if an odd attribute name results in a security hole. You can see that we already check the tag name when generating markup.

ok, cool, I'll add an invariant.

jimfb · 2015-02-06T23:45:12Z

cc @sebmarkbage

sebmarkbage · 2015-02-06T23:52:58Z

src/browser/ui/ReactDOMComponent.js

@@ -235,7 +235,7 @@ ReactDOMComponent.Mixin = {
          propValue = CSSPropertyOperations.createMarkupForStyles(propValue);
        }
        var markup =
-          DOMPropertyOperations.createMarkupForProperty(propKey, propValue);
+          DOMPropertyOperations.createMarkupForProperty(this._tag, propKey, propValue);


This is leaking unnecessarily much information to DOMPropertyOperations. All the tag name logic is already in this file.

I'd rather see the switching done in this file where we already have a regexp for tag names. We might as well use that first check to cache if we're a custom element or not. That way you don't have to recheck for every attribute and every update.

sebmarkbage · 2015-02-06T23:55:15Z

If you do the branching in DOM component you can just have a separate methods for createMarkupForCustomAttribute and setValueForCustomAttribute so that we have a separate code path for those. Avoids the boolean-as-argument problem.

jimfb · 2015-02-10T00:04:27Z

@sebmarkbage better?

syranide · 2015-02-10T10:29:48Z

May not be relevant right now, but does this play nice with SVG's <font-face> etc?

jimfb · 2015-02-10T17:45:18Z

Yeah, it'll potentially pass additional properties to font-face if they're transferring props, but shouldn't be a huge problem. If anyone is doing that (I think it's relatively rare), SVG should either ignore the additional attributes or give an error message; either way, it sounds like a fine migration story.

I suppose we could special case that tag if anyone really cares. I feel like it's not necessary, but @sebmarkbage tends to prefer being super cautious, so I'll let him have the final say.

sebmarkbage · 2015-02-10T17:47:26Z

We might need to special case it when we support more SVG attributes, but for this case we're already using setAttribute so it should be fine. Don't think people are transferring random props to font-face very often and even if they do, it shouldn't hurt.

gaearon · 2015-02-10T18:10:14Z

src/browser/ui/ReactDOMComponent.js

-        var markup =
-          DOMPropertyOperations.createMarkupForProperty(propKey, propValue);
+        var markup = null;
+        if(this._tag != null && this._tag.indexOf('-') >= 0) {


There's a missing space after if

jimfb · 2015-04-27T23:43:54Z

I don't see a todo item on this PR; am I missing something? Is there a blocking item, or are we just waiting for a CR?

sophiebits · 2015-04-27T23:46:39Z

src/browser/ui/dom/DOMPropertyOperations.js

+   * @return {?string} Markup string, or null if the property was invalid.
+   */
+  createMarkupForCustomAttribute: function(name, value)
+  {


{ on previous line

zpao · 2015-04-28T20:40:28Z

src/browser/ui/ReactDOMComponent.js

+    );
+    validatedAttributeNameCache[attributeName] = true;
+  }
+  return attributeName;


If we keep this we don't need the return value

It's a style that allows you to add validation while passing a value through, similar to function chaining (where a builder returns its self). Otherwise, the function is just void, so we don't loose anything by returning the value. This pattern is pretty common in other languages, but we can remove it if it isn't common javascript.

I understand the style but if we don't use it like that, there's no need to do it.

Ok, fixing now.

zpao · 2015-04-28T22:12:26Z

src/browser/ui/dom/DOMPropertyOperations.js

+var validatedAttributeNameCache = {};
+
+function validateDangerousAttributeName(attributeName) {
+  if (!hasOwnProperty.call(validatedAttributeNameCache, attributeName)) {


hasOwnProperty doesn't exist here…

jimfb · 2015-04-28T23:40:11Z

Switched to warn instead of throw for invalid attribute name.

jimfb · 2015-04-30T18:23:37Z

cc @zpao Anything tasks remaining?

jimfb · 2015-05-08T20:28:14Z

@zpao Ping.

jimfb · 2015-06-02T00:54:41Z

Ping @zpao.

zpao · 2015-06-02T18:34:47Z

src/renderers/dom/shared/DOMPropertyOperations.js

@@ -17,6 +17,30 @@ var DOMProperty = require('DOMProperty');
 var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser');
 var warning = require('warning');

+var VALID_ATTRIBUTE_NAME_REGEX = /^[a-zA-Z_][a-zA-Z_\.\-\d]*$/; // Simplified subset


nit: move comment to own line above

jimfb · 2015-06-02T19:15:51Z

@zpao updated.

zpao · 2015-06-03T18:00:37Z

👍

…ments Support arbitrary attributes on elements with dashes in the tag name.

sophiebits reviewed Feb 5, 2015
View reviewed changes

jimfb force-pushed the arbitrary-attributes-for-dash-elements branch from 076a97a to c841cf6 Compare February 6, 2015 00:14

sebmarkbage reviewed Feb 6, 2015
View reviewed changes

sebmarkbage added the GH Review: needs-revision label Feb 6, 2015

jimfb force-pushed the arbitrary-attributes-for-dash-elements branch from c841cf6 to 2f442e6 Compare February 9, 2015 23:53

jimfb added GH Review: review-needed and removed GH Review: needs-revision labels Feb 9, 2015

gaearon reviewed Feb 10, 2015
View reviewed changes

jimfb force-pushed the arbitrary-attributes-for-dash-elements branch from 2f442e6 to d70f53c Compare February 10, 2015 20:29

facebook-github-bot added the CLA Signed label Apr 7, 2015

zpao mentioned this pull request Apr 27, 2015

Ignore whitelisted attributes for native custom elements. #3752

Merged

sophiebits reviewed Apr 27, 2015
View reviewed changes

sophiebits added GH Review: needs-revision and removed GH Review: review-needed labels Apr 27, 2015

jimfb force-pushed the arbitrary-attributes-for-dash-elements branch from d70f53c to 89f9c44 Compare April 28, 2015 01:23

reactjs-bot added GH Review: review-needed and removed GH Review: needs-revision labels Apr 28, 2015

zpao reviewed Apr 28, 2015
View reviewed changes

jimfb force-pushed the arbitrary-attributes-for-dash-elements branch 2 times, most recently from ad30a5a to 9561a01 Compare April 28, 2015 23:39

sophiebits mentioned this pull request Apr 30, 2015

Missing SVG tags and attributes #1657

Closed

jimfb force-pushed the arbitrary-attributes-for-dash-elements branch from 9561a01 to 90ba511 Compare April 30, 2015 20:49

jimfb mentioned this pull request May 22, 2015

Added 'is' attribute for custom elements #3930

Merged

jimfb force-pushed the arbitrary-attributes-for-dash-elements branch 3 times, most recently from 45e7c20 to 2d2468b Compare June 2, 2015 00:49

zpao reviewed Jun 2, 2015
View reviewed changes

Support arbitrary attributes on elements with dashes in the tag name.

b1db817

jimfb force-pushed the arbitrary-attributes-for-dash-elements branch from 2d2468b to b1db817 Compare June 2, 2015 19:15

sebmarkbage mentioned this pull request Jun 2, 2015

Umbrella 0.14 #3220

Closed

25 tasks

zpao added GH Review: accepted and removed GH Review: review-needed labels Jun 3, 2015

jimfb added a commit that referenced this pull request Jun 3, 2015

Merge pull request #3067 from jimfb/arbitrary-attributes-for-dash-ele…

9a02ea2

…ments Support arbitrary attributes on elements with dashes in the tag name.

jimfb merged commit 9a02ea2 into facebook:master Jun 3, 2015

syranide mentioned this pull request Sep 22, 2015

className prop not working with custom DOM elements in 0.14.0-rc1 #4933

Closed

davidkpiano mentioned this pull request Oct 21, 2015

Updating "JSX Gotchas" docs for Custom Attributes #5231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support arbitrary attributes on elements with dashes in the tag name. #3067

Support arbitrary attributes on elements with dashes in the tag name. #3067

jimfb commented Feb 5, 2015

sophiebits Feb 5, 2015

jimfb Feb 5, 2015

sophiebits Feb 5, 2015

jimfb Feb 5, 2015

jimfb commented Feb 6, 2015

sebmarkbage Feb 6, 2015

sebmarkbage commented Feb 6, 2015

jimfb commented Feb 10, 2015

syranide commented Feb 10, 2015

jimfb commented Feb 10, 2015

sebmarkbage commented Feb 10, 2015

gaearon Feb 10, 2015

jimfb commented Apr 27, 2015

sophiebits Apr 27, 2015

zpao Apr 28, 2015

jimfb Apr 28, 2015

zpao Apr 28, 2015

jimfb Apr 28, 2015

zpao Apr 28, 2015

jimfb Apr 28, 2015

jimfb commented Apr 28, 2015

jimfb commented Apr 30, 2015

jimfb commented May 8, 2015

jimfb commented Jun 2, 2015

zpao Jun 2, 2015

jimfb commented Jun 2, 2015

zpao commented Jun 3, 2015

Support arbitrary attributes on elements with dashes in the tag name. #3067

Support arbitrary attributes on elements with dashes in the tag name. #3067

Conversation

jimfb commented Feb 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimfb commented Feb 6, 2015

Choose a reason for hiding this comment

sebmarkbage commented Feb 6, 2015

jimfb commented Feb 10, 2015

syranide commented Feb 10, 2015

jimfb commented Feb 10, 2015

sebmarkbage commented Feb 10, 2015

Choose a reason for hiding this comment

jimfb commented Apr 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimfb commented Apr 28, 2015

jimfb commented Apr 30, 2015

jimfb commented May 8, 2015

jimfb commented Jun 2, 2015

Choose a reason for hiding this comment

jimfb commented Jun 2, 2015

zpao commented Jun 3, 2015