ngIf repeats itself instead of updating itself in 1.2.0 #4852

Closed
jnizet opened this Issue Nov 9, 2013 · 18 comments

Comments

Projects
None yet
10 participants
@jnizet
Contributor

jnizet commented Nov 9, 2013

Elements using ngIf aren't updated (or removed and recreated) when using a string as the condition and this string changes. This worked fine in angular 1.2.0-rc2, but doesn't work in 1.2.0.

See this plunkr for example: http://plnkr.co/edit/b6CUTqTbZIAhhZYMOXBu?p=preview

Fill me: <input type="text" ng-model="test" /><br/>
Show when filled:
<span ng-if="test">
  I'm added when the text is filled.
</span>

Each time a new char is added to the textbox, a new span is added to the page.

Workaround: use a real boolean condition:

Fill me: <input type="text" ng-model="test" /><br/>
Show when filled:
<span ng-if="!!test">
  I'm added when the text is filled.
</span>
@lucsky

This comment has been minimized.

Show comment
Hide comment
@lucsky

lucsky Nov 9, 2013

Confirmed, it happens in one of my apps. Angular 1.2.0 is broken in many different ways which is ironic for a framework that Google advertises as inherently testable... :>

lucsky commented Nov 9, 2013

Confirmed, it happens in one of my apps. Angular 1.2.0 is broken in many different ways which is ironic for a framework that Google advertises as inherently testable... :>

@igorzg

This comment has been minimized.

Show comment
Hide comment
@igorzg

igorzg Nov 9, 2013

Thy exchanged behavior, according to new documentation on http://docs.angularjs.org/api/ng.directive:ngIf

The ngIf directive removes or recreates a portion of the DOM tree based on an {expression}. If the expression assigned to ngIf evaluates to a false value then the element is removed from the DOM, otherwise a clone of the element is reinserted into the DOM.

So this is not a bug...

igorzg commented Nov 9, 2013

Thy exchanged behavior, according to new documentation on http://docs.angularjs.org/api/ng.directive:ngIf

The ngIf directive removes or recreates a portion of the DOM tree based on an {expression}. If the expression assigned to ngIf evaluates to a false value then the element is removed from the DOM, otherwise a clone of the element is reinserted into the DOM.

So this is not a bug...

@jnizet

This comment has been minimized.

Show comment
Hide comment
@jnizet

jnizet Nov 9, 2013

Contributor

@igorzg The I would see it as a bug of the documentation in addition to a bug in the implementation :)
IMHO, ngIf is supposed to do what anyone would expect from a directive with this name: if the expression is falsy, the node shouldn't be in the dom, and if it's truthy, it should be in the DOM. Having several instances of the node is just not acceptable for ngIf.

Contributor

jnizet commented Nov 9, 2013

@igorzg The I would see it as a bug of the documentation in addition to a bug in the implementation :)
IMHO, ngIf is supposed to do what anyone would expect from a directive with this name: if the expression is falsy, the node shouldn't be in the dom, and if it's truthy, it should be in the DOM. Having several instances of the node is just not acceptable for ngIf.

@igorzg

This comment has been minimized.

Show comment
Hide comment
@igorzg

igorzg Nov 10, 2013

@jnizet I agree with you, if expression property is empty that should be false and it make no sense for us to pass it to ngif as boolean notation ( ng-if="!test" -> false and ng-if="!!test" -> true ) , that make no sense :).

But bug is in this function while is ng-if="test":

function toBoolean(value) {
  if (value && value.length !== 0) {
    var v = lowercase("" + value);
    value = !(v == 'f' || v == '0' || v == 'false' || v == 'no' || v == 'n' || v == '[]');
  } else {
    value = false;
  }
  return value;
}

because if you type f or 0 or no or n or [] or false in input field ngif will not show a expected behavior it will not clone element as is specified in documentation.

And when is evaluated as boolean ( ng-if="!test" -> false and ng-if="!!test" -> true ) when value is always true or false then it works.

igorzg commented Nov 10, 2013

@jnizet I agree with you, if expression property is empty that should be false and it make no sense for us to pass it to ngif as boolean notation ( ng-if="!test" -> false and ng-if="!!test" -> true ) , that make no sense :).

But bug is in this function while is ng-if="test":

function toBoolean(value) {
  if (value && value.length !== 0) {
    var v = lowercase("" + value);
    value = !(v == 'f' || v == '0' || v == 'false' || v == 'no' || v == 'n' || v == '[]');
  } else {
    value = false;
  }
  return value;
}

because if you type f or 0 or no or n or [] or false in input field ngif will not show a expected behavior it will not clone element as is specified in documentation.

And when is evaluated as boolean ( ng-if="!test" -> false and ng-if="!!test" -> true ) when value is always true or false then it works.

@igorzg

This comment has been minimized.

Show comment
Hide comment
@igorzg

igorzg Nov 10, 2013

@jnizet This one is not closed , the ticket #4863 is closed because it's duplicate to this ticket :D

igorzg commented Nov 10, 2013

@jnizet This one is not closed , the ticket #4863 is closed because it's duplicate to this ticket :D

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@matthughes

This comment has been minimized.

Show comment
Hide comment
@matthughes

matthughes Nov 11, 2013

I do have a pull request for this with tests (PR 4894). Is this the right fix?

I do have a pull request for this with tests (PR 4894). Is this the right fix?

@ghost ghost assigned tbosch Nov 11, 2013

tbosch added a commit to tbosch/angular.js that referenced this issue Nov 12, 2013

@tbosch

This comment has been minimized.

Show comment
Hide comment
@tbosch

tbosch Nov 12, 2013

Member

Hi @matthughes, thanks for your PR. However, the solution of @IgorMinar was simpler, so I used that. Also added another test case.

Member

tbosch commented Nov 12, 2013

Hi @matthughes, thanks for your PR. However, the solution of @IgorMinar was simpler, so I used that. Also added another test case.

@jnizet

This comment has been minimized.

Show comment
Hide comment
@jnizet

jnizet Nov 16, 2013

Contributor

This fix avoids the repetition of the block with ngIf, but it still uses truthiness/falsiness rules that are not the same as the standard JavaScript ones, which makes things confusing and unintuitive to a JS programmer.

See this plunkr (which is the same as the original one in the bug report but uses angular 1.2.1).

One would expect that the block is displayed only if the field is filled, because the JavaScript falsy values are undefined, null, NaN, 0, "" (empty string), and false, and all the rest is truthy. But if you fill the field with false, no or f, the span disappears, although "false", "no" and "f" are truthy values.

So the workaround consisting in transforming the expression to a true boolean (ng-if="!!test") is still necessary.

Voting to reopen.

Contributor

jnizet commented Nov 16, 2013

This fix avoids the repetition of the block with ngIf, but it still uses truthiness/falsiness rules that are not the same as the standard JavaScript ones, which makes things confusing and unintuitive to a JS programmer.

See this plunkr (which is the same as the original one in the bug report but uses angular 1.2.1).

One would expect that the block is displayed only if the field is filled, because the JavaScript falsy values are undefined, null, NaN, 0, "" (empty string), and false, and all the rest is truthy. But if you fill the field with false, no or f, the span disappears, although "false", "no" and "f" are truthy values.

So the workaround consisting in transforming the expression to a true boolean (ng-if="!!test") is still necessary.

Voting to reopen.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Nov 16, 2013

Contributor

It is a trivial change to toBoolean to fix that, but it may also break some/many apps who use the weird falsy-string rules. A quick grep of the source tree shows it used by input directives, ngIf, ngShow/ngHide, and the orderBy filter.

edit it seems that it is not really intended for use by expressions, and things which need logic similar to javascript should probably not use it, hm.

Contributor

caitp commented Nov 16, 2013

It is a trivial change to toBoolean to fix that, but it may also break some/many apps who use the weird falsy-string rules. A quick grep of the source tree shows it used by input directives, ngIf, ngShow/ngHide, and the orderBy filter.

edit it seems that it is not really intended for use by expressions, and things which need logic similar to javascript should probably not use it, hm.

@jnizet

This comment has been minimized.

Show comment
Hide comment
@jnizet

jnizet Nov 16, 2013

Contributor

Until 1.2-rc2, Angular didn't have this bug (in ngIf at least). I skipped rc3. So this new behavior will break all the apps that were written with 1.2rc2 or sooner (or maybe even rc3 or sooner), whereas removing it will break the ones that have relied on these new rules introduced in rc3 or later. So apps will break whatever the decision is.

My humble opinion is that Angular sells itself as a "plain old JavaScript" framework, and that these falsiness rules are not JavaScript and are counter-intuitive. So I still vote to reopen this bug and stick to standard JavaScript rules.

Contributor

jnizet commented Nov 16, 2013

Until 1.2-rc2, Angular didn't have this bug (in ngIf at least). I skipped rc3. So this new behavior will break all the apps that were written with 1.2rc2 or sooner (or maybe even rc3 or sooner), whereas removing it will break the ones that have relied on these new rules introduced in rc3 or later. So apps will break whatever the decision is.

My humble opinion is that Angular sells itself as a "plain old JavaScript" framework, and that these falsiness rules are not JavaScript and are counter-intuitive. So I still vote to reopen this bug and stick to standard JavaScript rules.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Nov 16, 2013

Contributor

The ng-if directive has relied on toBoolean (which itself has not changed since 2010) since it was adapted from ui-if earlier this year, however that is not necessarily correct behaviour (ui-if always used the boolean-cast of an angularjs expression to determine truthiness). So it would take some investigating to see exactly how it broke your app between release candidates (to be fair, not a single release that ng-if has showed up in until 1.2.0 has been considered stable)

I've hanged my mind about changing toBoolean (although it might be much more liberal than it necessarily should be), but there's no reason ng-if couldn't be changed to not rely on it any longer, and instead behave more like ui-if did.

Contributor

caitp commented Nov 16, 2013

The ng-if directive has relied on toBoolean (which itself has not changed since 2010) since it was adapted from ui-if earlier this year, however that is not necessarily correct behaviour (ui-if always used the boolean-cast of an angularjs expression to determine truthiness). So it would take some investigating to see exactly how it broke your app between release candidates (to be fair, not a single release that ng-if has showed up in until 1.2.0 has been considered stable)

I've hanged my mind about changing toBoolean (although it might be much more liberal than it necessarily should be), but there's no reason ng-if couldn't be changed to not rely on it any longer, and instead behave more like ui-if did.

@jnizet

This comment has been minimized.

Show comment
Hide comment
@jnizet

jnizet Nov 16, 2013

Contributor

Sorry. Forget about my previous message. I noticed the bug when blocks started to repeat themselves, and there is a good chance that the truthiness/falsiness bug existed before but I didn't notice it.

Contributor

jnizet commented Nov 16, 2013

Sorry. Forget about my previous message. I noticed the bug when blocks started to repeat themselves, and there is a good chance that the truthiness/falsiness bug existed before but I didn't notice it.

@tbosch

This comment has been minimized.

Show comment
Hide comment
@tbosch

tbosch Nov 21, 2013

Member

I think it's good that all directives that evaluate a boolean have the same semantics. Changing those is possible, but would be a breaking change. So I would rather leave it the way it is.
However, if you feel strongly about changing this, you can file a new issue....

Member

tbosch commented Nov 21, 2013

I think it's good that all directives that evaluate a boolean have the same semantics. Changing those is possible, but would be a breaking change. So I would rather leave it the way it is.
However, if you feel strongly about changing this, you can file a new issue....

jamesdaily added a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014

jamesdaily added a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014

@Crotery

This comment has been minimized.

Show comment
Hide comment
@Crotery

Crotery Mar 11, 2016

after updating to angular@1.5.0 (from npm) we got exactly same issue

we had to change all ng-if's to exact boolean expressions all over the project. Not good!

Crotery commented Mar 11, 2016

after updating to angular@1.5.0 (from npm) we got exactly same issue

we had to change all ng-if's to exact boolean expressions all over the project. Not good!

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Mar 23, 2016

Member

@Crotery, I'm not sure what error you are talking about, but if you found something that you believe needs fixing, please open a new issue (providing the necessary details).

Member

gkalpak commented Mar 23, 2016

@Crotery, I'm not sure what error you are talking about, but if you found something that you believe needs fixing, please open a new issue (providing the necessary details).

@Crotery

This comment has been minimized.

Show comment
Hide comment
@Crotery

Crotery Mar 31, 2016

I am talking about the @jnizet 's error from first message in this thread.
This is unexpected behavior and, I think, should be changed somehow. I leave this choice to contributors, because this needs much knowledge of angular source than I have.

Crotery commented Mar 31, 2016

I am talking about the @jnizet 's error from first message in this thread.
This is unexpected behavior and, I think, should be changed somehow. I leave this choice to contributors, because this needs much knowledge of angular source than I have.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Mar 31, 2016

Member

@Crotery, I can't reproduce the error. If you are still seeing this on v1.5.x, please open a new issue providing the necessary details to reproduce the problem (e.g. a minimal example or better yet a live demo using CodePen, Plnkr etc).

Member

gkalpak commented Mar 31, 2016

@Crotery, I can't reproduce the error. If you are still seeing this on v1.5.x, please open a new issue providing the necessary details to reproduce the problem (e.g. a minimal example or better yet a live demo using CodePen, Plnkr etc).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment