Support Custom Elements setAttribute call in render

[JAForbes]

Proposal

I propose we have a special case in render.js's setAttr that detects custom elements and always passes whatever value was received to element.setAttribute

Description:

Custom Elements often use calls to setAttribute to trigger internal mechanisms.

(An example: https://aframe.io/docs/0.3.0/core/entity.html#setattribute-attr-value-componentattrvalue)

Mithril only calls setAttribute based on some conditions that may or may not work for some custom elements (whitelisting attributes, checking for namespaces etc).

So custom-elements are unable to write certain attributes using the mithril API.

Steps to Reproduce:

1. http://jsbin.com/ruzaguvise/edit?js,output
2. Comment out the trust call
3. Uncomment the mithril api version
4. Nothing renders, open the inspector, and you'll see attributes (like position) are present but have no value.

Expected:

The box should have rendered in the same way the m.trust call did.

[dead-claudia]

I'm pretty sure this is the case for both 0.2.x and 1.x. And it seems like a relatively straightforward bug to fix, even without much knowledge of the framework. (read: good starter bug)

[dead-claudia]

Oh, and the reason why it prefers to set object properties where possible is because those are generally much faster than setAttribute (which is really slow).

[pygy]

Note that there are two scenarios that must be covered: searching for a dash in vnode.tag and for a non-null is attribute (which can be used when extending native elements).

[JAForbes]

So it seems there's support for this. Is there any objections to me creating a PR?

[barneycarroll]

@JAForbes you may have misread the setAttr logic — what the code actually does is fall through to setAttribute by default, only applying the property directly if key in node && !isAttr(key) (where isAttr contains the little whitelist.

This behaviour was introduced in #924 precisely because of custom element problems (ignore my comment, I misunderstood the proposal).

[JAForbes]

@barneycarroll I haven't stepped through in the debugger yet, but I think the logic isn't explicit enough.

key in element && !isAttribute(key) && ns === undefined

If there is a custom element, and the key is in the element, this branch will trigger. Which will lead to:

element[key] = value

My hunch is, this is exactly why aframe's custom element failed on the position attribute because all elements have position in that library.

So I think we need a branch, before the aforementioned one:

else if( vnode.tag.indexOf('-') > -1 || vnode.attrs.is ){
   element.setAttribute(key, value)
}

For the actual implementation, we might want to simply exclude it from that middle case so it gets caught by the bottom fallthrough?

So we could change the middle case to:

(key in element && !isAttribute(key) && ns === undefined && notCustomElement(vnode))

Either approach should fix the problem I imagine.

[barneycarroll]

Oh. So the element has the property applied directly to the DOM node itself but also has an attribute of the same name? In this case, would it be safe to say custom elements always expect change to be applied via setAttribute?

[lhorie]

Is there any objections to me creating a PR

Not at all

[lhorie]

Merged #1485