NgClass is not evaluating array expressions

[GiDW]

I'm submitting a ...

• bug report
• feature request
• other (Please do not submit support requests here (see above))

Current behavior:

When using ng-class with an expression that evaluates to an array, the classes are not added.

Expected / new behavior:

The classes defined in the objects of the array should be applied to the DOM element when their expression is true.
Basically the third way in which the ng-class directive operates as explained in the docs.

Minimal reproduction of the problem with instructions:

Toggle 1 should change the background color of both boxes.
This works in Angular 1.6.3 but it stops working with Angular 1.6.4.
By changing the angular version number you can see the different behavior.
http://plnkr.co/edit/GARSFyXFLKA00oa4Julz

Angular version: 1.6.4

Browser: [all]

[frederikprijck]

I've updated your plunkr and it's working fine: http://plnkr.co/edit/XIgL7uFKGFXOM06111Ir?p=preview

The reason why you're plunkr is not working is because ngClass does not watch the array for changes on it's items. Instead, it'll only be triggered when the array changes.

To demonstrate this, I updated your original plunkr once again (this time only using a minimal change to show you it's working, just not updating) to set a default style:
http://plnkr.co/edit/AckOI0FGweit33ZGjTEz?p=preview

You can see that the initial style gets applied perfect, however changes are not getting reflected in the UI because the array never changes.

[GiDW]

Thank you for your explanation. I understand what's happening.

So the behavior on Angular <1.6.4 is actually unexpected?

I was under the impression that each digest cycle would check all expressions even if the object was the same.

[frederikprijck]

As ngClass is using $watch (see:

angular.js/src/ng/directive/ngClass.js

Line 39 in d0622d0

scope.$watch($parse(attr[name], toClassString), ngClassWatchAction);

) it doesn't watch for changes in the array's items. To have this behavior when watching arrays, one should be using $watchGroup (see: https://docs.angularjs.org/api/ng/type/$rootScope.Scope ).

The question remains whether or not this is something that should be covered.

Tagging @gkalpak @Narretz :D

[GiDW]

For me this was quite a breaking change when I upgraded to 1.6.4.
The release notes did not really mention anything about this particular change. I saw the standardization note for $parse but I had no idea it had such an impact.

[frederikprijck]

It looks like this was introduced due to: 189461f (not entirely sure tho, but it mentions removing deep watching for ngClass)

So tagging @jbedard aswell.

[GiDW]

I quickly tested a few Angular versions. It works with 1.5.4 and higher until 1.6.3.

[Narretz]

Hm, this is an effect of the refactoring / fix Jason did.
The docs mention the following:

When the expression changes, the previously added classes are removed and only then are the new classes added.

It only talks about the expression as such, not about deep watching. There were also no tests for this, so we didn't purposefully change this. At the moment, I think this is expected behavior.

[frederikprijck]

@Narretz The commit also states:

ng-class is one example of deepEquals which is no longer required.

It looks like it was intended to change the behavior?

[Narretz]

Yes and no ;). What I meant was that we intended to remove deepEquals, but didn't intend to break this behavior, because it was never supported explicitly in the first place (no tests, no clear docs, and afact Jason didn't know that this might be use case).

[GiDW]

One of the bug fixes for 1.5.5 was:

ngClass: fix watching of an array expression containing an object (f975d8d, #14405)

Angular versions before 1.5.4 are behaving the same as 1.6.4 now. So I guess this a bug and not really intended behavior?

[Narretz]

@GiDW thanks for finding this commit. It really looks like for 1.5.5 to 1.6.3 this was an unsupported feature.

[jbedard]

The test for f975d8d still passes (unless it was deleted?). I think my change only effected one-time which was previously deep watched due to a now fixed bug. Is this issue only with one-time? (On mobile so it's a little hard to check the plunkr)

[OzzieOrca]

I'm also experiencing this trying to use a filter to determine whether a validation class should be shown.

An example is:

ng-class="{'has-error': (testForm.testInput | showErrors)}"

where testForm.testInput is a NgModelController

http://plnkr.co/edit/oVpXjI6RwwN3WN19dPb7?p=preview

I suppose I could figure out another solution but it doesn't seem to be as clean. I'm open to suggestions.

I figured I'd comment since this also seems to be affecting the object syntax of ng-class.

[jbedard]

Hmm, that expression should still work. Maybe the use of interceptors changes the normal literal watching?

[Narretz]

The test for f975d8d still passes

That's correct. The difference is that the test uses a variable in the expression ng-class="classVar"> where $rootScope.classVar = [{orange: true}] and the plunker uses a literal array with an obj variable: ng-class="[root.obj] (where $rootScope.obj = {orange: true}). Adjusting this, the test fails.

It's also definitely caused by 60394a9

@OzzieOrca example is special again because it's using a filter in the object value (but also a literal in the ngClass expression).

[Narretz]

Maybe the use of interceptors changes the normal literal watching?

I think the origin of the issue is this line: 60394a9#diff-780de070ede30180f6aedb6c5f7d57caR1921

When adding the interceptor, we now mark the regularInterceptedExpression as literal. Previously, when adding the interceptor this information was lost. And being literal obviously changes the change detection ... although I'm not sure about the details.

(The thing is that an expression like [obj] looks literal to the parser, and I think this is the correct thing to do).

[jbedard]

I thought marking it as a literal would handle this exact problem. Maybe that was wrong?

[jbedard]

I'm not too sure what to do here...

I've been trying to remove some of the issues with literals vs non-literals lately so code outside parse.js doesn't have to workaround these. Previously literals often forced the use of deepWatch=true or $watchCollection, or worse it might force directive/component authors to use =* bindings to support literals as binding inputs.

This specific issue was caused by 60394a9#diff-780de070ede30180f6aedb6c5f7d57caR1921 which took advantage of the literal watching enabled by 25f008f#diff-780de070ede30180f6aedb6c5f7d57caR1791

A few solutions:

1. revert 60394a9 :(
2. remove support for non-literal nested objects (keep 60394a9, maybe remove more?)
3. mark toClassString as $stateful since it basically adds state by recursing the object
4. don't propagate the literal flag, but note the disabled test...

Opinions? Other ideas?

[jbedard]

Also note that I think interceptors and filters should be treated the same, or at least very similar. Today they are treated the same for inputs/$stateful. 60394a9 added propagation of the literal flag onto the interceptor, maybe the same should be done for filters as well? That might (partially) fix #15874 as well.

[Narretz]

2. remove support for non-literal nested objects (keep 60394a9, maybe remove more?)

This would affect all expressions. So does something like [var] work as expected in directive bindings? I assume this would be a bigger breaking change.

3. mark toClassString as $stateful since it basically adds state by recursing the object

Sounds technically good, but does it impact performance? Or would there a measurable difference between before- 60394a9 , 60394a9 , and with $stateful?

[jbedard]

I meant only removing support in ng-class which has it's own custom watching logic. Directive bindings wouldn't change.

I would be worried about marking toClassString as $stateful, it might be measurable when used a large ng-repeat etc. Some cases it would be no difference (when watching just var and it is of type object), but other cases would be worse (plain strings and literals, which I think are very common for ng-class).

[jbedard]

I'm leaning toward a breaking change to remove the support for nested array/objects in ng-class (in 1.7, and revert the change in 1.6).

This will align it more with Angular ngClass (based on my 2 minutes of looking at the class and playing on plnkr, please correct me if I'm wrong). Then ng-class can be switched to a simple $watchCollection which should improve performance for every case that I can think of. Will also align it with ng-style, like class/style are aligned in Angular. Also keeping 60394a9 will allow more performance (and code deleting) in other areas, #15947 is the first example but more will be coming!

[jbedard]

This has been fixed in both 1.6 and 1.7 without any breaking changes to ng-class. I will attempt to summarize since there was a lot of confusion (from me) above...

7084dec + 189461f caused some expression-inputs to only be shallow watched (= good!). But in some cases that is not enough (= bugs).

ng-class class was a good example where obj in [obj] would only be shallow-watched even though there is an interceptor (toClassString) that reads state from within obj. Or with [obj | filter] the filters might read state from within obj. Or with [obj + ''] the obj.toString() method might read state from within obj.

b5118ac fixed the filter/+/other cases
b12a0b7 fixed the interceptor case