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

ngClass is too greedy #4960

Closed
matsko opened this issue Nov 14, 2013 · 1 comment
Closed

ngClass is too greedy #4960

matsko opened this issue Nov 14, 2013 · 1 comment

Comments

@matsko
Copy link
Contributor

matsko commented Nov 14, 2013

ngClass and {{ class }} are too greedy when dealing with class value changes.

For example:

<div ng-click="disableOne()" ng-class="{one:one, two:two, three:three}"></div>

If you set:

//addClass('one two three')
$scope.one = true;
$scope.two = true;
$scope.three = true;

$scope.disableOne = function() {
  //removeClass('one two three')
  //addClass('two three')
  $scope.one = false;
}

Then what will happen is it will remove all the classes first and then add back .two + .three.

This causes an issue with animations because if you have a transition on one of the classes (say .two) then the removeClass animation will be skipped since addClass happens right after cancelling out the removeClass animation, but this shouldn't happen in the first place since there should not be an addClass operation at all if only one ngClass value is set to false.

matsko added a commit to matsko/angular.js that referenced this issue Nov 15, 2013
ngClass works by removing all the former classes and then adding all the
new classes to the element during each watch change operation. This may
cause transition animations to never render. The ngClass directive will
now only add and remove the classes that change during each watch operation.

Closes angular#4960
@vojtajina
Copy link
Contributor

The problem is, there are many guys fighting to set the class:

  • from linking fn, using attr.$set
  • directly through jQuery/jqLite API
  • interpolation (eg. class="one {{x}}")
  • using ng-class (also ng-class-odd, ng-class-even)

There is IMHO no way to give everybody the power to "set" the value. Whoever is changing the class has to also pass its previous value (the previous value that this particular guy set before, not the actual class value at that time).

I suggest Attributes object takes care of it and everybody else does change the class through attr.$set API. The Attributes object will do a diff and add/remove classes (similar to current tokenDifference). This will work even if people use jQuery to add/remove classes (eg. third-party components). It will also solve this issue.

It is basically splitting the shared memory (full class name) into chunks and every component only changes its chunk (set of classes).

The only problem I can think of it when two components use the same class name. For instance, a directive set class to "a b" and interpolation sets the class to "b c". At that point, class is "a b c". Then, if the directive sets the class to "a", it will remove "b" class, even though the interpolation still expects it to be there.

@matsko matsko mentioned this issue Nov 19, 2013
@matsko matsko closed this as completed in 6b8bbe4 Nov 20, 2013
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
ngClass works by removing all the former classes and then adding all the
new classes to the element during each watch change operation. This may
cause transition animations to never render. The ngClass directive will
now only add and remove the classes that change during each watch operation.

Closes angular#4960
Closes angular#4944
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
ngClass works by removing all the former classes and then adding all the
new classes to the element during each watch change operation. This may
cause transition animations to never render. The ngClass directive will
now only add and remove the classes that change during each watch operation.

Closes angular#4960
Closes angular#4944
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants