Skip to content

Commit 13f5d69

Browse files
matskomatsko
committed
fix(ngAnimate): ensure nested class-based animations spaced out with a RAF
Prior to this fix any nested class-based animations (animations that are triggered with addClass/removeClass or ngClass) would cancel each other out when nested in DOM structure. This fix ensures that the nested animations are spaced out with sequenced RAFs so that parent CSS classes are applied prior to any ancestor animations that are scheduled to run. Closes angular#11812
1 parent f26fc26 commit 13f5d69

File tree

7 files changed

+215
-31
lines changed

7 files changed

+215
-31
lines changed

‎angularFiles.js

+1
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ var angularFiles = {
8888
'angularModules': {
8989
'ngAnimate': [
9090
'src/ngAnimate/shared.js',
91+
'src/ngAnimate/rafScheduler.js',
9192
'src/ngAnimate/animateChildrenDirective.js',
9293
'src/ngAnimate/animateCss.js',
9394
'src/ngAnimate/animateCssDriver.js',

‎src/ngAnimate/animateCss.js

+3-8
Original file line numberDiff line numberDiff line change
@@ -392,9 +392,9 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
392392
var gcsStaggerLookup = createLocalCacheLookup();
393393

394394
this.$get = ['$window', '$$jqLite', '$$AnimateRunner', '$timeout',
395-
'$document', '$sniffer', '$$rAF',
395+
'$document', '$sniffer', '$$rAFScheduler',
396396
function($window, $$jqLite, $$AnimateRunner, $timeout,
397-
$document, $sniffer, $$rAF) {
397+
$document, $sniffer, $$rAFScheduler) {
398398

399399
var applyAnimationClasses = applyAnimationClassesFactory($$jqLite);
400400

@@ -452,15 +452,10 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
452452
}
453453

454454
var bod = getDomNode($document).body;
455-
var cancelLastRAFRequest;
456455
var rafWaitQueue = [];
457456
function waitUntilQuiet(callback) {
458-
if (cancelLastRAFRequest) {
459-
cancelLastRAFRequest(); //cancels the request
460-
}
461457
rafWaitQueue.push(callback);
462-
cancelLastRAFRequest = $$rAF(function() {
463-
cancelLastRAFRequest = null;
458+
$$rAFScheduler.waitUntilQuiet(function() {
464459
gcsLookup.flush();
465460
gcsStaggerLookup.flush();
466461

‎src/ngAnimate/animateQueue.js

+8-4
Original file line numberDiff line numberDiff line change
@@ -58,9 +58,9 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
5858
return currentAnimation.state === RUNNING_STATE && newAnimation.structural;
5959
});
6060

61-
this.$get = ['$$rAF', '$rootScope', '$rootElement', '$document', '$$HashMap',
61+
this.$get = ['$$rAF', '$rootScope', '$rootElement', '$document', '$$HashMap', '$$rAFScheduler',
6262
'$$animation', '$$AnimateRunner', '$templateRequest', '$$jqLite',
63-
function($$rAF, $rootScope, $rootElement, $document, $$HashMap,
63+
function($$rAF, $rootScope, $rootElement, $document, $$HashMap, $$rAFScheduler,
6464
$$animation, $$AnimateRunner, $templateRequest, $$jqLite) {
6565

6666
var activeAnimationsLookup = new $$HashMap();
@@ -351,7 +351,9 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
351351
return runner;
352352
}
353353

354-
closeParentClassBasedAnimations(parent);
354+
if (isStructural) {
355+
closeParentClassBasedAnimations(parent);
356+
}
355357

356358
// the counter keeps track of cancelled animations
357359
var counter = (existingAnimation.counter || 0) + 1;
@@ -403,7 +405,9 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
403405
? 'setClass'
404406
: animationDetails.event;
405407

406-
closeParentClassBasedAnimations(parentElement);
408+
if (animationDetails.structural) {
409+
closeParentClassBasedAnimations(parentElement);
410+
}
407411

408412
markElementAnimationState(element, RUNNING_STATE);
409413
var realRunner = $$animation(element, event, animationDetails.options);

‎src/ngAnimate/animation.js

+67-18
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,16 @@ var $$AnimationProvider = ['$animateProvider', function($animateProvider) {
1919
return element.data(RUNNER_STORAGE_KEY);
2020
}
2121

22-
this.$get = ['$$jqLite', '$rootScope', '$injector', '$$AnimateRunner',
23-
function($$jqLite, $rootScope, $injector, $$AnimateRunner) {
22+
this.$get = ['$$jqLite', '$rootScope', '$injector', '$$AnimateRunner', '$$rAFScheduler',
23+
function($$jqLite, $rootScope, $injector, $$AnimateRunner, $$rAFScheduler) {
2424

2525
var animationQueue = [];
2626
var applyAnimationClasses = applyAnimationClassesFactory($$jqLite);
2727

28+
var totalPendingClassBasedAnimations = 0;
29+
var totalActiveClassBasedAnimations = 0;
30+
var classBasedAnimationsQueue = [];
31+
2832
// TODO(matsko): document the signature in a better way
2933
return function(element, event, options) {
3034
options = prepareAnimationOptions(options);
@@ -53,12 +57,18 @@ var $$AnimationProvider = ['$animateProvider', function($animateProvider) {
5357
options.tempClasses = null;
5458
}
5559

60+
var classBasedIndex;
61+
if (!isStructural) {
62+
classBasedIndex = totalPendingClassBasedAnimations++;
63+
}
64+
5665
animationQueue.push({
5766
// this data is used by the postDigest code and passed into
5867
// the driver step function
5968
element: element,
6069
classes: classes,
6170
event: event,
71+
classBasedIndex: classBasedIndex,
6272
structural: isStructural,
6373
options: options,
6474
beforeStart: beforeStart,
@@ -73,6 +83,10 @@ var $$AnimationProvider = ['$animateProvider', function($animateProvider) {
7383
if (animationQueue.length > 1) return runner;
7484

7585
$rootScope.$$postDigest(function() {
86+
totalActiveClassBasedAnimations = totalPendingClassBasedAnimations;
87+
totalPendingClassBasedAnimations = 0;
88+
classBasedAnimationsQueue.length = 0;
89+
7690
var animations = [];
7791
forEach(animationQueue, function(entry) {
7892
// the element was destroyed early on which removed the runner
@@ -87,23 +101,58 @@ var $$AnimationProvider = ['$animateProvider', function($animateProvider) {
87101
animationQueue.length = 0;
88102

89103
forEach(groupAnimations(animations), function(animationEntry) {
90-
// it's important that we apply the `ng-animate` CSS class and the
91-
// temporary classes before we do any driver invoking since these
92-
// CSS classes may be required for proper CSS detection.
93-
animationEntry.beforeStart();
94-
95-
var operation = invokeFirstDriver(animationEntry);
96-
var triggerAnimationStart = operation && operation.start; /// TODO(matsko): only recognize operation.start()
97-
98-
var closeFn = animationEntry.close;
99-
if (!triggerAnimationStart) {
100-
closeFn();
104+
if (animationEntry.structural || animationEntry.anchors) {
105+
triggerAnimationStart();
101106
} else {
102-
var animationRunner = triggerAnimationStart();
103-
animationRunner.done(function(status) {
104-
closeFn(!status);
107+
classBasedAnimationsQueue.push({
108+
node: getDomNode(animationEntry.element),
109+
fn: triggerAnimationStart
105110
});
106-
updateAnimationRunners(animationEntry, animationRunner);
111+
112+
if (animationEntry.classBasedIndex === totalActiveClassBasedAnimations - 1) {
113+
// we need to sort each of the animations in order of parent to child
114+
// relationships. This ensures that the child classes are applied at the
115+
// right time.
116+
classBasedAnimationsQueue = classBasedAnimationsQueue.sort(function(a,b) {
117+
return b.node.contains(a.node);
118+
}).map(function(entry) {
119+
return entry.fn;
120+
});
121+
122+
$$rAFScheduler(classBasedAnimationsQueue);
123+
}
124+
}
125+
126+
function triggerAnimationStart() {
127+
// it's important that we apply the `ng-animate` CSS class and the
128+
// temporary classes before we do any driver invoking since these
129+
// CSS classes may be required for proper CSS detection.
130+
animationEntry.beforeStart();
131+
132+
var startAnimationFn, closeFn = animationEntry.close;
133+
134+
// in the event that the element was removed before the digest runs or
135+
// during the RAF sequencing then we should not trigger the animation.
136+
var targetElement = animationEntry.anchors
137+
? (animationEntry.from.element || animationEntry.to.element)
138+
: animationEntry.element;
139+
140+
if (getRunner(targetElement)) {
141+
var operation = invokeFirstDriver(animationEntry);
142+
if (operation) {
143+
startAnimationFn = operation.start;
144+
}
145+
}
146+
147+
if (!startAnimationFn) {
148+
closeFn();
149+
} else {
150+
var animationRunner = startAnimationFn();
151+
animationRunner.done(function(status) {
152+
closeFn(!status);
153+
});
154+
updateAnimationRunners(animationEntry, animationRunner);
155+
}
107156
}
108157
});
109158
});
@@ -175,7 +224,7 @@ var $$AnimationProvider = ['$animateProvider', function($animateProvider) {
175224
var lookupKey = from.animationID.toString();
176225
if (!anchorGroups[lookupKey]) {
177226
var group = anchorGroups[lookupKey] = {
178-
// TODO(matsko): double-check this code
227+
structural: true,
179228
beforeStart: function() {
180229
fromAnimation.beforeStart();
181230
toAnimation.beforeStart();

‎src/ngAnimate/module.js

+2
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
/* global angularAnimateModule: true,
44
55
$$rAFMutexFactory,
6+
$$rAFSchedulerFactory,
67
$$AnimateChildrenDirective,
78
$$AnimateRunnerFactory,
89
$$AnimateQueueProvider,
@@ -742,6 +743,7 @@ angular.module('ngAnimate', [])
742743
.directive('ngAnimateChildren', $$AnimateChildrenDirective)
743744

744745
.factory('$$rAFMutex', $$rAFMutexFactory)
746+
.factory('$$rAFScheduler', $$rAFSchedulerFactory)
745747

746748
.factory('$$AnimateRunner', $$AnimateRunnerFactory)
747749

‎src/ngAnimate/rafScheduler.js

+48
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
'use strict';
2+
3+
var $$rAFSchedulerFactory = ['$$rAF', function($$rAF) {
4+
var tickQueue = [];
5+
var cancelFn;
6+
7+
function scheduler(tasks) {
8+
tickQueue.push(tasks);
9+
nextTick();
10+
}
11+
12+
scheduler.waitUntilQuiet = function(fn) {
13+
if (cancelFn) cancelFn();
14+
15+
cancelFn = $$rAF(function() {
16+
cancelFn = null;
17+
fn();
18+
nextTick();
19+
});
20+
};
21+
22+
return scheduler;
23+
24+
function nextTick() {
25+
if (!tickQueue.length) return;
26+
27+
var updatedQueue = [];
28+
for (var i = 0; i < tickQueue.length; i++) {
29+
var innerQueue = tickQueue[i];
30+
runNextTask(innerQueue);
31+
if (innerQueue.length) {
32+
updatedQueue.push(innerQueue);
33+
}
34+
}
35+
tickQueue = updatedQueue;
36+
37+
if (!cancelFn) {
38+
$$rAF(function() {
39+
if (!cancelFn) nextTick();
40+
});
41+
}
42+
}
43+
44+
function runNextTask(tasks) {
45+
var nextTask = tasks.shift();
46+
nextTask();
47+
}
48+
}];

‎test/ngAnimate/animationSpec.js

+86-1
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,90 @@ describe('$$animation', function() {
288288
};
289289
}));
290290

291+
it('should space out multiple ancestorial class-based animations with a RAF in between',
292+
inject(function($rootScope, $$animation, $$rAF) {
293+
294+
var parent = element;
295+
element = jqLite('<div></div>');
296+
parent.append(element);
297+
298+
var child = jqLite('<div></div>');
299+
element.append(child);
300+
301+
$$animation(parent, 'addClass', { addClass: 'blue' });
302+
$$animation(element, 'addClass', { addClass: 'red' });
303+
$$animation(child, 'addClass', { addClass: 'green' });
304+
305+
$rootScope.$digest();
306+
307+
expect(captureLog.length).toBe(1);
308+
expect(capturedAnimation.options.addClass).toBe('blue');
309+
310+
$$rAF.flush();
311+
expect(captureLog.length).toBe(2);
312+
expect(capturedAnimation.options.addClass).toBe('red');
313+
314+
$$rAF.flush();
315+
expect(captureLog.length).toBe(3);
316+
expect(capturedAnimation.options.addClass).toBe('green');
317+
}));
318+
319+
it('should properly cancel out pending animations that are spaced with a RAF request before the digest completes',
320+
inject(function($rootScope, $$animation, $$rAF) {
321+
322+
var parent = element;
323+
element = jqLite('<div></div>');
324+
parent.append(element);
325+
326+
var child = jqLite('<div></div>');
327+
element.append(child);
328+
329+
var r1 = $$animation(parent, 'addClass', { addClass: 'blue' });
330+
var r2 = $$animation(element, 'addClass', { addClass: 'red' });
331+
var r3 = $$animation(child, 'addClass', { addClass: 'green' });
332+
333+
r2.end();
334+
335+
$rootScope.$digest();
336+
337+
expect(captureLog.length).toBe(1);
338+
expect(capturedAnimation.options.addClass).toBe('blue');
339+
340+
$$rAF.flush();
341+
342+
expect(captureLog.length).toBe(2);
343+
expect(capturedAnimation.options.addClass).toBe('green');
344+
}));
345+
346+
it('should properly cancel out pending animations that are spaced with a RAF request after the digest completes',
347+
inject(function($rootScope, $$animation, $$rAF) {
348+
349+
var parent = element;
350+
element = jqLite('<div></div>');
351+
parent.append(element);
352+
353+
var child = jqLite('<div></div>');
354+
element.append(child);
355+
356+
var r1 = $$animation(parent, 'addClass', { addClass: 'blue' });
357+
var r2 = $$animation(element, 'addClass', { addClass: 'red' });
358+
var r3 = $$animation(child, 'addClass', { addClass: 'green' });
359+
360+
$rootScope.$digest();
361+
362+
r2.end();
363+
364+
expect(captureLog.length).toBe(1);
365+
expect(capturedAnimation.options.addClass).toBe('blue');
366+
367+
$$rAF.flush();
368+
expect(captureLog.length).toBe(1);
369+
370+
$$rAF.flush();
371+
expect(captureLog.length).toBe(2);
372+
expect(capturedAnimation.options.addClass).toBe('green');
373+
}));
374+
291375
they('should return a runner that object that contains a $prop() function',
292376
['end', 'cancel', 'then'], function(method) {
293377
inject(function($$animation) {
@@ -513,7 +597,7 @@ describe('$$animation', function() {
513597
}));
514598

515599
it("should not group animations into an anchored animation if enter/leave events are NOT used",
516-
inject(function($$animation, $rootScope) {
600+
inject(function($$animation, $rootScope, $$rAF) {
517601

518602
fromElement.addClass('shared-class');
519603
fromElement.attr('ng-animate-ref', '1');
@@ -528,6 +612,7 @@ describe('$$animation', function() {
528612
});
529613

530614
$rootScope.$digest();
615+
$$rAF.flush();
531616
expect(captureLog.length).toBe(2);
532617
}));
533618

0 commit comments

Comments
 (0)