fix($animateCss): avoid flicker caused by temporary classes

[gkalpak]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.

What is the current behavior? (You can also link to an open issue here)
Adding the temporary classes immediately, while forcing a reflow before starting the animation, could cause flickering. Currently, only ngHide/ngShow animations are affected (but theoretically any future animation relying on tempClasses would be).
See #14015.

What is the new behavior (if this is a feature change)?
The temporary classes are not applied until the animation actually starts, which avoids the flickering.

Does this PR introduce a breaking change?
No. (Since tempClasses was an undocumented, private feature (currently only used for ngHide/ngShow), no other built-in or custom animations will be affected).

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:
This is an alternative approach to #15463.
Fixes #14015.

[Narretz]

I think that's good.
Ideally, not every drive would have to add the temp classes, but since there are no custom drivers (to my knowledge), this is irrelevant.

Just to clarify, before this change was it possible to use a temporary class to trigger a css animation? I.e. a temporary class applies a transition that makes the animator detect an animation.

[gkalpak]

Just to clarify, before this change was it possible to use a temporary class to trigger a css animation? I.e. a temporary class applies a transition that makes the animator detect an animation.

It was and it still is. This PR only affects adding the classes in beforeStart(), at which point the animations are determined. The tempClasses are used elsewhere, so this PR does not affect that:
Animaton works with this PR)

[Narretz]

Actually, the current ngHide example from the docs doesn't work correctly for the "show" (ng-hide-remove) action with this patch, see https://plnkr.co/edit/tyteqvfPbhcwrBKXpn5c?p=preview

When you comment in the patched angular-animate (copied from you pen), the show action doesn't animate anymore.

[gkalpak]

😞 It's always possible that I've missed something. If #15463 works, I'm happy to go with that.
_{I've had too much ngAnimate lately to investigate further 😩}

[Narretz]

My PR actually has the same problem ... the thing is that the plnkr example I posted puts the actual transition css on the temporary class, which is not something we advocate. I'm not sure where I got this plnkr / css from, maybe an issue or something?
So if we are okay that this way of defining animations is wrong, then we could use this PR.

Though the ng-hide-animate class is definitely known by devs (it's in the docs, and used to work around this bug), so we can't really argue that the temporary class feature is an implementation detail.

[gkalpak]

The question is: Can we fix the issue without breaking the usecase?

[Narretz]

I don't think so. The rAF call that causes this is elementary to the way ngAnimate works. Removing it or working around it would probably mean a big rewrite.

[gkalpak]

In that case, I vote for fixing it with a breaking change (i.e. that animation styles on temporary classes won't work) for 1.7.0.

[Narretz]

Bad news: I've re-tested the plnkr with our patches, and even with the regular ngShow / ngHide example from the docs, they don't work for showing: https://plnkr.co/edit/tyteqvfPbhcwrBKXpn5c?p=preview (the current ngAnimate file is active, but use the patched files and it doesn't work).
I thought this didn't work because the transition was set on the temporary class, but this is not the case.

[Narretz]

Here's the original repro to test again: https://plnkr.co/edit/l70kaJ?p=preview (flickers in IE11reliably)

[gkalpak]

Yeah, I realized our both our fixes are problematic. I started looking into the issue again 😞

[gkalpak]

I give up 😱 Unfortunately, I couldn't get to the bottom of this in a reasonable amount of time 😞
It only happens on IE for me (which is not great for debugging 😁) and there are work-arounds available:

1. If you don't mind destroying and recreating the view, you can use ng-switch.
2. If you want to keep the view around (but hidden), you can use ng-class="{'ng-hide': checked}"

In any case, I am going to close this PR, because it breaks a valid usecase, so is not an appropriate fix anyway.