WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.06k stars 112 forks source link

Boolean readonly attribute #389

Closed jaschaio closed 4 years ago

jaschaio commented 4 years ago

Not sure if this is a bug or I am misunderstanding something. It works with disabled but not with readonly.

Taking the example from the documentation on how boolean attributes work I would expect the following:

// Should render <input type="text" />
hyperHTML.bind( form )`<input type="text" disabled=${ false } readonly=${ false } />`;

But it actually renders readonly="false":

<!-- Code referenced above renders <input type="text" readonly="false" /> -->
<input type="text" readonly="false" />

According to Mozilla readonly is a valid boolean attribute for input type="text" elements so I am unsure why I am seeing this "bug".

Here is the codepen: https://codepen.io/jaschaio/pen/poJrENr

In all major browser the text input is actually not writeable as the readonly attribute is present.

jaschaio commented 4 years ago

Btw. checking out #155 passing null removes the attribute. But I thought just passing true or false should be sufficient?

WebReflection commented 4 years ago

The readonly attribute is not really a boolean one, because it either exists, or not.

var input = document.createElement('input');
'readonly' in input; // false

input.readonly = true;
input.outerHTML; // <input>

However, you can consider it a weird semantic exception of the standard, but since there is a mechanism in hyperHTML to deal with such cases, all I can suggest for these kind of attributes is to use the following interpolation:

hyperHTML.bind( form )`
  <input type="text"
    disabled=${disabled}
    readonly=${readonly || null} />`;

That would cover the readonly case 👋

WebReflection commented 4 years ago

P.S. it looks like using readOnly instead, which is another standard shenanigan, you should be good to go as boolean, hope this helps.

If you want to keep it even more explicit, you can write:

hyperHTML.bind( form )`
  <input type="text"
    disabled=${disabled}
    .readOnly=${readonly} />`;

Please note the . setter to opt in as explicit JS setters, instead of HTML attribute.

Background

There's no way I can know, given a generic attribute name, specified as lower case, if there's some variant of that attribute that once made camelCase would work as expected or not, unless I maintain the entirety of tagName => special cases somewhere, I use a dictionary to transform words on the fly, or .... nay, I ain't gonna do that.

jaschaio commented 4 years ago

Thanks, I think it's completely fine that those "cases" are not covered as you've mentioned in your Background Info. It might just make sense to mention it and the workaround of using ${ readonly || null } within the documentation. Either way if somebody else trips over this they will find this issue now 👍