Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

When I mix class + expression with ng-class, the ng-class is not being compiled if expression is broken or fails to return anything #9109

Closed
iladarsda opened this issue Sep 16, 2014 · 17 comments

Comments

@iladarsda
Copy link

I noticed when I mix ng-class with class + expression (class="something-{{data}}"), if the expression fails to return anything (does not exist for example), than the ng-class will not get compiled at all.

Also, it only happens when the broken expression is in the middle of the class <div class="stuff-{{ok}} another-{{nothing}} third-{{ok}}"></div>.

Here is fiddle example: http://jsfiddle.net/seefeld/4z4s4hhs/6/

@iladarsda iladarsda changed the title When I mix class + expression with ng-class, the ng-class is not being compiled if expression fails to return anything When I mix class + expression with ng-class, the ng-class is not being compiled if expression is broken or fails to return anything Sep 16, 2014
@gkalpak
Copy link
Member

gkalpak commented Sep 16, 2014

I fact it doesn't happen when you say it does.
It happens when there are two or more interpolations (even on the same class).

Here is the updated fiddle: http://jsfiddle.net/ExpertSystem/4z4s4hhs/15/

BTW, this seems to have been broken in v1.3.0-beta.19.
It works as expected in v1.2.25 and all 1.3.0 betas up to v1.3.0-beta.18.

@gkalpak
Copy link
Member

gkalpak commented Sep 16, 2014

Let's assume the following html:

<div class="color-{{color}} size-{{size}}" ng-class="(size>10)?'font-large':'font-small'">
    ...
</div>

evaluated against the following context: {color: 'red', size: 20}


The complexity of this is kind of beyond my reach, but I'll just post my observations from trying to track the problem down (in hope of saving someone some time):

(A)
There is a difference in the sequence of events between 1.3.0-beta.19 (which introduced the issue) and 1.3.0-beta.18.

  • In beta.18:
    1. The callback of the watcher of attribute-interpolation gets executed, which sets the element's className to color-red size-20.
    2. The callback of the watcher of ngClass gets executed which sets the element's className to color-red size-20 font-large.

This is the expected outcome.

  • In beta.19:
    1. The callback of the watcher of ngClass gets executed first, which sets the element's (still uninterpolated) className to color-{{color}} size-{{size}} font-large.
    2. The callback of the watcher of attribute-interpolation gets executed, which sets the element's className to color-red size-20. (Notice how the font-large class was removed.)

This happens (or so I say), because attribute-interpolation interpolates the string color-{{color}} size-{{size}}, then ngClass changes the className, then interpolation determines that the original string should color-red size-20 and (without being aware that the class "attribute" has been changed) it sets its value to the result of the intrpolation of the original string.

(B)
I believe (without being able to prove it) that the above behaviour is caused by introducing asynchronous evaluation (by means of $evalAsync()) inside the $watchGroup() method:

$watchGroup: function (watchExpressions, listener) {
    ...
    forEach(watchExpressions, function (expr, i) {
        var unwatchFn = self.$watch(expr, function watchGroupSubAction(value, oldValue) {
            ...
            self.$evalAsync(watchGroupAction);

(CCing @IgorMinar because he seems to be the one introducng $evalAsync())

(C)
If we only have one interpolation in the className (e.g. class="color-{{color}}"), then the above example works as expected. Why ? I will tell you why :)
Because $watchGroup() treats a watchExpressions array with just one element differently:

$watchGroup: function (watchExpressions, listener) {
    ...
    if (watchExpressions.length === 1) {
        return this.$watch(watchExpressions[0], function watchGroupAction(value, oldValue, scope) {
            ...
            listener(...);   // <----- See ? No `$evalAsync()` here.

(D)
Again I can't prove it (yet), but I have a very strong feeling that this problem affects all attributes (not just class).
Basically, I believe that this problem is likely to arise any time there is an attribute whose value is an interpolated string and someone (a directive) also changes the value of that attribute (not replacing it, but modifying it).

Generally, I think attribute interpolation should check (how?) that there has been no modification to the original template string (based on which the new value has been computed). If the original template string has been modified, then the computed value is useless and interpolation should rerun.

I am not sure this is indeed the problem, I am just speculating here.
But if it is, I could imagine a solution to be something along these lines:

  1. Inside the watch callback add a check: a comparison between the original template string (a reference to it needs to be kept) and the current template string*.
  2. If they are the same, proceed as usual.
  3. If they are different, throw the computed value away and start all over again.

*: In order for this to be feasible, there needs to be a way to extract the template string from the current interpolated string. I.e.:

  1. Initially: Template string: color-{{color}}, String value: color-{{color}}
  2. Resolve {{color}} to red: Template string: color-{{color}}, String value: color-red
  3. Someone modifies the string: String value: color-red hello-world
    Somehow, we need to be able to infer the new template string from this: color-{{color}} hello-world
  4. Things can get ugly if someone modifies part of the string that belongs to the original template.

Oooor...maybe I am overthinking this. The solution might be totally different.


Anyway, sorry for the large comment and let me know if I can provide any further help :)

@iladarsda
Copy link
Author

I am impressed. Thanks for the in-depth explanation. I am working around the issue by making sure I only bind expressions that I am sure will return, or avoiding mixing class with ng-class all-together.

@gkalpak
Copy link
Member

gkalpak commented Sep 17, 2014

@seefeld:
Jut making sure I am clear: The problem has absolutely no connection to whether the expression will return something or not (as demonstrated in my fiddle - see second example).

The problem arises only when ngClass is used together with a className that contains 2 or more interpolations, basically 2 or more pairs of {{...}} (be it on the same class or on different ones).

So, avoiding mixing ngClass with interpolated classNames should work around the problem (until this is fixed).

@caitp
Copy link
Contributor

caitp commented Sep 26, 2014

@gkalpak's interpretation of the issue looks right --- I'm not sure why we're deferring invokation of the listener (this seems a bit weird).

I'd like to get this fixed properly, but I don't want to write a patch until I know specifically what we're doing this for

@tedbeer
Copy link

tedbeer commented Jan 19, 2015

Escalate!
ng-hide and ng-show are also affected - http://jsfiddle.net/tedbeer/4z4s4hhs/16/

@havenchyk
Copy link

@gkalpak sorry if it's not a best place to ask, but is there any difference between using of ng-class="myVar" and class="{{myVar}}" in performance? If I understand correctly, the same directive will be used for both cases, but I believe that ng-class is faster because it doesn't need to evaluate interpolated string from class. Could anyone clarify?

@gkalpak
Copy link
Member

gkalpak commented Jul 16, 2015

@havenchyk, I don't think there is any difference in performance, but it there are differences:

  1. It is not the same directive. In fact ngClass is a directive, but in the latter case it will be plain old interpolation setting the value of the class attribute. Under the hood, text interpolation in attributes is also implemented as a directive, but a different one.
  2. The ngClass uses $animate to add/remove classes, so it can be used for hooking into animations.
  3. As a direct consequence of the above, there will be differences in timing (i.e. when a class is added/removed if myVar changes). E.g. $animate.add/removeClass() will defer the actual DOM operation until after the digest (but in most cases it is an implementation detail that doesn't matter.)

Bottom line(s):
They are pretty different under the hood. If anything else, class is more straightforward (so it might be marginally more efficient, but nothing noticable), ngClass offers more features.
In the case of a simple myVar (i.e. not object), I don't it makes any difference (other than the timing).

@havenchyk
Copy link

@gkalpak thanks a lot!

@plong0
Copy link

plong0 commented Jul 16, 2015

FWIW, I am encountering this bug, but only when there is an ng-if attribute on the element with the ng-class + (2 or more) interpolated class values.

I can confirm that it occurs only when there is more than 1 interpolated values in the class attribute.

@petebacondarwin
Copy link
Contributor

This appears to be fixed in 1.4: http://jsfiddle.net/d2gcvd3g/

@petebacondarwin
Copy link
Contributor

And in 1.3.18: http://jsfiddle.net/1guyaxob/

@gkalpak
Copy link
Member

gkalpak commented Sep 14, 2015

Nice :) Thx @petebacondarwin for verifying !
(Accidentally fixed by changing the timing of applying classes, I guess.)

The general problem still exists though; it's more explicitly tracked in #12813.

@artuska
Copy link

artuska commented Oct 30, 2015

This issue is not fixed untill latest 1.4.7 version.

I'm using Angular 1.4.7 (also tested in 1.4.5 and 1.4.6) — I have the same issues as described in this topic — if I have 2 or more {{...}} in class attribute then ng-class and ng-show|hide directives stop working.

(In fact, they are not working only in first digest cycle — after the scope is updated second time (after some action which updates the scope, for example), everything works as expected :)

So, here ng-class does not work:
<div class="button button_{{name}} button_{{name}}_{{action}}" ng-class="{'processable': isAllowed(action), 'active': madeAction(action), 'foo': 1, 'bar': true}" ng-click="runButton(action)">

All functions return true, also foo and bar are true... but ng-class just not running and I do not see any processable|active|foo|bar classes in my class attribute.

ng-class starts working in two ways:

  1. If I just remove some {{...}} from the class attribute:
    <div class="button button_{{name}}" ng-class="{'processable': isAllowed(action), 'active': madeAction(action), 'foo': 1, 'bar': true}" ng-click="runButton(action)">
    everything magically starts working.

  2. Or, if I just update the scope (ng-click updates the scope) — both ng-class and ng-show|hide directives start workng as expected.

@petebacondarwin
Copy link
Contributor

@artuska - can you move your report to #12813, where we are continuing to track this bug?

@sneakyfildy
Copy link

You can use ng-class="[cls, addCls, {'if-class': isClass()}]" syntax

markusherzog added a commit to markusherzog/angular-schema-form-bootstrap that referenced this issue Apr 9, 2018
@pawan-logiciel
Copy link

You can use ng-class="{'classOne': true, 'classTwo': false, 'items-tier3-for-reorder-{{firstTierIndex}}-{{secondTierIndex}}-{{thirdTierIndex}}': true}"

This works for me

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