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

[hellosmithy]

The className prop does not appear to get mapped correctly when applied to custom DOM elements in 0.14.0-rc1.

JSFiddle: https://jsfiddle.net/hellosmithy/5pdujnfq/1/

[syranide]

It works if you set class instead.

cc @jimfb perhaps? (#3067 is related I assume)

[hellosmithy]

@syranide I think you're right that the #3067 commit introduced the issue

https://github.com/facebook/react/blob/v0.14.0-rc1/src/renderers/dom/shared/ReactDOMComponent.js#L637-L641

Any custom element will match the isCustomComponent check and skip all the DOM property operations.

I'm struggling to understand the use-case for #3067. Surely attributes should work the same for custom elements and standard DOM elements?

[syranide]

I'm struggling to understand the use-case for #3067. Surely attributes should work the same for custom elements and standard DOM elements?

I'm not very familiar with all this, but there's a difference between custom tags <customtag> and web components <web-component>. Web components are much like React components and aren't themselves visible nodes, so the list of DOM attributes don't inherently apply to them (again, just like it makes little sense passing DOM properties to React components), whereas they do for custom tags which just translate to unstyled DOM nodes.

I assume <custom-tag> was previously valid too, but as of the introduction of web components that's no longer true? This is my limited understanding of all this anyway, so don't take my word for it!

[zpao]

cc @jimfb

[jimfb]

@syranide is correct, there is a difference between DOM nodes and webcomponents (which are much more like React components). For standard DOM nodes, we know a priori that there is no className attribute, so we can commandeer the namespace for our own purposes. For web components, there is no such guarantee, so we can't steal the namespace. In fact, web component authors may prefer className over class for the same reasons we stole it for standard dom components, and to remain compatible, we need to pass it through unmolested.

Another way of thinking about this: For standard DOM nodes, we conceptually have a react-component defined for each node, that happens to take className, just as we happen to take defaultValue for uncontrolled form elements and happen to use camel case for attributes. We happen to follow some consistent naming conventions in our framework-provided components. The fact that we do most of our markup transformations at the framework level instead of inside react-component definitions is an implementation detail. Web component authors are free to follow those same conventions (or not), but we need to pass them through without transformations or we'll confuse the web components.

TLDR: Custom elements DO work in React. You must use class instead of className because the custom element spec requires that we allow users to specify a className attribute and we need to preserve that functionality for custom elements.

[hellosmithy]

I agree there's a distinction between custom elements and web components. Custom elements are one of the building blocks of web components but they are a spec in their own right. But there's a use case for creating semantically meaningful custom elements which don't extend default DOM behaviour.

Wouldn't a more consistent approach be to treat all elements the same within React but allow either users to escape attributes somehow or alternatively they have the option to dangerouslySetInnerHTML if they are indeed using web components that need to operate outside the React scope?

[jimfb]

@hellosmithy Using a component with a dash is the escape. As per the specification, a node with a dash in it may have arbitrary attributes defined.

[hellosmithy]

@jimfb I'm not sure that's a safe assumption to make. My understanding is that a custom element must have a dash in it. Custom elements are used in web components. But an element with a dash in it is not necessarily a web component.

https://developer.mozilla.org/en-US/docs/Web/Web_Components/Custom_Elements
"They are part of Web Components but they can also be used by themselves."

[jimfb]

Semantics. Custom element is a subset of the web component specification. We use the term "web component" loosely, but I believe the statement holds if you replace "web component" with "custom element"; namely that the custom element may define arbitrary attributes and process those attributes using arbitrary rules.

[hellosmithy]

@jimfb I think this is a little more than semantics. Either way this change prevents custom elements from being interoperable with core dom elements, when surely there are other options such as escaping special case attributes (className etc) for cases where people need to and thus not create a breaking change. Unless there is another way to use custom elements as purely custom elements and still use React props?

[jimfb]

You'd have to special case/escape a whole ton of attributes including every possible camelcase attribute that could ever (past, present, or future) occur on a native DOM component, not to mention dealing with event handlers and other complexities. It would not be as simple as escaping a couple special-cased attributes; the rules would be complex and confusing (certainly more so than the current attribute whitelisting) - I can't see a way to do such a thing in a way that's better than what we currently do (pass attributes straight through).

[hellosmithy]

I'm suggesting there should be a discussion on the best way to do it, escaping may not be it.

One way would be an escape pattern convention e.g. myPropEscaped (just a straw man example). But I'm also wondering, if you want to create web components that are outside of React's scope surely that is a case for using dangerouslySetInnerHTML, I would have thought that the default behaviour should be interoperability otherwise there's no way to apply React attributes such as className to custom elements, which seems an odd and inconsistent restriction. On the other hand there are other ways to pass through web component attributes in those cases where you might want to, and in those cases surely you should be explicitly saying "I don't want React to do the default behaviour in this instance", rather than it just automatically making that assumption for you.

Looking at the JSX code:

<div className="myClass" />
<div is="custom-elem" className="myClass" />
<custom-elem className="myClass" />

It seems right to expect those to behave consistently.

[hellosmithy]

@jimfb @zpao has there been any more thought on this?

Having read the thread at #140 I can see why this direction was taken, but I think it raises inconsistencies and will make code harder to reason about. I really do think escaping is the more consistent and flexible option, and forces code to be explicit if it's breaking React convention. Would be interested to hear what your thoughts are.

[jimfb]

@hellosmithy I can't see how to make it work in a way that's better than what we currently have. If you have an escaping solution that you'd like to propose, we'd be happy to take a look.

The constraints are:

• Must allow people to specify all possible attributes in a case sensitive way. Users must be able to specify any combination of class and className (zero, one, or both). Users must also be able to specify any combination of onclick and onClick (zero, one, or both). This is because custom elements can impose arbitrary processing rules on their attributes, and case sensitivity matters.
• Must be intuitive for users. Ideally, the first thing people try should work. If it doesn't work, users must have an obvious "next step" to try so they can fix their code (ie. what happens when an attribute they wanted to use gets rewritten? do they get a useful error message? how do they intuitively know what to try next?).
• Must allow us to do basic sanity checks (like preventing/mitigating XSS attacks) in a way that minimizes the impact on performance.
• Should not require modifications to the JSX syntax, which is already becoming a defacto standard as other languages/editors/IDEs/parsers understand and implement the syntax, so any changes would break all of them. You can read the specification here: https://facebook.github.io/jsx/
• Should play nice with Allow custom (nonstandard) attributes. #140 and futureproof us from maintaining an ever-growing whitelist as browsers add new elements/attributes.

[hellosmithy]

@jimfb @sebmarkbage I'm happy to submit a PR for escaping attrs for web components. However this seems like a separate issue. The changes that have gone through in this release candidate make it difficult to reason about how React will treat different DOM elements in JSX. The onus should be on those wanting to use web components with their own attribute API's to do so in a way that operates safely with existing React behaviour including the way it treats custom elements.

The changes also seem to contradict what is said here by @sebmarkbage: #5052 (comment)

"The primary component strategy for React will never be Web Components."

So it seems odd to me that a breaking change is being introduced here to appease those that want to use web components inside their React components. While I understand the original sentiment, this feels like the wrong solution is being rushed through, and I worry that once it's gone through into a release it will be 10x harder to argue for it to come out again.

[gargantuan]

This breaking change has to get a big thumbs down from me. Custom elements are useful today. Web Components are not and probably never will be. It's frustrating to lose the semantic goodness of custom elements for no practical reason that helps developers today.

[cosminnicula]

+1 for Custom Elements support

[mikeybox]

The problem here is actually the determination of what a developer intends by using a custom component. Here you're assuming that a custom html tag will become a web-component and you decide non intuitive attribute behaviour. In a lot of cases custom html elements are used to provide meaning to the HTML based components. The fix here would be to have the option of setting the the behaviour. Here - ReactDOMComponent.js#L637-L641 - You're testing for a custom component and then deciding that it means a need for custom attribute handling. This is wrong and should be in the hands of developers not framework assumptions. it could easily be set in the component before render is called if this component is intended for promoting as a web-component. This way you remove the need for a breaking change and at the same time provide the functionality you want on a developer chosen basis.

[jimfb]

To be clear: Custom elements DO work in React. You must use the class attribute instead of className because the custom element spec requires that we allow users to specify arbitrary attributes (including className) and we need to preserve that functionality for custom elements.

There is no conflict with Sebastian's comment in #5052 (comment). Web components are NOT our primary use case, but we will still do the best we can to support the DOM technologies for anyone who wants to use those technologies. Same goes for custom elements. This is why we've added support for them, while consistently recommending you use React composite components instead.

If someone would like to propose a solution that is better than the one we've implemented, subject to the constraints I mentioned above, we'd be more than happy to take a look.

[hellosmithy]

To be clear: Custom elements DO work in React.

@jimfb they may work in the sense that they can be rendered to the DOM, but they will not work with React attributes. Even if we put the inconsistency of using class sometimes and className other times aside for a moment, there is a bigger issue here. None of the React events will work such as onClick, onDrag etc. Effectively you are leaving "React land" by using a custom element.

I think @mikeybox hit the nail on the head when he said this should be in the hands of developers not the framework.

[gargantuan]

I'd like to propose an <AttributeEscape/> wrapper component. I'd advocate this solution for 5 reasons.

1. It's declarative
2. No "magic"
3. It's idiomatic React
4. Doesn't break custom-elements
5. Does not need to be part of the react library.

So how would this work? It would intercept any child elements, create new DOM elements, assign the props unchanged and dangerously set inner HTML.

You could also create a white/black list feature giving the developer total control over how attributes are handled.

The beauty of this solution is that React has already solved this problem. It can be a third party solution and does not need to be part of the react library.

Here's a couple of examples

React Component

// 1
<my-image-component className="someClass" src="someSrc" />

Web Component

All attributes live in WebComponent land

// 2
<AttributeEscape>  
    <my-image-component className="someClass" src="someSrc" />  
</AttributeEscape>

In this example, we actually want className to behave as normal.

// 2.1
<AttributeEscape ignore={['className']}>  
    <my-image-component className="someClass" src="someSrc" />  
</AttributeEscape>

Conversely, you could have an include prop, which would trigger AttributeEscape wrapper to ignore all props by default, and only escape the ones we want to include.

// 2.2
<AttributeEscape include={[’src']}>  
    <my-image-component className="someClass" src="someSrc" />  
</AttributeEscape>

The important idea to take away is: The Web Components solution should not be part of React. It should be a third party solution.

[benoitgrelard]

I haven't personally used Web Components/Custom Elements alongside React.
However, reading @jimfb:

Web components are NOT our primary use case

To me, this makes it even more obvious that React should NOT be handling this itself and should instead leave it to developers / third party (something like @gargantuan suggests).

If it is not the primary use case for React, as a developer, if I know that, and I still put myself in a situation where I want to use both at the same time, then I am prepared to go through a few hoops.

I think it would make sense for React to stay as is, and let developers decide what they want to do.

[mikeybox]

@jmfb:

the custom element spec requires that we allow users to specify arbitrary attributes

I think we all agree that this is correct. A developer should be allowed to pass all parameters and that is fine. The key here is allow and not enforce. In its current state you're not able to choose and that is the problem. Custom HTML elements do not require this behaviour to be used and essentially most developers will not want to lose basic React element behaviour when using a custom element. By it's very nature custom elements can cover a vast range of intended uses.

The simple conclusion is that yes it is right to enable this raw attribute mode.
But it should most definitely be a developer choice to opt in to the functionality. This can be either a new component type or a Class property setting.

[jimfb]

@gargantuan That does not allow custom elements to play as first class citizens. You've effectively turned React into a templating system at that point because your AttributeEscape component destroys the virtual dom. You are eliminating React's ability to do diffing (because you convert the virtual dom to dangerouslySetInnerHTML), eliminate the ability for the parent components to make decisions based on the children, eliminate the ability to attach refs and event handlers, etc.

[gargantuan]

I think I've not clearly explained how this one example proposal would work. I stress that this shouldn't be part of React. You would only put a web-component inside an AttributeEscape component.

That said, I think it's wrong to focus on my half baked proposal and to ignore the wider problem that others have raised.

1. Is it Reacts responsibility to deal with web components?
2. Should developers have a choice about how custom elements are handled?

In addition, my proposal has two saving graces:

1. It doesn't break anything.
2. Web Component users are still free to implement their own solution.

Finally - I would plead with you to at least deprecate custom elements and issue a warning before releasing this breaking change. I think that would give the wider community the chance to feed back on change that I'm sure will surprise them.

[jimfb]

@gargantuan as per my comment above, it means that elements within AttributeEscape will not play by the vdom rules. This DOES break lots of things. And it's a huge problem if your render tree has an AttributeEscape with an arbitrary tree of children placed inside.

[hellosmithy]

@jimfb the solution suggested by @gargantuan is one option, with it's own merits and considerations. It probably deserves it's own PR and discussion thread.

The point being made here by me and several other commenters is that the RC1 changes related to custom elements also have their pros and cons and should also be justified to the same stringent standards you are requesting for alternative solutions. Several of us have raised concerns related to consistency, backwards compatibility, explicitness, and developer control. Specifically there are concerns about attributes such as className and all of the React events such as onClick, onDrag etc, which will behave in unexpected and non-declarative ways with the proposed RC1 changes.

I think the point being made here is that the proposed changes at jimfb@b1db817 require some further consideration.

Given that "the primary component strategy for React will never be Web Components."...

If it were up to me (which I appreciate it's not), I would apply the following constraints to the proposed changes:

• Must not break existing React behaviour with regards to events and React attributes such as className and React events.
• Users must be able to explicitly specify when they want a DOM elements attributes to be controlled outside of React.
• Should be explicit and declarative

Now I appreciate that there has been a vocal request for the ability to support web components, and specifically to allow attributes to be passed through to them. However I would appeal to your better judgement to consider, is this change really the right solution?

Lets not forget, we are not suggesting to add a feature here, but in fact to hold off from adding in a breaking change without due diligence.

[sebmarkbage]

I think the primary motivating factor for allowing something is simply to make it easier once you hit an edge case where you need to consume a custom element and wrap it somehow.

We have escape hatches that you're not supposed to need but still might need for practical reasons.

On Oct 7, 2015, at 3:42 PM, Ben Smith notifications@github.com wrote:

@jimfb the solution suggested by @gargantuan is one option, with it's own merits and considerations.

The point being made here by me and several other commenters is that the RC1 changes related to custom elements also have their pros and cons and should also be justified to the same stringent standards you are requesting for alternative solutions. Several of us have raised concerns related to consistency, backwards compatibility, explicitness, and developer control. Specifically there are concerns about attributes such as className and all of the React events such as onClick, onDrag etc, which will behave in unexpected and non-declarative ways with the proposed RC1 changes.

I think the point being made here is that the proposed changes at jimfb/react@b1db817 require some further consideration.

Given that "the primary component strategy for React will never be Web Components."...

If it were up to me, I would apply the following constraints:

Must not break existing React behaviour with regards to events and React attributes such as className and React events.
Users must be able to explicitly specify when they want a DOM elements attributes to be controlled outside of React.
Should be explicit and declarative
Now I appreciate that there has been a vocal request for the ability to support web components, and specifically to allow attributes to be passed through to them. However I would appeal to your better judgement to consider, is this change really the right solution?

Lets not forget, we are not suggesting to add a feature here, but in fact to hold off form hastily adding in a change without due diligence.

—
Reply to this email directly or view it on GitHub.

[hellosmithy]

I think the primary motivating factor for allowing something is simply to make it easier once you hit an edge case where you need to consume a custom element and wrap it somehow.

@sebmarkbage I appreciate there are motivating factors, but are the counter points being fully considered here? Consistency, backwards compatibility etc. Declarative code in particular is one of the things that makes React so great. I realise in this instance the issues raised here were not necessarily apparent when the change was made but there are some pretty strong arguments here for seeking a better solution now that they have been raised.

The discussion here is not be about wether any of the proposed alternative solutions is the right one, but rather wether any of the solutions including the one in RC1 is the right one.

[hellosmithy]

@jimfb I wonder if we're talking at cross-purposes here. If we're circling back I think it's because the points are being disputed which seems reasonable enough. And to echo @mikeybox it's because we care about keeping React the great project that it is.

Rendering exactly what the user typed is intuitive because a user can type className, see that it literally outputted className, and then try class

I can't speak for others but I can say that this certainly wasn't intuitive for me as I debugged this after upgrading to 0.14.0-rc1 and it seemed like a bug.

the whitelisting would be extensive. You would need to whitelist className, htmlFor, value, all the other dom attributes that have boolean values and/or otherwise behave in intelligent ways

What we're suggesting is to not make an arbitrary disctinction. So why would there be any extra whitelisting other than the single point of entry for setting escaped attrs? The suggestion is simply that <custom-elem> by default behaves the same as <div> or <span>. In fact the behaviour worked exactly this way before, just without the extra ability to set some kind of escape attr for those that do want to connect to external web-component APIs.

[jimfb]

Ok, I think it's time to call it a day. We've had a pretty good discussion, but this conversation is looping back on its self and is past the point of being productive. The thread is just rehashing points that have already been made. I think we fully understand the concerns mentioned in this issue, and we will update the thread when we have new information to provide.

[braden-qualtrics]

Any thoughts on whether or not this (http://stackoverflow.com/questions/37638268/can-i-put-underscores-in-my-html-tagnames) is a reasonable work around for now?

[jimfb]

@braden-qualtrics Sounds like a bad idea to me. You're going to hit all kinds of edge cases that are going to bite you. Cases that our current design/support of custom elements is specifically designed to help you handle, as described earlier in this thread.

[braden-qualtrics]

@jimfb So using underscores in element names might break React/HTML?

[gaearon]

I'm closing since this is not a bug, but a different (and now documented) behavior for web components.
Use class for them.

[sebmarkbage]

This might be related to #7901. If we make a breaking change to switch to properties then this would become className.