fix($animate): ensure that class-based animations only consider the most recent DOM operations

[matsko]

Prior to this fix $animate would maintain a count of each time a class was
added and removed within $animate. With this fix, $animate instead only cares
about the most recent addClass or removeClass operation and will only perform
that operation (depending on what was last called).

// before
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +2
removeClass => +1
// this will cause an addClass animation

// now
addClass    => add
removeClass => remove
addClass    => add
addClass    => add
removeClass => remove
// this will cause a rmeoveClass animation

Closes #8946

[IgorMinar]

this looks complicated. Why not use Math.abs(count) < 1 as the guard?

[caitp]

question: why are we counting anything at all? -1 -> class was removed last time, +1 -> class was added --- this way you get the most recent result. It doesn't matter if the same classes was added N times or removed N times, only the most recent operation matters

[caitp]

and for that matter, why are we doing this push('+' + class) / push('-' + class) thing? arrays might be slightly better (perf-wise) than doing this with objects (at least in V8), but for multiple operations on a single class, I think we actually do better with the object instead. In any case, the object path is simpler, and since there's probably no good reason to do the array thing, it just strikes me as a much better option.

[matsko]

@IgorMinar when here's why:

when VAL == -1 then it's removeClass
when VAL == 0 then nothing happens
when VAL == 1 then addClass

The idea here is that we can have one of three values. Using Math.abs(VAL) < 1 can result in true or false but not an even value of zero. This wouldn't work with what we have here since we need to know when classes cancel each other out.

[matsko]

@caitp unless you can provide a working example using another, more efficient mechanism, then I'm sticking with what I have here. Please provide a benchmark to prove to me that your example works more effectively.

[caitp]

The one test that you added will not fail using the approach I showed, so if you're making it fail you're doing something different. It's simple, when you add a class, we set the state of the operation for that class to 1, and when you remove a class, you set it to some other value (in this case -1) --- because of this, only the most recent operation for any one class will matter, which is what we want (I'm not seeing any other scenario in your test case). When adding or removing classes, we can also check whether the element already has or does not have the class, but realistically this doesn't matter a whole lot (it just helps for testing).

So, under what circumstance (which is not being tested here) do we care about the "when the classes cancel each other out"?

[caitp]

Or "the order that classes are operated on", for that matter --- since the manipulation happens so close together, this shouldn't matter to the layout engine

[matsko]

Based on what you've commented, the snippet below doesn't involve the addClas/removeClass operation cancelling itself out right? Have a closer look.

$animate.addClass(element, 'two');
$animate.addClass(element, 'two');
$animate.removeClass(element, 'two');
$rootScope.$digest();
$animate.triggerReflow();
expect(log.length).toBe(0);
$animate.triggerCallbacks();

The usecase here is that if one directive (such as ngClass) performs an addClass operation and then a removeClass operation within the same digest then there is no point in running an animation.

[caitp]

$animate.addClass(element, 'two'); // cache.two === 1, element doesn't have .two
$animate.addClass(element, 'two'); // cache.two === 1, element doesn't have .two
$animate.removeClass(element, 'two'); // cache.two === -1, element doesn't have .two
$rootScope.$digest(); // nothing happens, because we wanted to remove a class that the element didn't have
$animate.triggerReflow();

[caitp]

basically this means $$addClassImmediately and $$removeClassImmediately are never called

[matsko]

@caitp yes you're correct about the hasClass operation detecting the class at the end of the resolveElementClasses function. This makes the addClass/removeClass counting unnecessary.

I've updated the code for this fix to work. Thanks for being persistent on this. Otherwise I wouldn't have changed it.

[caitp]

note that hasClass() really just makes it nicer for testing, but it shouldn't really affect applications --- so if we do care about perf, we could remove it and just mock a fake version that uses it when testing