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

Boolean attributes #1101

Closed
edemaine opened this issue Jul 3, 2022 · 25 comments
Closed

Boolean attributes #1101

edemaine opened this issue Jul 3, 2022 · 25 comments
Milestone

Comments

@edemaine
Copy link
Contributor

edemaine commented Jul 3, 2022

Questions: How should JSX attributes foo and foo={true} behave for an HTML element, as in <div foo> or <div foo={true}>? Should the behavior differ for components and/or spreads?

Current behavior in Solid:
Assume const Div = (props) => <div {...props}/>

JSX Equivalent HTML
<div foo> <div foo="">
<div foo=""> <div foo="">
<div foo={true}> <div foo="true">
<div foo="true"> <div foo="true">
<Div foo> <div foo="true">
<Div foo=""> <div foo="">
<Div foo={true}> <div foo="true">
<Div foo="true"> <div foo="true">

The ☆ behavior (another consequence of setAttribute's string casting) is particularly counterintuitive, as it differs between HTML elements and component-wrapped elements. (It may also be an SSR discrepancy?) @fabiospampinato's observation of this weirdness is what spawned this exploration (in #templating channel, also with @lxsmnsyc).

Relevant facts:

  • HTML 5 defines attribute foo to be equivalent to foo=""
  • React defines that foo is equivalent to foo={true}.
  • TypeScript treats foo like foo={true} (attribute should accept Boolean value)
  • React (specifically react-dom) converts JSX attribute foo to HTML equivalent of foo="true" unless foo is on a whitelist of "Boolean" attributes, in which case it converts to foo="". Here's a relevant list; note that there are some Boolean-like attributes which aren't actually Boolean. For example, React "correctly" converts draggable={true} to draggable="true" and "correctly" converts async={true} to async="".

Desired properties:

  1. foo is always equivalent to foo="" for HTML/SVG elements (as in HTML 5). This is useful for copy/pastability of HTML, which the JSX spec considers important.
  2. foo is equivalent to foo={true} (as in React and TypeScript)
  3. draggable={true} is equivalent to draggable="true"
  4. <div foo> is equivalent to const Div = (props) => <div {...props}/>; <Div foo>

We can't have all four properties.

  • Properties 1, 2, and 3 contradict each other: 1+2 imply draggable={true} is equivalent to draggable="".
  • React satisfies Properties 2, 3, and 4. It follows Property 1 only for a whitelist of Boolean attributes.
  • Solid satisfies Properties 1 and 2. It satisfies Property 3 for Div but not div. It doesn't satisfy Property 4 because of ☆.

Proposal:
I really like Property 1, and would propose gaining Property 4 by compiling foo={value} to essentially _el$.setAttribute('foo', value === true ? '' : value), where value is stored in a temporary variable if it's a general expression, with a similar modification to spreads. This would basically flip ☆, and also change those marked "!":

JSX Equivalent HTML
<div foo> <div foo="">
<div foo=""> <div foo="">
<div foo={true}> <div foo=""> !
<div foo="true"> <div foo="true">
<Div foo> <div foo="">
<Div foo=""> <div foo="">
<Div foo={true}> <div foo=""> !
<Div foo="true"> <div foo="true">

(By contrast, React's approach of an attribute whitelist feels gross...)

@fabiospampinato
Copy link

I think potentially there's a 5th desired property:

  • Writing <div foo /> should produce a div with an identical outerHTML string for any value of foo both if Solid's custom transform is used and if it isn't.

Maybe this isn't super desirable or it's too much of an edge case, but since using Solid with React's transform is supported I'd find it strange if something completely static like <div draggable /> produced different divs depending on which transform is used, though in general some differences are expected or the custom transform wouldn't exist.

@otonashixav
Copy link
Contributor

To be clear, this wouldn't affect directives, correct? i.e. <div use:foo> still calls foo(ref, () => true), and not foo(ref, () => "").

@LiQuidProQuo
Copy link
Contributor

what about the case where you want to remove the attribute
undefined probably
false null maybe

@edemaine
Copy link
Contributor Author

edemaine commented Jul 3, 2022

To be clear, this wouldn't affect directives, correct? i.e. <div use:foo> still calls foo(ref, () => true), and not foo(ref, () => "").

Correct. It won't even affect props in components: foo will still appear as props.foo === true. It's just about when something hits the DOM, which never happens for directives.

what about the case where you want to remove the attribute
undefined probably
false null maybe

undefined and null already behave this way (whereas false renders as "false"). Oops, I didn't realize there's already a setAttribute helper which could be used for exactly this purpose:

https://github.com/ryansolid/dom-expressions/blob/3c593d43147a73ddd52c58ba8c49f4f1e0e7e560/packages/dom-expressions/src/client.js#L68-L71

I took a stab at implementing the proposal here: ryansolid/dom-expressions#141

edemaine added a commit to edemaine/dom-expressions that referenced this issue Jul 3, 2022
Fixes solidjs/solid#1101
by making `<div foo={true}>` behave like `<div foo>` and `<div foo="">`
@LiQuidProQuo
Copy link
Contributor

LiQuidProQuo commented Jul 3, 2022

undefined and null already behave this way

yes

(whereas false renders as "false").

but then isn't odd if true does this:
<div foo={true}> | <div foo="">
and false does this
<div foo={false}> | <div foo="false">

?

@ryansolid
Copy link
Member

ryansolid commented Jul 3, 2022

I see. In general we set known boolean attributes by property and everything else by setAttribute. I see I left boolean(no arg) working like a boolean in it literally sets empty string but this is probably wrong. A boolean attribute cannot be removed so there is no inconsistency on that side but it means we probably should be setting it to true.

We used to do this the other way but there are attributes that actually want "true" and "false". I think we should continue checking against the list and apply it to the boolean form. I guess thats siding with React. But the other way is no good either.

There is no guarantee it ends up in the DOM through a component so it has to come in as true. The spread has the choice. And we've landed on pass list. So i think it has to be the first example that is off.

EDIT: If you are wondering the source of Solid's implementation. As usual Inferno is what I trust for these sort of things. React probably has the most thorough but I suspect this is a place both Inferno and Preact save size by doing something reasonable.

@lxsmnsyc
Copy link
Member

lxsmnsyc commented Jul 4, 2022

Some helpful resource (from React).

Nothing is mentioned about aria-* properties, but should we handle them like draggable?

Edit:
Seems like Solid already properly handles aria-* properties based on my tests on solid-headless

@edemaine
Copy link
Contributor Author

edemaine commented Jul 4, 2022

This is a great analysis of React behavior @lxsmnsyc! I think this line actually makes aria-* and data-* behave like aria-*="true" and data-*="true"; at least that matches my testing.

Obviously one option is to reproduce React's behavior. I think there's still an open question of what to do with attributes that aren't on any of these lists:

  1. We could default to foo/foo={true} meaning foo="true", as Solid currently does.
  2. We could default to foo/foo={true} meaning foo="" as in Desired Property 1 / HTML 5 spec.
  3. We could default to foo/foo={true} meaning "make no attribute, but issue warning", which I just discovered React does.

Personally I think Option 2 makes sense, given HTML spec, but it differs from both current Solid and React. An advantage of this approach is that you only need a list of "enumerated but Boolean-like" attributes, like those listed in bullets 1 and 2 of @lxsmnsyc's post and aria-* (not sure about React's decision to include data-* — that feels like it should be application-specific, so matching HTML behavior makes more sense to me). No need to have a list of all Boolean properties because Option 2 will just handle them correctly. (There's a similar advantage to Option 1: you'd "only" need to list Boolean properties, not pseudo-Boolean properties. But I think that list is longer.)

@lxsmnsyc
Copy link
Member

lxsmnsyc commented Jul 4, 2022

With data-*, how does dataset property work with raw boolean values?

@edemaine
Copy link
Contributor Author

edemaine commented Jul 4, 2022

With data-*, how does dataset property work with raw boolean values?

Good point: dataset casts true to "true". Currently, Solid seems to use setAttribute for setting data-* attributes, but if it switched to using dataset in the future, it might make sense to map data-foo to data-foo="true" for consistency with the reactive case.

@ryansolid ryansolid added this to the 1.5.0 milestone Jul 20, 2022
@ryansolid
Copy link
Member

If I didn't make it clear in my previous response. While I see potential of changing more things and doing stuff differently. It's the first row that is the bug today and should be fixed. We already default to true except for a pass-list and I'm not prepared to change that at the moment. We were the other way in the past but it caused other confusion and errors.

So I've updated this case to use that pass-list: <div foo> => <div foo="true">. Where a known boolean attribute would work appropriately.

@edemaine
Copy link
Contributor Author

For what it's worth, I recently bumped into this React GitHub issue where users were confused about the inconsistent behavior, in particular for data-* attributes and how it differs from HTML. (They write "JavaScript" but they mean the innerHTML interface.)

facebook/react#24812

At the least I think we need to document things clearly here. (Personally I'd also much rather have the default be "like HTML" instead of "like React".)

@lxsmnsyc
Copy link
Member

I think we should follow the convention of handling booleans for specific attributes, just for consistency. It's just a small price to pay.

@LiQuidProQuo
Copy link
Contributor

but this generalization works for any known and unknown boolean attribute ( will passing "true" to a known html attribute be considered non standard? will it not work as expected?)

it is also more or less consistent between Component and an Element.
even if slightly deviates from native html interpretation foo=""

Element

<div foo> === <div foo={true}> => <div foo="true">

Component

<MyComp foo/>
const MyComp = function(p){  
console.log(p.foo===true) // true 
...
 }

null value

null, is the a special case, as it removes the attribute, and doesn't simply assign the value natively ( for Element, for component value is passed)

<div foo={null}> => <div>

and if one doesn't like the "true" they can be explicit and set <div foo="" />

@ryansolid
Copy link
Member

This thread is relevant here: #383. Where I justified the current behavior at the end by being browser. But I didn't realize the inconsistency with spread. It looks like I pulled inspiration from Svelte for the current behavior but did not test spread there I assume.

I do think coercing {true} to "" and {false} to remove can cause issues which means changing this feels major version to me. It is interesting though. See we used to set all things as properties so boolean would just work that way. Then we changed to default to attributes which meant that I'd need to look for falsey to remove the attribute. I kept booleans on elements being "" simply because it was simple enough to do and I justified it by being like the browser. But I suppose if we really wanted to be like the browser we would force people to use "true" for draggable. Or their webcomponents that expect it not to be handled like a boolean attribute. This is a defensible position even if I find it unfortunate. But masking browser behavior is awkward. Generally when in doubt we do side with the browser, so this is strangely inconsistent with that.

That being said from my previous research not a single other JavaScript framework aligns with the browser here. true means "true" and false means "false" with exception of the pass-list. Even close to the browser frameworks like Preact or Svelte. Preact works identical to how I proposed to fix this. And Svelte works exactly like how Solid works today with the inconsistency on the spread.

@edemaine
Copy link
Contributor Author

That thread is a neat read. The idea of bool:foo={true} meaning foo="" is an interesting approach.

It's a shame that browser and frameworks are so divergent here. I still like siding with the browser default, for the sake of "you can copy/paste HTML and it just works the same".

But I suppose in actual practice, it doesn't really make much difference as long as you stick to standard attributes, so either way you'll get whitelisted to Boolean or pseudo-Boolean behavior as needed. The main practical difference you'd see is probably data attributes, where <div data-foo> differs between browser and React or Solid (under Ryan's proposal, unless we decide to whitelist data-* as Boolean too).

@ryansolid
Copy link
Member

I think the one thing that we have going for us is that foo={true} isn't a copy-paste. Like the decision of how we handle booleans is independent of this until we consider spreads. But components are also not copy-paste. So if we left everything exactly as it is, like Svelte, other than the expectation on spreading boolean attributes coming from a component which is arguably a bit whatever we are retaining copy-pasteability.

It's possible the right answer is no change.

@LiQuidProQuo
Copy link
Contributor

no change means

      <button draggable={true}>can drag</button>
      <button draggable>can't drag</button>

@ryansolid
Copy link
Member

@LiQuidProQuo

Yes which came up with the linked issue. I have very little problem with this.

The argument is the browser doesn't work this way either. We are free to define the meaning of draggable={true} but draggable as copy pasted from StackOverflow will not work all the same without being given a true value. The current behavior doesn't violate the browser at all but leaves us free to have the convenience of using booleans instead of always using strings for these sort of things.

I guess I'm curious would you rather this be the case or that neither worked and you needed to do:

<button draggable={condition() ? "true" : "false"}>can drag</button>

I'm trying to gauge the cost of compromise here. Like is the concern that these look inconsistent? Or that it requires the knowledge of how draggable and boolean attributes work.

@LiQuidProQuo
Copy link
Contributor

LiQuidProQuo commented Jul 28, 2022

@ryansolid

honestly I am just realizing that draggable is an edge case, it is not really a boolean attribute
but a boolean value based.

so if we look at a real boolean attribute(presence base), we need to get to the equivalent of:
<input type="checkbox" checked={condition() ? "checked" : null}>is checked?</button>
or
<input type="checkbox" checked={condition() ? "" : null}>is checked?</button>
which is already cover by current behavior

"" | "checked" is to comply with the recommendation of the standard, but "true" also works

I am not personally in favor of making draggable get treated as an edge case(not sure if there are other attributes like it),

I did like your proposed change, as it seem to strike a good balance.
<div foo> => <div foo="true">
it "works/normalize" with fake boolean attribute draggable or any real one.

but realizing that draggable is an exception, I am thinking no change might actually be better
users of draggable will just have to be explicit, like they need to in actual html

<button draggable={true} />

but then for Components do we expect prop.draggable true or undefined ?
<Button draggable />
I prefer that true is passed in props, because it simplifies the check, compared to checking if key exists in the props.

so perhaps I am also starting to lean towards no change

and any special edge case magic, should have special dedicated explicit syntax in my opinion( if its worth having).

@edemaine
Copy link
Contributor Author

I'd indeed prefer "no change" over changing the meaning of <div foo>; this indeed preserves copy/pastability.

But could we change spread to match, i.e., change the behavior of ☆ in the original post? Here's a perhaps better illustration of the inconsistency. I guess it's only "inconsistent" if you think of foo and foo={true} as equivalent, which perhaps they only are in the presence of components...

@ryansolid
Copy link
Member

I'm not sure it is possible to do just that. Because at that point all we know is foo = true so all spread can do is treat it the same. And boolean attribute foo on a Component I think has to be true. So really only 2 options. I guess technically 3. React/Preact/Vue/Inferno, Svelte/Solid, or treat all booleans as boolean attributes by default.

@ryansolid
Copy link
Member

Yeah I think Svelte is good company here. Let's leave this as is with one exception. I was handling it the Preact/React way in SSR. I will change that and hopefully that will be consistent enough. Dynamic probably needs to be part of the compiler. I'm not sure exactly what that looks like but I think that is the only way we deal with some of the desired gaps.

@ryansolid
Copy link
Member

Closing with 1.5 release.

@trusktr
Copy link
Contributor

trusktr commented May 12, 2024

Continued in

Offroaders123 added a commit to Offroaders123/Smart-Text-Editor that referenced this issue Sep 26, 2024
Slowly breaking this one into it's own component, things are still breaking even though I'm trying for them not to. I think the issue is with the getters and setters for the refs not being updated by way of `query()` and the `EditorElement.state` property, which I'm trying to build the state off of.

https://docs.solidjs.com/reference/jsx-attributes/attr
solidjs/solid#1101
solidjs/solid#1689
https://www.solidjs.com/docs/latest/api#attr___
https://www.reddit.com/r/MacOS/comments/12xt6jt/do_you_use_natural_scrolling/
https://www.reddit.com/r/apple/comments/tkxtn0/universal_control_should_have_an_option_to_use/

This commit was from the last day or so, wanted to come back with fresh eyes the next day to ensure everything it was doing was correct. With the things I changed, I think it's mostly right. The only problem is the state-related parts I was mentioning before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants