fix(ngAnimate): correctly animate transcluded clones with `templateUrl` #15514

gkalpak · 2016-12-16T09:03:26Z

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)
$animate decides whether an animation should be cancelled based on some assumption that don't hold in specific cases (e.g. when animating transcluded clones with templateUrl directives on them for the first time). As a result, the entering elements will not be animated in such cases. This affects commonly used, structural built-in directives (ngIf, ngRepeat, ngSwitch etc).
See also #15510.

What is the new behavior (if this is a feature change)?
All entering animations work as expected (from the first time).

Does this PR introduce a breaking change?
No.

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:
BTW, I believe this is a side effect of lazily compiling transcluded content.

In addition to the last fix commit that fixes the bug, this PR also includes several refactor commits that simplify $animate internally, mainly by removing unnecessary wrapping/unwrapping of DOM nodes in jqLite/jQuery collections. This changes may also positively affect performance, especially in animation-heavy apps (~~although I haven't benchmarked it - yet~~ it does indeed 😉).
(It is probably easier to review one commit at a time.)

Party addresses #14074 and #14124.

Fixes #15510.

…undant calls

…dundant calls

…undant calls

…ve redundant calls

…e redundant calls

…calls

gkalpak · 2016-12-17T13:59:43Z

I added one more commit with a (very basic) animation benchmark. It simply $animate.enters and immediatelly $animate.leaves 1000 elements - via ngRepeat. With the changes proposed in this commit, the benchmark shows a 5%-7% reduction in total execution time (which is not bad at all for a bunch of refatoring commits 😛 - I am tempted to change them to perf).

_{(Take the results with a grain of salt, as the benchmark scenario is a little artificial.)}

mgol · 2016-12-19T18:47:09Z

src/ngAnimate/animateQueue.js

+      var rootNode = getDomNode($rootElement);
+
+      var bodyNodeDetected = (node === bodyNode) || node.nodeName === 'HTML';
+      var rootNodeDetected = (node === rootNode);


Why the parens? (here & in other places)

A couple of reasons (at least):

I find them more readable. I think it is a remnant from my early Java days (and this ancient document).

It is also visually closer to isMatchingElement(...) 😛

_{(Hey, I changed the trailing dots, I am not removing the parens.)}

Looks like we need an eslint rule for that :D But I prefer the parens too

I like not having parents here but as long as ESLint will shout at me if I forget them I'd be satisfied as well. ;)

mgol · 2016-12-21T11:05:50Z

test/ngAnimate/animateSpec.js

+        $templateCache.put('foo.html', '<div>FOO</div>');
+        $compile(parent)($rootScope);
+
+        expect(capturedAnimation).toBe(null);


Everywhere in other places it's only checked if capturedAnimation is falsy. We should be consistent.

So, you want me to change all other places to toBe(null)? Fair enough...

I was thinking the other way round but maybe your idea has more sense. ;)

In some cases it's initially undefined, though, not null, this file is not very consistent in that regard. Perhaps that's material for a separate PR (or at least a commit) but I just want us to be consistent. :)

Ah, it's undefined only in one test:
https://github.com/gkalpak/angular.js/blob/acd9e7b757cdb9f04ae7a12ff21fdc8d27f314b5/test/ngAnimate/animateSpec.js#L21-L46

I guess the changes are fine then.

I (blindly) changed all occurrences of expect(capturedAnimation).toBeFalsy() to expect(capturedAnimation).toBe(null) and the tests were passing 😕

mgol · 2016-12-21T11:12:02Z

benchmarks/animation-bp/app.js

+'use strict';
+
+angular.
+  module('animationBenchmark', ['ngAnimate'], config).


I dislike this dot placement a lot. :P We use the leading dot in most places and I'd like to ESLint-enforce it one day...

Come on! I speeded up your animations 5%-7%. And all you care about is where I put a tiny little dot?

I'd like to ESLint-enforce it one day...

What a coincidence. I'd like to ESLint-enforce the trailing dot one day. So I am tryign to sneak in as many of them as possible, so when the day comes I can say: "Hey, look, there are more trailing dots than leading, let's stick with that 😛

I am afraid I am the only one around to favor the trailing dot, so I'll change it 😞

Come on! I speeded up your animations 5%-7%. And all you care about is where I put a tiny little dot?

No, I like this PR a lot! I'm just providing feedback for some small issues. :)

Good job overall, though, perhaps I should have spelled that out. 👏

_{Just in case it wasn't clear, I was just kidding (in an attempt to distract everyone from the fact that I was busted trying to sneak in those trailing dots 😛}

mgol · 2016-12-21T11:16:10Z

benchmarks/animation-bp/main.html

+  [ng-cloak] { display: none !important; }
+  .animation-container .ng-enter,
+  .animation-container .ng-leave {
+    transition: all 0.1s


A missing semi-colon;

gkalpak · 2016-12-21T12:15:42Z

Addressed (most of) @mgol's comments.

mgol · 2016-12-21T12:17:01Z

test/ngAnimate/animateSpec.js

@@ -168,7 +168,7 @@ describe('animations', function() {
      inject(function($animate, $rootScope) {
        $animate.enter(element, parent);
        $rootScope.$digest();
-        expect(capturedAnimation).toBeFalsy();
+        expect(capturedAnimation).toBe(null);


I'd put those changes in a separate commit as they don't really have a lot in common with the original one.

gkalpak · 2016-12-31T11:21:54Z

Rearranged the commits (and fixed a test failure).

Narretz · 2017-01-04T13:37:56Z

src/ngAnimate/animateQueue.js

+        // Note: We still need to use the old `node` for certain things, such as looking up in
+        //       HashMaps where it was used as the key.
+
+        element = stripCommentsFromElement(originalElement);


I take it this has no performance implications? This now applies to all animations.

I haven't benchmarked this change on its own. I assume it has an unnoticeable impact on performance (considering all the performance-heavy-ish stuff that is going on during animations).

But overall, this PR offers a non-negligible performance gain (see #15514 (comment)).

Previously, `$animate` would decide whether an animation should be cancelled based on some assumption that didn't hold in specific cases (e.g. when animating transcluded clones with `templateUrl` directives on them for the first time). As a result, the entering elements would not be animated in such cases. This affected commonly used, structural built-in directives (`ngIf`, `ngRepeat`, `ngSwitch` etc). This commit fixes it by avoiding invalid assumptions (i.e. by taking into account the transformations that take place while compiling such elements). Partly addresses angular#14074 and angular#14124. Fixes angular#15510

gkalpak · 2017-01-09T10:14:07Z

@mgol, @Narretz: PTAL

Narretz · 2017-01-20T17:32:06Z

LGTM
@mgol can you check this again?

Previously, `$animate` would decide whether an animation should be cancelled based on some assumption that didn't hold in specific cases (e.g. when animating transcluded clones with `templateUrl` directives on them for the first time). As a result, the entering elements would not be animated in such cases. This affected commonly used, structural built-in directives (`ngIf`, `ngRepeat`, `ngSwitch` etc). This commit fixes it by avoiding invalid assumptions (i.e. by taking into account the transformations that take place while compiling such elements). Partly addresses #14074 and #14124. Fixes #15510 Closes #15514

Previously, `$animate` would decide whether an animation should be cancelled based on some assumption that didn't hold in specific cases (e.g. when animating transcluded clones with `templateUrl` directives on them for the first time). As a result, the entering elements would not be animated in such cases. This affected commonly used, structural built-in directives (`ngIf`, `ngRepeat`, `ngSwitch` etc). This commit fixes it by avoiding invalid assumptions (i.e. by taking into account the transformations that take place while compiling such elements). Partly addresses angular#14074 and angular#14124. Fixes angular#15510 Closes angular#15514

gkalpak added 7 commits December 15, 2016 15:04

refactor(ngAnimate): remove unused argument

3db2092

refactor(ngAnimate): simplify areAnimationsAllowed() and remove red…

d5ea497

…undant calls

refactor(ngAnimate): simplify cleanupEventListeners() and remove re…

8c71c02

…dundant calls

refactor(ngAnimate): simplify closeChildAnimations() and remove red…

a4d4d3c

…undant calls

refactor(ngAnimate): simplify clearElementAnimationState() and remo…

9f55361

…ve redundant calls

refactor(ngAnimate): simplify markElementAnimationState() and remov…

21ca2ee

…e redundant calls

refactor(ngAnimate): simplify findCallbacks() and remove redundant …

288351b

…calls

gkalpak added component: ngAnimate frequency: moderate needs: review severity: regression type: bug labels Dec 16, 2016

gkalpak added this to the 1.5.11 milestone Dec 16, 2016

googlebot added the cla: yes label Dec 16, 2016

gkalpak mentioned this pull request Dec 16, 2016

Enter/leave animation not working for for first item in ng-repeat #15510

Closed

This was referenced Dec 20, 2016

ngAnimate with ngRepeat in directive using templateUrl issue #14124

Closed

$animateCss used on a directive that uses a templateUrl breaks #14074

Open

gkalpak force-pushed the fix-ngAnimate-enter-transcluded-templateUrl branch from 5ccff06 to 06124e3 Compare December 20, 2016 09:35

mgol suggested changes Dec 21, 2016

View reviewed changes

gkalpak force-pushed the fix-ngAnimate-enter-transcluded-templateUrl branch from be4adcf to acd9e7b Compare December 21, 2016 12:14

mgol reviewed Dec 21, 2016

View reviewed changes

gkalpak force-pushed the fix-ngAnimate-enter-transcluded-templateUrl branch from acd9e7b to e1a8bd6 Compare December 31, 2016 10:58

chore(benchmarks): add basic animation benchmark

df88676

gkalpak force-pushed the fix-ngAnimate-enter-transcluded-templateUrl branch from e1a8bd6 to c215896 Compare December 31, 2016 11:00

Narretz reviewed Jan 4, 2017

View reviewed changes

gkalpak added 2 commits January 9, 2017 12:10

test(ngAnimate): make expectations more specific

d552f11

gkalpak force-pushed the fix-ngAnimate-enter-transcluded-templateUrl branch from c215896 to 5341108 Compare January 9, 2017 10:13

gkalpak modified the milestones: 1.6.2, 1.5.11 Jan 11, 2017

mgol approved these changes Jan 24, 2017

View reviewed changes

gkalpak closed this in d24617b Jan 24, 2017

gkalpak deleted the fix-ngAnimate-enter-transcluded-templateUrl branch January 24, 2017 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngAnimate): correctly animate transcluded clones with `templateUrl` #15514

fix(ngAnimate): correctly animate transcluded clones with `templateUrl` #15514

gkalpak commented Dec 16, 2016 •

edited

Loading

gkalpak commented Dec 17, 2016 •

edited

Loading

mgol Dec 19, 2016

gkalpak Dec 21, 2016

Narretz Jan 4, 2017

mgol Jan 4, 2017

mgol Dec 21, 2016

gkalpak Dec 21, 2016

mgol Dec 21, 2016

mgol Dec 21, 2016

gkalpak Dec 21, 2016

mgol Dec 21, 2016

gkalpak Dec 21, 2016

mgol Dec 21, 2016

gkalpak Dec 21, 2016

mgol Dec 21, 2016

gkalpak Dec 21, 2016

gkalpak commented Dec 21, 2016

mgol Dec 21, 2016

gkalpak commented Dec 31, 2016

Narretz Jan 4, 2017

gkalpak Jan 4, 2017 •

edited

Loading

gkalpak commented Jan 9, 2017

Narretz commented Jan 20, 2017

fix(ngAnimate): correctly animate transcluded clones with templateUrl #15514

fix(ngAnimate): correctly animate transcluded clones with templateUrl #15514

Conversation

gkalpak commented Dec 16, 2016 • edited Loading

gkalpak commented Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Dec 21, 2016

Choose a reason for hiding this comment

gkalpak commented Dec 31, 2016

Choose a reason for hiding this comment

gkalpak Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

gkalpak commented Jan 9, 2017

Narretz commented Jan 20, 2017

fix(ngAnimate): correctly animate transcluded clones with `templateUrl` #15514

fix(ngAnimate): correctly animate transcluded clones with `templateUrl` #15514

gkalpak commented Dec 16, 2016 •

edited

Loading

gkalpak commented Dec 17, 2016 •

edited

Loading

gkalpak Jan 4, 2017 •

edited

Loading