Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some boolean attributes/properties with weird behavior #2809

Open
pygy opened this issue Dec 14, 2022 · 7 comments
Open

Some boolean attributes/properties with weird behavior #2809

pygy opened this issue Dec 14, 2022 · 7 comments
Assignees
Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage

Comments

@pygy
Copy link
Member

pygy commented Dec 14, 2022

Edit: The original title was "Rendering m([spellcheck=false]) results in <div spellcheck="true">", but there are more issues, see below.

As mentioned in MithrilJS/docs#20 by @anonghuser, m([spellcheck=false]) results in <div spellcheck="true">, it should obviously be false.

I don't know if this should be fixed though. In scenarios where you want it not set, statically, you can omit the attribute. For dynamic scenarios the attrs object is a better method.

@pygy pygy added the Type: Bug For bugs and any other unexpected breakage label Dec 14, 2022
@pygy pygy self-assigned this Dec 14, 2022
@anonghuser
Copy link

anonghuser commented Dec 14, 2022

scenarios where you want it not set, statically

It is different from most other attributes in that when not set, it defaults to true, thus there are scenarios where you want it statically set to false.

@pygy
Copy link
Member Author

pygy commented Dec 14, 2022

Ooooh... So we must fix it.

This is a m.render bug, not a hyperscript/selector parser bug btw. I'll add a special case in the attribute handling code.

@anonghuser
Copy link

I imagine the special case would not be based on the property name but when the property type is bool and the assigned value is the string "false"?

@pygy
Copy link
Member Author

pygy commented Dec 15, 2022

The problem here comes from the facts that

  • spellcheck is both an attribute and a property. In that general case, Mithril defaults to using properties (with exceptions to work around browser quirks).

  • HTML can encode boolean attributes in two ways, either checking for the presence of an attribute (think contenteditable), or with literal =true/false values.

Our logic AFAIK handles most of actual attributes/properties fine, because attributes with literal =true/false values generally don't have matching properties (think aria-x) or default to false when absent, obviating the need for a static [x=false].

We can't just add spellcheck to the list of exceptions to the properties code path, because it would then be buggy when passed a boolean false value through vdom attributes.

With that said, implementing your suggestion may break existing user code, so I'd rather be conservative and add a special case for spellcheck in the properties code path.

We may revisit this for the next major version.

Edit: actually, since spellcheck is not an HTML boolean attribute, but an attribute that uses stringly-typed bool-ish values, we could just use the setAttribute code path, and consider {spellcheck:false} to be buggy and {spellcheck: "false"} to be correct. This would probably trip people up though and break existing code...

@pygy
Copy link
Member Author

pygy commented Dec 15, 2022

Investigating the problem space, this is a temporary braindump.

I've extracted the attribute list from MDN using

const attrMap = entries.map(tr => [
  tr.children[0].textContent,
  tr.children[1].textContent === "Global attribute" 
  ? ['div'] 
  : tr.children[1].textContent.trim().split(/\s*,\s*/g).map(s => s.replace(/^<|^\\x3C|>$/g, ''))
])

This doesn't list the aria-* attributes.

From there, we can list the attributes that double up as properties, and, if these are boolean, their default values.

Of these, three may need special attention: download, spellcheck and translate.

  • async for scripts, which defaults to true (not sure if we want to cater to this) (nope, setting async to false has no effect, we can ignore this).
  • download can take a filename or no value (letting the browser generate a file name). The corresponding property is always a string, which is ambiguous because both <a> and <a download> return the same '' value when queried through the getter.
  • spellcheck the topic of this discussion
  • translate can either take yes, no or no value (meaning yes). It defaults to yes when the attribute is absent, and the matching property is boolean (thus true by default).

TODO: investigate aria, SVG

edit: The script above didn't properly clean up the attr names, and I had missed <script async> because of it.

edit2: recapitulating:

Currently rendering m('a[download][spellcheck=false][translate=no]') results in <a download="true" spellcheck="true" translate="yes"></a> which is not what's expected. download should be '', and the others are obvious.

@panoply
Copy link

panoply commented Dec 15, 2022

Just want to reference MithrilJS/mithril.d.ts#46 (comment) which while a tad unrelated in the scope of the issue it does pertain to attribute structures and more specifically object prop > value expressed. While I know a lot of the hardcore SWE are not adopting TS when it comes to things like this it’s golden shackles.

@pygy pygy changed the title Rendering m([spellcheck=false]) results in <div spellcheck="true"> Some boolean attributes/properties with weird behavior Dec 16, 2022
pygy added a commit to pygy/mithril.js that referenced this issue Dec 25, 2022
@pygy pygy mentioned this issue Jan 16, 2023
11 tasks
@dead-claudia dead-claudia added the Area: Core For anything dealing with Mithril core itself label Jan 18, 2023
@dead-claudia
Copy link
Member

I'm leaning towards a "no" for this. The DOM APIs all accept and return strings, and the HTML attributes have to be stuff like spellcheck="true" rather than the standard boolean spellcheck="".

Not closing yet, because I'd like to solicit a little more discussion for this before shutting the door on it.

@dead-claudia dead-claudia mentioned this issue Oct 13, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage
Projects
None yet
Development

No branches or pull requests

4 participants