fix(DomRenderer): assume boolean attributes when an attribute binding… - #5761

lgtm. but let's wait for @tbosch before merging

CI failing:

Chrome 39.0.2171 (Linux 0.0.0) MdButton a[md-button] should remove disabled anchors from tab order FAILED
    Caught EXCEPTION: type 'String' is not a subtype of type 'bool' of 'boolean expression'. in [isAriaDisabled?"true":"false" in TestApp@0:0]

Imo interpretting false as "remove an attribute" makes sense only for boolean attributes. I need to check what happens with regular DOM if you assign false programatically to an attribute.

@pkozlowski-opensource I'd argue that I want the property handled as a boolean attribute if the assigned value is of type boolean. Otherwise I can assign myBooleanValue.toString() to make it non-boolean.

So, after thinking about it some more I'm +1 for moving stringify logic to a renderer but -1 for removing attributes on false.

IMO this opens a can of worms that we might not to open (or at least not just now). What I mean by this is that all of the sudden we need to think about cases like [attr.foo]="" where people might start different arguments (I've heard already arguments for both removing and not removing an attribute in this case).

It all boils down to deciding what is falsy enough to remove an attribute and with JS it is never easy....

Let's discuss with @tbosch, @mhevery and me before merging this one.
On Thu, Dec 10, 2015 at 9:15 AM Pawel Kozlowski notifications@github.com
wrote:

So, after thinking about it some more I'm +1 for moving stringify logic to
a renderer but -1 for removing attributes on false.

IMO this opens a can of worms that we might not to open (or at least not
just now). What I mean by this is that all of the sudden we need to think
about cases like [attr.foo]="" where people might start different
arguments (I've heard already arguments for both removing and not removing
an attribute in this case).

It all boils down to deciding what is falsy enough to remove an attribute
and with JS it is never easy....

—
Reply to this email directly or view it on GitHub
#5761 (comment).

The current situation is kind of weird, we remove the attribute on null but not on false (boolean).

The options I have considered:

** new prefix [?attr.name] / [boolattr.name] **

It could work. If we pick this solution, I would prefer a very explicit prefix ("boolattr").
I'm afraid that people would copy paste "?attr" (or whatever short name) wihtout paying attention to the "?" and have hard time understanding why it would fail.

** boolean value **

IMO this is a "nicer" solution, assign a boolean value means that you deal with a boolean attribute. The drawback is obviously the rare case where you really want "true" / "false", you have to write "value?.toString" (see some ex in this PR).

also I would prefer that null keeps the attribute with a value of "", but that would be a bigger breaking change

Also I think the is no question [attr.foo]="" should keep the attribute - how would you really assign an empty string otherwise.

@vicb I hear you. I'm not saying that I've got a perfect solution but at the same time I feel like the current state is slightly less bad as compared to [attr.foo]="false" proposal. Apart from "what is falsy" questions I've mentioned it will add another one: what should happen if we do [atrr.foo]="true"? Shell it be foo, foo="true" or foo="foo"?

[atrr.foo]="true"? Shell it be foo, foo="true" or foo="foo"?

There is a unit test for that :) it will be "", if you need to make it "true" or "foo" you have to be explicit.

There is no falsy/truthy in the current state of the PR, it's either true or false.

This PR as is is not a perfect solution either. I've opened it to start the discussions.

This needs proper design with @mhevery

Can someone poke holes why my simplistic thinking is not correct.

1. [non contentious] Renderer does stringification.
2. [non contentious] null always removes. What does it even mean to have attribute set to null
3. [contentious] Rendered decides which attributes are boolean, in which case false may be interpreted as remove for some attributes. (Render / Platform specific)

But given that we have 2 why do we need 3?

<button [disabled]="false"> would do the right thing since we are binding to property not attribute.
What is the value of writing: <button [attr.disabled]="false"> Why would I want to bind to attribute rather than property. Do we have a use case for this?

Assuming that attribute boolean on false does not sound right to me.

@mhevery some component libraries use attributes for styling (e.g. material
design uses attributes heavily). So the need for binding to attributes that
should appear and disappear is real.

// @jelbourn

On Sun, Dec 13, 2015 at 11:48 AM Miško Hevery notifications@github.com
wrote:

Can someone poke holes why my simplistic thinking is not correct.

1. [non contentious] Renderer does stringification.
2. [non contentious] null always removes. What does it even mean to
   have attribute set to null
3. [contentious] Rendered decides which attributes are boolean, in
   which case false may be interpreted as remove for some attributes. (Render
   / Platform specific)

But given that we have 2 why do we need 3?

<button [disabled]="false"> would do the right thing since we are binding
to property not attribute.
What is the value of writing: <button [attr.disabled]="false"> Why would
I want to bind to attribute rather than property. Do we have a use case for
this?

Assuming that attribute boolean on false does not sound right to me.

—
Reply to this email directly or view it on GitHub
#5761 (comment).

• I also think that the main use case is styling.
• Intuitively I'd rather have an expression that evaluates to true/false (bool) rather than null/whatever arbitrary string

I'm starting to think that an explicit "?" or similar symbol will make
templates easier to read. Otherwise not only do you need to read the
binding but also understand what type the expression evaluates to - that
feels like bad magic to me, but I'm willing to be convinced otherwise if
someone can explain how readability is not a problem.

On Sun, Dec 13, 2015 at 2:21 PM Victor Berchet notifications@github.com
wrote:

• I also think that the main use case is styling.
• Intuitively I'd rather have an expression that evaluates to true/
  false (bool) rather than null/whatever arbitrary string

—
Reply to this email directly or view it on GitHub
#5761 (comment).

0) <div [attr.foo]="null"> I think we cann all agree that null removes.

This gives us three choices of dealing with booleans (Please don't focus on exact syntax, which can be fine tuned later, but rather on semantics of it):

1. <div [attr.foo]="false">
2. <div [?attr.foo]="false"> or <div [boolattr.foo]="false">
3. <div [attr.foo]="false | nullify">

1. treat boolean as special
   • (con) tools can't statically deduce that attribute is boolean. (Does this matter since any attribute could be removed through null assignment)
   • (con) boolean's do not stringify like other types.
2. prefix key side
   • (con) introduces new syntax
   • (pro) tools can statically deduce that attribute is boolean. (Why do we need to know this? How is this different from setting an attribute to null)
3. suffix a pipe
   • (mixed) tools can statically deduce that attribute is boolean like 2, but there could be other filters which have this property which would make it like 1.

Given that null removes the attribute already, I don't see additional benefit to communicate this information to the tools. So I am starting to think that treating boolean type special is a reasonable way forward.

To answer @IgorMinar question. Why do we need to know that an attribute is boolean, given that any expression could be null which removes the attribute. I think this matters for href

<a [attr.href]="expr">

Here we need to be able to remove href if we have no URL in expr, but it is not boolean attribute. (Boolean attributes have no value, they are either there or not.)

What additional information would reader of code gaing from knowing that an attribute is boolean?

Off the top of my head, the only time you would want setting false to an attribute and leave the value in the DOM is for ARIA attributes (such as aria-disabled), which must be present and explicitly "true" or "false". It would be convenient to be able to set those with boolean true / false. In this case, the very presence of the attribute tells you one thing, and then the value tells you another thing. I can't think of another case for doing something like this, but the web is a big place and people imagine some pretty novel stuff (like using [ ] in attribute names, for example...).

As far as styling:
Aside from that ARIA case, if you're going to style based on an attribute that can have "false" as a value, then it's only sensible that that attribute is a boolean attribute, and you can just as easily base your style on the absence or presence thereof. I would call an attribute that can be "true" / "false" / "other" an anti-pattern.

My vote is option No. 2 - it's clear, easily distinguishable from other
attribute bindings by people not deeply familiar with angular (they'll ask
what is this "?" - thing (or whatever syntax we choose) and will find the
answer easily via google or in our docs and cheatsheet.

On Mon, Dec 14, 2015 at 10:48 AM Jeremy Elbourn notifications@github.com
wrote:

Off the top of my head, the only time you would want setting false to an
attribute and leave the value in the DOM is for ARIA attributes (such as
aria-disabled), which must be present and explicitly "true" or "false".
It would be convenient to be able to set those with boolean true / false.
In this case, the very presence of the attribute tells you one thing,
and then the value tells you another thing. I can't think of another case
for doing something like this, but the web is a big place and people
imagine some pretty novel stuff (like using [ ] in attribute names, for
example...).

As far as styling:
Aside from that ARIA case, if you're going to style based on an attribute
that can have "false" as a value, then it's only sensible that that
attribute is a boolean attribute, and you can just as easily base your
style on the absence or presence thereof. I would call an attribute that
can be "true" / "false" / "other" an anti-pattern.

—
Reply to this email directly or view it on GitHub
#5761 (comment).

@IgorMinar I have a different story. When people will see [?attr.foo] they will not check the cheat sheet or google for the "?", they will copy paste the code and ask on gitter (or GH) why their code is not working when they're dealing with non-bool attrs.

we should throw a clear error message if the result of the expression is
not a boolean.

On Mon, Dec 14, 2015 at 3:54 PM Victor Berchet notifications@github.com
wrote:

@IgorMinar https://github.com/IgorMinar I have a different story. When
people will see [?attr.foo] they will not check the cheat sheet or google
for the "?", they will copy paste the code and ask on gitter (or GH) why
their code is not working when they're dealing with non-bool attrs.

—
Reply to this email directly or view it on GitHub
#5761 (comment).

@vicb what is the status of this PR?

I think we should merge the PR as is. 'false' removes the attribute stringify otherwise.

(We have spent a lot of time debating it, and in grand scheme of things it does not matter much which way we chose.)

I would rather keep the current behavior. If we don't have good reasons,
for this change, we might change it anytime in the future again.

On Mon, Feb 1, 2016 at 8:30 PM Miško Hevery notifications@github.com
wrote:

I think we should merge the PR as is. 'false' removes the attribute
stringify otherwise.

(We have spent a lot of time debating it, and in grand scheme of things it
does not matter much which way we chose.)

—
Reply to this email directly or view it on GitHub
#5761 (comment).

ok, lets close the PR then.

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}