Set non primitives as properties instead of attribute

[porsager]

To comply with custom-elements and most likely also user expectations any non primitive should be set as a property on the dom element instead of as attributes

Description

I've added a check to set value as a property on the dom element if the type is not a primitive

Motivation and Context

Fixes custom-elements compliance (#2220) which should have been added in #2221 , but I couldn't find where.

How Has This Been Tested?

All existing tests passes, and new tests for non primitives to be set as properties have been added

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation change

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have updated docs/change-log.md

[barneycarroll]

As discussed on Gitter, it's rarely Mithril's place to make these assumptions about whether input should map to property or attribute (or both). Eg id is a direct bi-di binding, depended on by DOM and CSS selectors.

The classic case of near-enough bi-di binding is writable boolean attributes where

Property	Attribute
dom[key] = true	dom.setAttribute(key, '')
dom[key] = false	dom.removeAttribute(key)

When this is desirable it is handled by the element's DOM as determined natively or in a custom element definition.

There's a few special cases where Mithril does need to step in to determine how to handle special property / attribute handling - className & class, tabIndex & tabindex, input.type change mandates a new element. But there are innumerable other possibilities.

Current behaviour - assuming property setting by default - is correct. Native elements do (and custom elements can) use property setter traps to determine attribute mapping where desirable. In other scenarios - innerHTML, innerText, scrollTop, slot - there is no attribute mapping. Custom elements can bring in any number of such properties (and can and should take responsibility to define the bi-di binds themselves where desired).

[porsager]

Sorry, but I think you might be thinking of the muted discussion still? This one is about custom elements compliance. Would you mind confirming you're responding to my changes/descriptions in this specific pr?

[barneycarroll]

Haha sorry

[barneycarroll]

Sorry, I'm confused - I didn't realise we had a fall through to setAttribute. As per my comment, I'm still against setAttribute as a default even if we're dealing with primitive values. I'm currently trying to wrap my head around why scrollTop (number) won't work but innerText (string) will. 🤔

[dead-claudia]

@porsager @barneycarroll The current behavior of attempting to set properties then falling through to setting attributes if missing been the behavior since the v0.2.x days.

I'm heavily against this particular PR, though, specifically because it makes significant decisions based on the type of attributes' values. The only precedent we have for this is diffing input.value and friends to avoid causing Chrome to reset its cursor location if the value didn't really change. Going beyond that could potentially confuse users because it's not immediately clear at the vnode site that it's doing anything other than what they'd expect. Plus, it's worth mentioning not all custom elements restrict their setAttribute value input to just strings, nor do they have to - in fact, that very example permits objects to be used for attribute values. Finally, the added type checking in a hot loop creates a performance risk.

How about this alternative: let's add two magic properties for DOM vnodes, attrs: {...} and props: {...}, to force properties to be read as attributes or properties. It'd be almost trivial to add, and it'd avoid the semantic mess of doing things based on the type of the value.

[porsager]

Sorry, I'm very confused now too. I think this whole PR got muddled in muted and assignment vs setAttribute, which it is not at all about. Could we try to keep to only the part about what happens when objects, arrays (and functions) are applied to the attrs object? If we want to follow custom elements we need the behavior in this PR, and I have a hard time understanding why we wouldn't want this behavior even without the custom elements compliance argument. Why would anyone want to have "[object Object]” as an attribute? Now the only one i thought someone could have relied on was the comma separated stringification of arrays, but it was actually you @isiahmeadows who convinced me that that would hardly be a problem and that we hadn't even documented or made tests for that behavior.

I was actually sure we already decided to have this implemented in the issue i linked above, and that it was also implemented in the PR #2221 by you isiah, but that you either missed the crucial part or that there had been a regression since.

[fuzetsu]

I am also not understanding the arguments against this PR. Like @porsager says I can't think of any scenario where [object Object] would be desirable.

The MDN docs for Element.setAttribute() have this to say about the 2nd parameter:

A DOMString containing the value to assign to the attribute. Any non-string value specified is converted automatically into a string.

Emphasis is mine.

@isiahmeadows I think in that link you posted the setAttribute method is being overridden.
https://github.com/aframevr/aframe/blob/v0.8.0/src/core/a-entity.js#L638

Are you suggesting that for custom elements to be compatible with Mithril they will have to override setAttribute?

[fuzetsu]

@isiahmeadows
My bad, I see the alternative solution you proposed, and your argument about performance with the multiple type checks.

The nested attrs and props seem reasonable, but it does feel a bit awkward / non-standard. Would caching the typeof in a variable help the performance? (or are repeated typeof calls on the same ref already optimized)

Also I assume that these magic properties would be optional and existing attributes at the top level would continue to work as they do now?

[barneycarroll]

TL;DR: I wasn't reading current source correctly — but it is correct and the PR is unnecessary.

I think I've muddied the water horribly, I apologise. I was wrong-footed by brain-farting some of my JavaScript interpretation & weird behaviour specifically relating to scrollTop which led me to misinterpret the conditional graph.

@isiahmeadows I think you had it right in 2220 when you said

the fact we send everything to attributes for custom elements and not normal ones would be surprising to some consumers

@fuzetsu is IMO correct in asserting that A-frame's behaviour is exceptionally weird. Per MDN, setAttribute's value parameter is

A DOMString containing the value to assign to the attribute. Any non-string value specified is converted automatically into a string.

Attributes should be serialiseable, eg parsable as static XML and interpretable by CSS selectors.

Back to @porsager's PR, what I was previously missing in the Mithril logical tree was the early condition to test for key in element as the determining factor in whether an attr is determined to be a property (Here's a simple demo showing how pseudo bi-directional binding happens on a dynamic level based on DOM internals).

If you haven't already done so I highly recommend GDN's instructions for how and why to implement this kind of behaviour in custom elements.

The reason your test cases aren't valid @porsager is that you're using plain elements while asserting that they could be custom elements with special interfaces for those non-standard properties: in practice any custom elements that did have special interfaces for those properties would expose them in the key in element check. This logic may seem gnarly at first but on reflection it's far more transparent than A-frame, which clobbers custom elements' standard interface for observable attributes — perhaps because they wrote for v0 which hadn't yet defined static observedAttributes — and offers no standard inference for determining what their attribute / property interface might be.

[barneycarroll]

@porsager flems with an appropriate custom element + your tests.

[dead-claudia]

@barneycarroll True, and maybe we could String(value) all attributes other than style and just document we do it in the case of custom elements as well as native DOM elements. If A-Frame users want the optimization, they should roll their own components. (A-Frame has a unique API that doesn't directly map well to virtual DOM, much like Bootstrap.)

[dead-claudia]

@porsager

Sorry, I'm very confused now too. I think this whole PR got muddled in muted and assignment vs setAttribute, which it is not at all about.

Yeah, I misread the whole thing - I thought this was related to that when it wasn't. Apologies about that @porsager.

---

@barneycarroll @porsager

(This quotes @porsager's comment, but it's really in reply to some of the subsequent discussion.)

I was actually sure we already decided to have this implemented in the issue i linked above, and that it was also implemented in the PR #2221 by you isiah, but that you either missed the crucial part or that there had been a regression since.

We decided to keep Mithril's existing property + attribute fallback in place, to do to custom elements what's already done with DOM nodes. #2221 specifically says this in the description (emphasis not in original):

Align custom elements to work just like normal elements, minus our HTML workarounds. Previously, our support has been undocumented and completely untested, but I plan to fix that in a follow-up commit (along with a changelog entry).

This is also documented here, where it states "And yes, this translates to both attributes and properties, and it works just like they would in the DOM."

(Note to self for later: the docs in that section could stand to be revised to be a little less editorial-y around custom element support. For example: "[...] For example, you can use A-Frame within Mithril, no problem!" should read more like "[...] For example, you can use A-Frame with Mithril like this:". There's a couple other places that need some minor tweaks.)

In addition, the existing behavior of passing properties through unmodified is documented in that same section: "For custom elements, it doesn't auto-stringify properties, in case they are objects, numbers, or some other non-string value".

So yes, this is a breaking change which requires a docs update. But that particular section talks about passing through values only in reference to properties, leaving attributes purely implied, so unconditionally stringifying attributes is still something I'm open to. It also wouldn't be that hard, and I might be able to simplify a few HTML-specific checks. The key here is that custom elements are treated like normal HTML elements, just ones that happen to not need all the hacks. That's our story - custom elements are first-class elements, no extra support required.

[dead-claudia]

The implementation is still not quite complete, though: if you register your custom elements after Mithril sees it, it borks our data model. It is possible to fix, so it's not like we couldn't provide a better story. Oddly, this isn't tested for by the Custom Elements Everywhere site, so we might be a step ahead of our peers on it.

[barneycarroll]

@isiahmeadows I don't think forcibly coercing attribute values to string on Mithril's side is valuable. The behaviour happens automatically via the element interface, or in the case of A-frame, is skipped altogether.

The definition promise is interesting. Presumably we'd want to then trigger a redraw that invalidates the previous vtree so a new one can be built from scratch with the right inferences?

[dead-claudia]

@barneycarroll

I don't think forcibly coercing attribute values to string on Mithril's side is valuable. The behaviour happens automatically via the element interface, or in the case of A-frame, is skipped altogether.

FWIW, my original intent is what's implied now. Like I said, I'm open to changing it, but I'm waiting for a compelling reason, and I'm just not seeing anything here persuasive enough to make the change.

The definition promise is interesting. Presumably we'd want to then trigger a redraw that invalidates the previous vtree so a new one can be built from scratch with the right inferences?

Sure, but I'd also like a magic onelementdefined hook instead, specific to custom elements and their immediately containing components. Either way, we'd need to replace vnode.dom on upgrade.

I'll file a new issue for this, since it's off-topic here. Edit: #2339

[pygy]

Wouldn't it make sense to dispatch once per element into either updateAttrs or updateCustomElementAttrs?

More generally, we could add a flag on the vnode (think bit set) during the "create" phase to mark elements, and use it to dispatch to updateInputAttrs and updateSelectAttrs, etc., which would defer to updateInputAttr(), etc... so that we avoid dispatching in a loop.

That would let us cut down the branches in the common cases, and will probably boost perf.

The flag would also let us drop a typeof call to tell components apart from other elements.

[dead-claudia]

@porsager @pygy @barneycarroll and other interested parties: I've filed #2340 as an alternative proposal to this.

[dead-claudia]

@pygy I'm having trouble getting what you're trying to say. Could you elaborate a little, especially with regards to context? I'm having trouble tracing what your comment is in reply to.

More generally, we could add a flag on the vnode (think bit set) during the "create" phase to mark elements, [...]

Just thought I'd point out I've been considering adding this for a while for various things. Mainly, it's because I can avoid type checks for existing data, but pretty much every time I've looked into using it for a feature, I've noticed it's been one of a couple things:

1. It's really just a null/existence check, equality check, or something similar that doesn't require type info to check.
2. I've overcomplicated something and it could be reduced to the first one, something that doesn't require type info to use.

Also, about the only real precedent we had for a boolean state option, vnode.skip, was itself removed because when you simplified and streamlined the diffing algorithm, it was no longer necessary.

[barneycarroll]

I think I'm interpreting @pygy right in the notion that this code is horribly forked at the minute and the set/update attr/attrs code is getting 0.1ey in its moistness. That's something we should fix regardless of whatever comes out of this editorial decision.

[dead-claudia]

As per discussion on Gitter between @barneycarroll and I, we've decided to just document how we do resolution and encourage oncreate/onupdate as appropriate when it fails (which is almost never).

[pygy]

@isiahmeadows: @barneycarroll is correct: I mean that setAttritbute() is full of branches that could be statically determined before we call setAttributes() and updateAttributes().

Not having to worry about form fields or custom elements would make the common case faster.

[pygy]

Also, the latest RC seems to work just fine with custom elements and arbitrary props in the browser...

The mocks need to support custom elements if we want to ensure there are no regressions...

[dead-claudia]

@pygy They already kinda do in that they don't validate their input at all. It's why our m("a", ...) and other similar nonsense in the tests works. We also use custom elements using the DOM mock in some of the tests, so things to seem to work at least somewhat. Really, our mock is so overly permissive I haven't run into any real issues using it with things like custom elements.

We could stand to use some tests of the mock itself in this area, so we know it works as we expect it to. But as it stands, I don't think the DOM mock itself needs patched.