ngAnimate crash regression between 1.2.13 and 1.2.14

[mgcrea]

I got crashing animations since 1.2.14, usually when the $animate.enter is called while the $animate.leave is running. For instance if you hover very fast between two tooltips.

Worked flawlessly with angular v1.2.13:

• http://plnkr.co/edit/x5DFnQXo7xYKLkY7MoPx?p=preview

Crashes with angular v1.2.14:

• http://plnkr.co/edit/D0cKtVrJSzQWUpdEgSuH?p=preview

Uncaught TypeError: Cannot read property 'length' of null
angular-animate.js:290

Which translate to this line:
https://github.com/angular/angular.js/blob/v1.2.14/src/ngAnimate/animate.js#L284

The only change regarding ngAnimate between v1.2.13 and v1.2.14 seems to be the replace of done && $timeout(done, 0, false); by async(done).

@matsko Do you have any idea where this regression might come from?

---

I've tested ngAnimate#1.2.13 with angular#1.2.15-build.2389+sha.c5f2f58 and it still works, so it looks like this is a bug confined into ngAnimate.

[Narretz]

There was also a huge change regarding setClass / addClass / removeClass:

18c41af

[matsko]

I'm checking it out. I don't think that the callback issue could be to blame, but possibly the way that cancellations are done.

@Narretz yes there were two big commits (one for 1.2.13 and another for 1.2.14) that made the setClass animations possible (1.2.14 made it so that if setClass is not present then it breaks down into addClass + removeClass). This shouldn't have effected it, however, the commit that you posted may be causing the issue due to the way that the runner code was refactored.

[mgcrea]

@Narretz That's strange, the compare view did not show theses changes, the diff might be a bit big for it to work correctly.

Will be happy to help if there is anything I can do.

[matsko]

Still looking into this. I can reproduce the bug, but it's hard to track down.

[mgcrea]

@matsko any news regarding this bug? Should I try to dig into it or is the API not stable enough yet? Thanks!

[matsko]

Hey @mgcrea. Yes I spent a couple of hours yesterday breaking down the issue. I'm still not sure what's causing it. I'm getting close.

[mgcrea]

@matsko to me it looks like the issue is that when $tooltip.show() is called while $animate.leave() (from $tooltip.hide()) is still running, in that case I do remove the DOM element, that seems to break $animate.

tooltip.js#L180

// Remove any existing tipElement
if(tipElement) tipElement.remove();

Here is the logging details of the events leading to the issue:

show "edit" before remove/relink (!tipElement ? true) tooltip.js:179
show "edit" after remove/relink (!tipElement ? false) tooltip.js:184
hide "edit" (!tipElement ? false) tooltip.js:233 // mouseleave edit button
show "delete" before remove/relink (!tipElement ? true) tooltip.js:179 // mouseenter delete button
show "delete" after remove/relink (!tipElement ? false) tooltip.js:184
hide "delete" (!tipElement ? false) tooltip.js:233 // mouseleave delete button
show "edit" before remove/relink (!tipElement ? false) tooltip.js:179 // mouseenter edit button, while the tip is disappearing
show "edit" after remove/relink (!tipElement ? false) tooltip.js:184
hide "edit" (!tipElement ? true) tooltip.js:233
Uncaught TypeError: Cannot read property 'length' of null angular-animate.js:290

I find it very strange that tipElement is null in the last hide from "edit" as it just have been relinked. In that case $animate is called with a null element, hence the bug.

---

Changing if(tipElement) tipElement.remove(); with if(tipElement) tipElement[0].style.display = 'none'; does fix the issue on the AngularStrap side (no more $animate.leave() crash), interestingly enough, the DOM does not seem to get trashed (overridden references by relink do seem to be removed by $animate, not sure how it does it) by this.

[matsko]

I thought it was the length issue, but it isn't. There's definitely an issue with the tooltip passing in an empty object, but what I'm trying to solve is why the code works in 13 and not in 14.

[mgcrea]

@matsko looks like I was wrong about the tipElement.remove(); thing. The real issue is caused by this line in the $animate.leave() callback.

tooltip.js#L228

$animate.leave(tipElement, function() {
  tipElement = null;
});

The scheduling of $animate callbacks has changed between v1.2.13 and v1.2.14 revealing the tooltip bug (if a previous $animate.leave callback is called right after a fresh $animate.enter callback, the tipElement var gets wrongly destroyed and I loose the reference for the next hide that gets a null value).

Regarding the scheduling:

• <= v1.2.13

$animate.enter "edit" !tipElement false tooltip.js:194
$animate.leave "edit" !tipElement false tooltip.js:235
$animate.enter "delete" !tipElement false tooltip.js:194
$animate.enter.done "edit" !tipElement false tooltip.js:196
$animate.leave "delete" !tipElement false tooltip.js:235
$animate.enter "edit" !tipElement false tooltip.js:194 // mouseover the edit button before the previous leave callback has even started
$animate.enter.done "delete" !tipElement false tooltip.js:196
// so far so good
$animate.leave "edit" !tipElement false tooltip.js:235
$animate.enter.done "edit" !tipElement false tooltip.js:196
$animate.leave.done "delete" !tipElement false tooltip.js:237
$animate.leave.done "edit" !tipElement false tooltip.js:237

• = v1.2.14

$animate.enter "edit" !tipElement false tooltip.js:194
$animate.leave "edit" !tipElement false tooltip.js:235
$animate.enter "delete" !tipElement false tooltip.js:194
$animate.enter.done "edit" !tipElement false tooltip.js:196
$animate.leave "delete" !tipElement false tooltip.js:235
$animate.enter "edit" !tipElement false tooltip.js:194 // mouseover the edit button before the previous leave callback has even started
$animate.enter.done "delete" !tipElement false tooltip.js:196
// so far so good
$animate.leave.done "edit" !tipElement false tooltip.js:237 // kill the tipElement refs
$animate.leave "edit" !tipElement true tooltip.js:235 // breaks consequently
Uncaught TypeError: Cannot read property 'length' of null angular-animate.js:290

Using a long animation time .am-fade { animation-duration: 3s; } makes reproducing the bug quite easy.

[gen4sp]

hi, guys!
any progress with fixing this issue?
or instructions to fix it?

[matsko]

@mgcrea I recently revisited this issue? Did you happen to fix this within your framework because I am unable to reproduce the issue locally or within the 1.2.14 plunkr.

[gen4sp]

ok, update to ngStrap to 2.0.2 and ngAnimate to 1.2.16 solve all problems, awesome
sorry for disturb

[mgcrea]

@matsko, Indeed I patched AngularStrap for this specific issue for the v2.0.0 release. You can still reproduce the bug in this frozen plunker if you want.

However, if you take a look at my post from a month ago regarding $animate callback sequencing, it looks like the post v1.2.14 sequencing is broken, so imo there is still a bug to fix somewhere (at least in v1.2.14). I haven't tested the callback sequencing with the latest v1.3 betas.

[matsko]

Sorry @mgcrea things have been really busy lately. Last time I tried to get to the bottom of this bug I was unable to reproduce it and get it to work out of the context of AngularStrap. If you or anyone here has any time this week could you possibly recreate the bug using the latest snapshot of ng/ngAnimate? Then I can get to the bottom of it and get rid of it.

[matsko]

Closing this for now. Please reply incase an isolated example (without AngularStrap) comes out.