WebReflection / hyperHTML-Element

An extensible class to define hyperHTML based Custom Elements.
ISC License
202 stars 25 forks source link

Unexpected reflection with boolean attributes #33

Closed Granze closed 6 years ago

Granze commented 6 years ago

Hi, I'm trying to initialize a property based on an attribute like: this.active = this.hasAttribute('active') || null;

It works fine in a vanilla custom element but isn't working as I expect in a hyperHTML-Element because the property is reflected as attribute, this means this.active will be always true.

I'm missing something or boolean attributes shouldn't be reflected in this case?

WebReflection commented 6 years ago

In hyperHTML boolean attributes already works like that.

render`<my-el active=${true || false}/>`;

However, if you want to define your own attribute handler differently from how HyperHTMLElement defaults observed attributes, you can do it in whatever way you prefer, as shown in this fork of your CodePen.

class MyEl extends HyperHTMLElement {
  static get observedAttributes() { return ['active']; }
  get active() {
    return this.hasAttribute('active');
  }
  set active(value) {
    value ?
      this.setAttribute('active', '') :
      this.removeAttribute('active');
  }
  created() {
    this.html`<span
      class="${this.active ? 'active' : ''}"
    >hello</span>`;
  }
}

boolean attributes shouldn't be reflected in this case?

boolean attributes don't really exists in the DOM, and hyperHTML is all about standards.

I wouldn't know if an element attribute is supposed to be boolean, or accept 1 and 0 as boolean, or it should set true or false as value, like aria attributes.

Since the class owner/author is the only one that knows how special could be an observed attribute, you are in full control of it, being able to define your own behavior.

Observable attributes by default are reflected that way, but I might consider in the future an helper to define static get booleanAttributes() { return ['active']; } since it's a no brainer and it might be handy in general.

WebReflection commented 6 years ago

static get observedBooleans() { return ['active']; } might also be a thing ... now I'm not sure which name is better, but both cases would be like observed, but boolean ...

Granze commented 6 years ago

boolean attributes don't really exists in the DOM, and hyperHTML is all about standards.

This is the confusing part for me. :)

Aren't async, disabled etc. boolean attributes? For my understanding, all the attributes are actually boolean. They return true if present, false otherwise.

The fact hyperHTML is all about standards is the reason I'm considering it for a large project at work, at the same time, I expect it will work like the standard in a situation like this.

I know getters and setters can be a solution, but the other reason I'd like to use hyperHTML for my project is to avoid the verbosity of getters and setters.

WebReflection commented 6 years ago

Aren't async, disabled etc. boolean attributes?

Yes, but the notion of boolean cannot be represented by the DOM API. https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attribute

<label>
  <input type='checkbox' checked name=cheese disabled="true"> Cheese
</label>

Those two attributes are fine with a value, and their presence is all it matters, but other attributes can also have a value.

The most recent proposal to deal with boolean attributes as utility is toggleAttribute, which uses setAttribute or removeAttribute.

When you node.setAttribute('async', true) the node will have async="true" attribute. That's by specs.

For my understanding, all the attributes are actually boolean. They return true if present, false otherwise.

It's like saying all object properties in JavaScript are boolean because of object.hasOwnProperty(name) returns true.

You understand that's nonsense, right?

Accordingly, I need a way to understand your intent. active means nothing to me, how would I know you meant to use it as boolean instead of regular attribute?

Hence my suggestion for observedBooleans, or simply booleanAttributes.

I expect it will work like the standard in a situation like this.

It does. It behaves exactly like native. The issue here is that you put an observable attribute in observedAttributes without having an attributeChangedCallback, which makes no sense.

If you drop that, HyperHTMLElement example would works exactly the same.

If you put an attribute as observedAttributes it means you want listen to changes, not only to the constructor state, and in that case your vanilla demo does nothing if you change that attribute externally, with or without setAttribute or as setter, so your vanilla demo it's not on feature pair with what you can do with HyperHTMLElement.

Then there are aria attributes, where using aria-disabled="true" and aria-disabled="false" produce semantic results and in hyperHTML you'd just aria-disabled=${condition}, which is boolean too, but you wouldn't listen directly to aria changes so that wouldn't be on observedAttributes.

TL;DR

Boolean behavior is very particular on the DOM and deciding a custom element attribute is boolean simply by checking the typeof its value might break existing code that trust attributes can be set with any value, including boolean.

If your issue is CSS, you can custom-el[active=true] and custom-el[active=false].

If your issue is bootstrap only, and you don't need to observe any change, don't put that attribute into observedAttributes.

If your issue is that you want boolean attributes configurable, then tell me which name sounds more reasonable between observedBoolean and booleanAttributes so I can add an utility to disambiguate the intent.

If you have better alternatives now that I've explained all the reasoning behind the currently expected behavior, I'm listening.

WebReflection commented 6 years ago

P.S. in case you are interested, I was involved in the toggleAttribute spec:

there are cases where boolean presence can have also meaningful values, which is a whole new world of possibilities. This is the case of the crossorigin attribute, which has a boolean meaning (is it there or not?) and also a possible meaningful value such crossorigin="use-credentials".

https://github.com/whatwg/dom/issues/461#issuecomment-303239277

So, boolean attributes are still ambiguous, in some case, on the the DOM too, because these can be there or not, and also have a value.

This case invalidates your assumption true and false defines an attribute as boolean, even if hyperHTML does that by default (but for content, not for custom elements, for those you need to decide what to do with boolean attributes).

WebReflection commented 6 years ago

P.S.2 just to be extremely clear, in hyperHTML this works as expected:

render`<button disabled=${true || false}/>`;

as explained in the documentation.

Your use case is on the top level of the node though, which behavior is defined by its class definition, not by hyperHTML.

WebReflection commented 6 years ago

On a second thought ...

It seems like aria-disabled="false" is the exact equivalent of not having aria-disabled at all.

Since there are no other cases where setting false, null, or undefined as attribute wouldn't result into a remove attribute operation, I might make HyperHTMLElement smart enough to understand the boolean attribute.

get attrName() {
  const value = this.getAttribute('attrName');
  return value == null ? false : (value || true);
}
set attrName(value) {
  if (value === false || value == null)
    this.removeAttribute('attrName');
  else
    this.setAttribute('attrName', value);
}

I need to figure out possible side effects though


Above behavior would result into the following simplified boilerplate:

class MyEl extends HyperHTMLElement {
  static get observedAttributes() { return ['active']; }
  render() {
    this.html`<span class="${this.active ? 'active' : ''}">hello</span>`;
  }
}

granting reaction on every change (another important thing, always use render() method to render nodes, not created)

WebReflection commented 6 years ago

Ambiguity

How would I know what to return in here? Example, button.disabled return false or true, but never null.

If I don't differenziate with other mechanism, would returning null by default OK, so that anyone could overwrite the getter, eventually, and return this.hasAttribute('attrName') ?

get attrName() {
  return this.getAttribute('attrName');
}

The setter seems simple/reasonable enough

WebReflection commented 6 years ago

OK, latest behaves reasonably smart. The only better way is to introduce non standard behavior but I think this is already an improvement over what standards offer.

WebReflection commented 6 years ago

FYI I've updated the Code Pen with a reactive accessor so that you can update the component state on the fly

Granze commented 6 years ago

It works like a charm! Thanks for the change and for the interesting discussion. 👍

WebReflection commented 6 years ago

@Granze this introduced a regression with common attribute case.

The next release up for review rollback to previous behavior but you can still specify, when boolean is meant, a getter for those properties.

Apologies for the inconvenience but I think I've rushed conclusion too early about the boolean attribute.

WebReflection commented 6 years ago

Hi @Granze , FYI v2.2.0 fixed introduced explicit get booleanAttributes() { return []; } at class level to have never ambiguous boolean semantics.

Every other behavior can be defined as you like too so 2.2 should cover all use cases.

WebReflection commented 6 years ago

P.S. the code pen has been updated to show how it works now

Granze commented 6 years ago

Hey @WebReflection, thanks for the update! 👍