facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.62k stars 46.43k forks source link

Allow custom (nonstandard) attributes. #140

Closed steida closed 7 years ago

steida commented 11 years ago

Various frameworks uses custom attributes. React could allow to extend default data- and aria- prefixes. Something like this:

React.registerCustomAttributePrefix('e-');
React.registerCustomAttributePrefix('ng-');
syranide commented 10 years ago

Cross-post from #1730

Have we considered something like an attrs-property? It would pass through any value as-is as an attribute for the DOM (same could be considered for a props-property, although it look kind of ambiguous).

I like it in the sense that it would work like data and aria is going to (soon), and it's use-case is actually very similar (i.e, use unsupported/unofficial attributes) so it kind of makes sense to me.

Swivelgames commented 10 years ago

@syranide That's definitely an option to consider, imo (for whatever its worth :P ). It would certainly contribute to making this much less ambiguous or esoteric for new users.

Swivelgames commented 10 years ago

@syranide, now, that being said, are you referring to using attrs- for standard properties? Or for React JS properties?

It might be a good idea (especially for users who aren't familiar with ReactJS) to use something like properties suffixed by react- for react-specific attributes. I'm not sure how big of a change we're talking about here, though, and this could be getting into a much larger discussion regarding how React JS essentially works at its core and how it introduces itself to developers, which is certainly not something I'm here trying instigate.

syranide commented 10 years ago

@Swivelgames Not sure if I understand, my intention with attrs-* (or rather attrs= to align with the coming data= and aria=) is that it would translate directly to attributes on the node. So <span attrs={{abc: 123}} /> would yield <span abc="123" />.

Swivelgames commented 10 years ago

OH, I see what you're saying. I apologize, I misunderstood :)

geelen commented 10 years ago

This is also an issue for interop between React and Custom Elements - something like an attrs property, that allowed passing through arbitrary properties and diffing them as simple strings seems like it would work well here.

janhancic commented 9 years ago

Do you guys know when this feature is going to land in React?

I'm playing around with node-webkit, which supports a custom attribute nwdirectory on <input type="file"/>s, that allow you to select folders. But I need to find a workaround as React strips it ...

syranide commented 9 years ago

@janhancic this.ref.myInput.getDOMNode().setAttribute('nwdirectory') in componentDidMount is a work-around until there's movement on this.

janhancic commented 9 years ago

Yep, that's what I came up with :) Thanks.

Aaronius commented 9 years ago

I don't think the workaround @syranide outlined will work in all the necessary cases. For example, I don't believe you can do:

var MyButton = React.createClass({
        displayName: 'MyButton',
        componentDidMount: function() {
            this.getDOMNode().setAttribute('is', 'my-button');
        },
        render: function() {
            return React.createElement('button', $.extend({}, this.props));
        }
    });

And have it be properly upgraded as a "my-button" custom element. I assume because we're setting the is attribute after the element is created and placed in the document. I'd be happy to hear if there's another way to deal with this particular case.

geelen commented 9 years ago

Good find @Aaronius! I'd love for an official way to do this before the element is added into the DOM

nfroidure commented 9 years ago

@syranide thanks for the trick.

As @Aaronius pointed out, it would be great to have something like a componentWillBeAttached(node:DOMNode) method in order to perform any action before its effective insertion in the DOM.

nfroidure commented 9 years ago

Sadly, the method described above doesn't work server side. A componentWillBeAttached as i mentionned above won't work either server side.

Is it feasible to create a componentWillRender method that would allow to modify outputted HTML either on server and the browser and allow us to simply solve the legacy/custom Tags/Attributes problem with a simple Mixin ?

nfroidure commented 9 years ago

Here is the solution i came accross https://github.com/SimpliField/react/commit/8861a9461c0f4dbac2c6dfb1bfe71a4d8c5fc356.

It allowed me to inject custom attributes for my needs with this piece of code:

var HTMLDOMLegacyPropertyConfig = {
  isCustomAttribute: function(attributeName) {
    return -1 !== [
      'align', 'bgcolor', 'border'
     ].indexOf(attributeName);
  },
  Properties: {
    align: null,
    bgcolor: null,
    border: null
  },
  DOMAttributeNames: {
  },
  DOMPropertyNames: {
  }
};

var React = require('react');

// Allow custom/legacy attributes for mail templates
React.Injection.DOMProperty.injectDOMPropertyConfig(HTMLDOMLegacyPropertyConfig);

Any better way to inject this ? If no, any plan on exposing ReactInjection on the React main object ?

robink commented 9 years ago

Just came across the same issue... The workaround mentioned by @syranide won't work on server side. The current situation prevents me from generating email HTML with legacy attributes like "align" "bgcolor".

steida commented 9 years ago

On the server side, you can use very dirty string replacement based workaround. Use data-fokfokfok prefix, and it will be safe enough. Still, React should and has to allow custom attributes, because Polymer. @sebmarkbage ?

robink commented 9 years ago

So you would need to do a workaround for client side and another one for server side. Not very neat... :( Too bad that HTMLDOMPropertyConfig is not easily configurable.

sebmarkbage commented 9 years ago

We want to move to a model where we render all attributes that you provide. Without a whitelist. There are a few concerns though.

It's a bit dangerous because we might need to change the meaning and signature of those attributes. E.g. as complex properties gets added to HTML, your code might break between versions. We prefer rich data types instead of strings.

We also have the issue of patterns like transferPropsTo used to transfer too many properties and people specified invalid HTML properties. These used to be silently ignored. We would need to provide a nice upgrade path for this case.

We could probably do it for web-components but not HTML or we could commit to always supporting string values and try to find an upgrade path for existing code.

gaearon commented 9 years ago

E.g. as complex properties gets added to HTML, your code might break between versions.

But isn't this true in web anyway (i.e. when not using React)?

IMO it's the expected behavior in the web that if HTML adds an attribute and I already use it, whether by accident or on purpose, something might break. Moreover it's not like attributes get added really often.

We also have the issue of patterns like transferPropsTo used to transfer too many properties and people specified invalid HTML properties.

Since transferPropsTo is deprecated, would this still be an issue?

sebmarkbage commented 9 years ago

@gaearon Yes, but properties are riskier. You're less likely to rely on a property (unless you patch prototypes) than an attribute. We try to model them as properties when possible.

geelen commented 9 years ago

I didn't follow this last comment @sebmarkbage, are you talking about more complex attribute values in custom elements, that can't be simply represented by strings? I'm not clear on how that would work, I'd have thought that all communication with the custom element needs to be done through the DOM, as strings?

Rather than rendering all attributes without a whitelist, maybe just adding a separate stringAttrs: { attr: val } object that can be used to explicitly declare a list of attributes to be treated as simple strings. That way simple attributes can be supported without breaking code, which is my use case and (from following this issue) seems like most of the others here?

sebmarkbage commented 9 years ago

For custom elements, we can only really support simple strings since we don't have any rich information about what they will be. For known HTML elements we can use rich data structures for style, classList, matrices for SVG attributes, etc. Maybe the solution is to simply use setAttribute if a string is provided when we also have rich property support. E.g. style would accept a string. Although that's a security risk so we might not allow that.

The nested stringAttrs property could work, but even then, it might not be great from a security perspective and not very elegant.

I think we can find a way to support all the things. My primary concern is the upgrade path. I have some ideas there.

We could replace existing uses of <div {...this.props} /> with a wrapper that only propagates the whitelist. E.g. <ConstrainedLegacyDiv {...this.props} />

syranide commented 9 years ago

@sebmarkbage You are definitely the authority on this, but to me it seems natural to otherwise simply add a dedicated prop for "setting attributes/properties with raw values on DOM nodes" (i.e. attrs={...}) which also comes with the understanding that whatever you provide it the attr gets set to.

It's probably not a very nice idea at all applied to custom elements, but then again, making the default implementation for all DOM elements non-whitelisted is definitely not nice either IMHO, especially as it conflates two very different behaviors without any hint as to which one you'll get.

Obviously, just custom elements could be "pass-through" and it kind of fits nicely with the fact that they too are uniquely named (apart from handful SVG nodes that conflict, unless they need their own namespace anyway?).

Anyway, just rambling here. :)

sebmarkbage commented 9 years ago

My rationale is that it is unlikely that a DOM property that accepts a string would have different behavior from the attribute. E.g. .setAttribute('class', str) has the same semantics as .className = str.

Except for the imperative quirks of what time they're mutated. Browsers have bugs or unintuitive behavior for when they update, and at which time you would want to invoke the setter. There's also internal state in the component that might update a property while leaving the attribute the same.

The point is that even if we added the special behavior for className later on, the behavior would be unaffected - except to fix unexpected life-cycle quirks. Having an attrs={...} escape makes it confusing how those differ from the other properties and they also don't get the bug fixes from the other properties.

nfroidure commented 9 years ago

I think we should think about what custom tags/attributes will be in the future. With web components adoption, i think they will be heavily used.

So if having a deep knowledge of properties is useful for 'traditional' HTML, why would it be different for the future HTML ?

That said, why not use the property when she is exposed and set the attribute when not. Allowing to inject your own rules like i suggested above seems to be the most universal solution.

It also could allow to put HTML/SVG properties definition in another repo that could be updated at a quicker pace. It would also decrease the sources size if you only use SVG or HTML or none of them.

The only downside of it is that the injection API would be harder to change since users will rely on it.

erkiesken commented 9 years ago

I ran across this "custom" attributes issue too, but with trying to render (standard) SVG. So I thought I'll add my real world example to this issue.

I was trying to use masks like so:

render: function () {
return <svg>
  <defs>
    <mask id="my-mask">
      <rect .../>
    </mask>
  </defs>
  <g mask="url(#my-mask)">
    ...
  </g>
</svg>;
}

First problem is that to make the url(#...) syntax work you have to have xmlns:xlink="http://www.w3.org/1999/xlink" attribute set on <svg> tag. And that is not whitelisted attr. OK I can add that with setAttribute() call in a componentDidMount function if needed.

But also the mask attribute is not whitelisted by React, so I'd need to go and modify all those DOM nodes one by one as well if I had many masked items rendered there.

Hopefully this shows that there are other and "standard" attributes that React is missing support for.

dandelany commented 9 years ago

+1 on some sort of solution to this - I think I prefer no attributes whitelist at all. There are a huge number of possible SVG attributes, many of which are currently unsupported. This makes it quite a pain to do anything beyond the most basic SVG rendering with React - which is a shame because I think SVG should be in React's wheelhouse. I've definitely run into the "mask" issue mentioned above before; this time though it's "markerWidth" and "markerHeight" that are causing problems for me.

jstrimpel commented 9 years ago

Would it be possible to introduce the concept of a config to support customization like this? Seems like this could be expanded to solve the SVG attributes issues as well by making isCustomAttribute for the SVG attributes config match all strings.

React.config({
  HTMLDOMPropertyConfig: {
    isCustomAttribute: RegExp.prototype.test.bind(/^(data|aria|lazo)-[a-z_][a-z\d_.\-]*$/),
    Properties: {
      'some-other-prop': null
    }
});

// would trigger ReactInjection.DOMProperty.injectDOMPropertyConfig(HTMLDOMPropertyConfig);
jimfb commented 9 years ago

@jstrimpel We want to avoid adding "configs" because they make things more complicated for users (more api surface area, more things that could go wrong / introduce component incompatibilities) and they make it more difficult for us (more permutations of conditions need to be tested). We know we want to get rid of the attributes whitelist anyway, so it makes no sense to introduce a "config" and then take it away in the subsequent release.

jimfb commented 9 years ago

Oops, didn't mean to close.

jstrimpel commented 9 years ago

@jsfb Thanks. +1 for removing the whitelist. Is there an ETA on that?

geelen commented 9 years ago

Awesome, removing the whitelist will fix my use case perfectly. Very interested to see what SVG wizardry you're cooking up @tehnomaag & @dandelany, I hadn't considered using React for SVG but it could be pretty cool!

jhicken commented 9 years ago

+1 For removing the whitelist.

Aaronius commented 9 years ago

I appreciate the removal of the tag name whitelist. That has allowed us to support many custom elements however it has not allowed us to support custom elements that extend from native elements which require the is attribute as described in my previous comment. A resolution on this would be super-duper-appreciated.

sophiebits commented 9 years ago

@Aaronius It's something we want to do. Until then, I'm happy to take a PR adding "is".

jhicken commented 9 years ago

@spicyj I would totally do a PR for something small like adding is but I don't think it would solve the entire custom element problem. So its more than just the is attribute. For example if you use the is attribute to extend an element and your extension uses a custom attribute named numberofplops then numberofplops would be removed and your custom element may fail to behave properly.

I can get around it for now using @nfroidure 's method but it make me really sad to do that. So any info about priority to remove the whitelist would be rad.

jimfb commented 9 years ago

@jhicken Currently we skip the whitelisting check if the component name contains a dash. To enable component inheritance, you would need to also skip the whitelist check if the component contained an is attribute; that would allow you to have arbitrary properties on your custom element.

sophiebits commented 9 years ago

@jsfb Do we? I know we talked about it but that's not what the code looks like:

https://github.com/facebook/react/blob/0183f70797183ae5371f61a40c1c13991cd7b104/src/browser/ui/ReactDOMComponent.js#L435-L441

jhicken commented 9 years ago

I just threw a commit together that does what @jsfb suggested. Have a peak let me know your thoughts.

Gozala commented 9 years ago

This would resolve an issue I've being struggling with for a while (see #2746 for details)

jhicken commented 9 years ago

So the now this stuff works on master I found an issue where if your using a custom component react does not translate className to class.

EX: <awesome-sauce className="fluffy" />

becomes

<awesome-sauce classname="fluffy" />

instead of

<awesome-sauce class="fluffy" />

What file makes this translation?

ljharb commented 9 years ago

This issue has been open for about 2 years - is there any chance of being able to use any attributes I wish in jsx for a general case?

sebmarkbage commented 9 years ago

@ljharb For custom elements, I.e. tags with dashes in them, yes. That has landed in master. For other tags, no. What is the use case? Is it an attribute with a dash in it? We might be able to support that.

@jhicken Custom elements uses "attributes" where as HTML nodes uses "properties". That's why they have different names. We don't intend to change that. It is just a consequence of custom elements relying heavily on attributes but the proper way to interact with DOM is through properties.

ljharb commented 9 years ago

No, no dashes. See https://help.pinterest.com/en/articles/prevent-pinning-your-site - the "nopin" attribute is what Pinterest requires to exempt an image from showing up in the "pin" dialog. This is nonstandard (shame on them), but has no dashes, and isn't something I can control.

How can I use this in jsx without creating a custom element?

jhicken commented 9 years ago

@ljharb You could use the the is attribute to force react to ignore the whitelist. The is attribute ignoring is also new in master. I think thats a really hacky way to use the is attribute but It would work. Just remember that if you put a class on the element with the is attribute you have to use class not className.

Like this. <img src="foo.jpg" is nopin="nopin" class="dontTazeMeBro" />

@sebmarkbage Thats too bad... I don't know enough about the separation of how react deals with attributes vs properties under the covers to really present a real solution to this problem. However I do see this as an api issue. I think as custom components become more popular. This is going to become really annoying to have a goofy mix of class and className attributes in your jsx. (I say this because I'm working on a project that makes heavy use of custom components) So this may be a naive question but, would it be possible when - is in element name or is attribute exist.. we convert the elements html attributes to react properties?

sebmarkbage commented 9 years ago

@jhicken We don't know what className might mean for a custom component. That might be the name of a legit attribute so if we were to convert it, we would now do some weird magic work that breaks that API. Unlike properties, attributes can only be strings so not all APIs can safely be converted. Additionally, you'll notice that event listeners won't work for these. Same for styles.

It is the opinion of the designers of web components and the spec that attributes is a mistake and should not be used directly. It is a serialized form. It shouldn't be used directly for anything other than serialized HTML, and then properties should take over. However, this is not really how most custom elements are consumed. The public APIs right now are mostly attributes. If properties are made available they're typically imperative methods that doesn't translate well to React's declarative API. We're forced to use attributes since it is the only thing that is possible for custom elements.

For consistency, the only solution would be to revert what we do for normal HTML elements and have them apply attributes instead of properties. That is more intuitive but strictly inferior since it doesn't allow us to properly model things like classList, transform, defaultValue vs. value, checked etc. These are not just attribute strings, they come with richer data structures or richer behavior differences than the serialized form allows for.

More over, it is our opinion (and the opinion of the React, Ember and Angular teams) that custom elements (Web Components) is a flawed model for composition and it won't be treated as first-class in either framework. There are too many compromises to make that work flawlessly.

We don't want to compromise the primary HTML API to support the custom elements edge case. So unfortunately, that's how we end up with this subpar API.

The recommended solution is to wrap your Web Components in a React component that can provide a richer API and use refs to interact with events / properties / methods. As part of that you can always translate the names as you see fit <my-component class={this.props.className} />.

@ljharb We would like to support a non-whitelist solution but not quite sure how to tackle it safely right now. As a workaround you can always get a ref on an element and call setAttribute('nopin', '') on it yourself. Ofc, this is annoying to do many times but best practice is to build small reusable components that you compose, if you do that then you should only need to use this hack once.

Btw, I will address these issues in my React Europe talk on July 2nd.

dantman commented 9 years ago

@sebmarkbage

@ljharb We would like to support a non-whitelist solution but not quite sure how to tackle it safely right now. As a workaround you can always get a ref on an element and call setAttribute('nopin', '') on it yourself. Ofc, this is annoying to do many times but best practice is to build small reusable components that you compose, if you do that then you should only need to use this hack once.

This probably won't work in this case, as Pintrest is probably not executing client side JS and instead simply statically analyzing <img> tags for a nopin attribute.

sebmarkbage commented 9 years ago

Oh, this is for server-side rendering? Yea, that sucks. Maybe Pinterest should be nicer community members and use the data- namespace like it was intended. :(

sophiebits commented 9 years ago

Oh, I assumed this was for the browser extension that adds "Pin It" buttons to everything.

dantman commented 9 years ago

Oh, this is for server-side rendering? Yea, that sucks. Maybe Pinterest should be nicer community members and use the data- namespace like it was intended. :(

Yeah, for that use case. Though data attributes also sound wrong. This would be easier if <img> had something like rel so rel="nopin" would be viable.