X Tutup
Skip to content

Commit 20b8ece

Browse files
matskoNarretz
matsko
authored and
Narretz
committed
fix(ngAnimate): properly cancel-out previously running class-based animations
Prior to this fix the addition and removal of a CSS class via ngAnimate would cause flicker effects because $animate was unable to keep track of the CSS classes once they were applied to the element. This fix ensures that ngAnimate always keeps a reference to the classes in the currently running animation so that cancelling works accordingly. The commit also adds a test for a previously untested animation merge path. Closes angular#10156 Closes angular#13822
1 parent 6406e3b commit 20b8ece

File tree

7 files changed

+92
-34
lines changed

7 files changed

+92
-34
lines changed

‎src/ngAnimate/.jshintrc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@
4949
"assertArg": false,
5050
"isPromiseLike": false,
5151
"mergeClasses": false,
52-
"mergeAnimationOptions": false,
52+
"mergeAnimationDetails": false,
5353
"prepareAnimationOptions": false,
5454
"applyAnimationStyles": false,
5555
"applyAnimationFromStyles": false,

‎src/ngAnimate/animateQueue.js‎

Lines changed: 23 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -42,22 +42,21 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
4242
});
4343
}
4444

45-
function hasAnimationClasses(options, and) {
46-
options = options || {};
47-
var a = (options.addClass || '').length > 0;
48-
var b = (options.removeClass || '').length > 0;
45+
function hasAnimationClasses(animation, and) {
46+
var a = (animation.addClass || '').length > 0;
47+
var b = (animation.removeClass || '').length > 0;
4948
return and ? a && b : a || b;
5049
}
5150

5251
rules.join.push(function(element, newAnimation, currentAnimation) {
5352
// if the new animation is class-based then we can just tack that on
54-
return !newAnimation.structural && hasAnimationClasses(newAnimation.options);
53+
return !newAnimation.structural && hasAnimationClasses(newAnimation);
5554
});
5655

5756
rules.skip.push(function(element, newAnimation, currentAnimation) {
5857
// there is no need to animate anything if no classes are being added and
5958
// there is no structural animation that will be triggered
60-
return !newAnimation.structural && !hasAnimationClasses(newAnimation.options);
59+
return !newAnimation.structural && !hasAnimationClasses(newAnimation);
6160
});
6261

6362
rules.skip.push(function(element, newAnimation, currentAnimation) {
@@ -83,19 +82,17 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
8382
});
8483

8584
rules.cancel.push(function(element, newAnimation, currentAnimation) {
86-
87-
88-
var nA = newAnimation.options.addClass;
89-
var nR = newAnimation.options.removeClass;
90-
var cA = currentAnimation.options.addClass;
91-
var cR = currentAnimation.options.removeClass;
85+
var nA = newAnimation.addClass;
86+
var nR = newAnimation.removeClass;
87+
var cA = currentAnimation.addClass;
88+
var cR = currentAnimation.removeClass;
9289

9390
// early detection to save the global CPU shortage :)
9491
if ((isUndefined(nA) && isUndefined(nR)) || (isUndefined(cA) && isUndefined(cR))) {
9592
return false;
9693
}
9794

98-
return (hasMatchingClasses(nA, cR)) || (hasMatchingClasses(nR, cA));
95+
return hasMatchingClasses(nA, cR) || hasMatchingClasses(nR, cA);
9996
});
10097

10198
this.$get = ['$$rAF', '$rootScope', '$rootElement', '$document', '$$HashMap',
@@ -167,8 +164,8 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
167164

168165
var applyAnimationClasses = applyAnimationClassesFactory($$jqLite);
169166

170-
function normalizeAnimationOptions(element, options) {
171-
return mergeAnimationOptions(element, options, {});
167+
function normalizeAnimationDetails(element, animation) {
168+
return mergeAnimationDetails(element, animation, {});
172169
}
173170

174171
// IE9-11 has no method "contains" in SVG element and in Node.prototype. Bug #10259.
@@ -362,6 +359,8 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
362359
structural: isStructural,
363360
element: element,
364361
event: event,
362+
addClass: options.addClass,
363+
removeClass: options.removeClass,
365364
close: close,
366365
options: options,
367366
runner: runner
@@ -374,11 +373,10 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
374373
close();
375374
return runner;
376375
} else {
377-
mergeAnimationOptions(element, existingAnimation.options, options);
376+
mergeAnimationDetails(element, existingAnimation, newAnimation);
378377
return existingAnimation.runner;
379378
}
380379
}
381-
382380
var cancelAnimationFlag = isAllowed('cancel', element, newAnimation, existingAnimation);
383381
if (cancelAnimationFlag) {
384382
if (existingAnimation.state === RUNNING_STATE) {
@@ -393,7 +391,8 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
393391
existingAnimation.close();
394392
} else {
395393
// this will merge the new animation options into existing animation options
396-
mergeAnimationOptions(element, existingAnimation.options, newAnimation.options);
394+
mergeAnimationDetails(element, existingAnimation, newAnimation);
395+
397396
return existingAnimation.runner;
398397
}
399398
} else {
@@ -403,12 +402,12 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
403402
var joinAnimationFlag = isAllowed('join', element, newAnimation, existingAnimation);
404403
if (joinAnimationFlag) {
405404
if (existingAnimation.state === RUNNING_STATE) {
406-
normalizeAnimationOptions(element, options);
405+
normalizeAnimationDetails(element, newAnimation);
407406
} else {
408407
applyGeneratedPreparationClasses(element, isStructural ? event : null, options);
409408

410409
event = newAnimation.event = existingAnimation.event;
411-
options = mergeAnimationOptions(element, existingAnimation.options, newAnimation.options);
410+
options = mergeAnimationDetails(element, existingAnimation, newAnimation);
412411

413412
//we return the same runner since only the option values of this animation will
414413
//be fed into the `existingAnimation`.
@@ -419,7 +418,7 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
419418
} else {
420419
// normalization in this case means that it removes redundant CSS classes that
421420
// already exist (addClass) or do not exist (removeClass) on the element
422-
normalizeAnimationOptions(element, options);
421+
normalizeAnimationDetails(element, newAnimation);
423422
}
424423

425424
// when the options are merged and cleaned up we may end up not having to do
@@ -429,7 +428,7 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
429428
if (!isValidAnimation) {
430429
// animate (from/to) can be quickly checked first, otherwise we check if any classes are present
431430
isValidAnimation = (newAnimation.event === 'animate' && Object.keys(newAnimation.options.to || {}).length > 0)
432-
|| hasAnimationClasses(newAnimation.options);
431+
|| hasAnimationClasses(newAnimation);
433432
}
434433

435434
if (!isValidAnimation) {
@@ -459,7 +458,7 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
459458
var isValidAnimation = parentElement.length > 0
460459
&& (animationDetails.event === 'animate'
461460
|| animationDetails.structural
462-
|| hasAnimationClasses(animationDetails.options));
461+
|| hasAnimationClasses(animationDetails));
463462

464463
// this means that the previous animation was cancelled
465464
// even if the follow-up animation is the same event
@@ -491,7 +490,7 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
491490

492491
// this combined multiple class to addClass / removeClass into a setClass event
493492
// so long as a structural event did not take over the animation
494-
event = !animationDetails.structural && hasAnimationClasses(animationDetails.options, true)
493+
event = !animationDetails.structural && hasAnimationClasses(animationDetails, true)
495494
? 'setClass'
496495
: animationDetails.event;
497496

‎src/ngAnimate/shared.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,10 @@ function applyAnimationToStyles(element, options) {
218218
}
219219
}
220220

221-
function mergeAnimationOptions(element, target, newOptions) {
221+
function mergeAnimationDetails(element, oldAnimation, newAnimation) {
222+
var target = oldAnimation.options || {};
223+
var newOptions = newAnimation.options || {};
224+
222225
var toAdd = (target.addClass || '') + ' ' + (newOptions.addClass || '');
223226
var toRemove = (target.removeClass || '') + ' ' + (newOptions.removeClass || '');
224227
var classes = resolveElementClasses(element.attr('class'), toAdd, toRemove);
@@ -250,6 +253,9 @@ function mergeAnimationOptions(element, target, newOptions) {
250253
target.removeClass = null;
251254
}
252255

256+
oldAnimation.addClass = target.addClass;
257+
oldAnimation.removeClass = target.removeClass;
258+
253259
return target;
254260
}
255261

‎test/ngAnimate/.jshintrc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"browser": true,
44
"newcap": false,
55
"globals": {
6-
"mergeAnimationOptions": false,
6+
"mergeAnimationDetails": false,
77
"prepareAnimationOptions": false,
88
"applyAnimationStyles": false,
99
"applyAnimationFromStyles": false,

‎test/ngAnimate/animateSpec.js‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,18 @@ describe("animations", function() {
66
beforeEach(module('ngAnimateMock'));
77

88
var element, applyAnimationClasses;
9+
10+
beforeEach(module(function() {
11+
return function($$jqLite) {
12+
applyAnimationClasses = applyAnimationClassesFactory($$jqLite);
13+
};
14+
}));
15+
916
afterEach(inject(function($$jqLite) {
10-
applyAnimationClasses = applyAnimationClassesFactory($$jqLite);
1117
dealoc(element);
1218
}));
1319

20+
1421
it('should allow animations if the application is bootstrapped on the document node', function() {
1522
var capturedAnimation;
1623

@@ -1118,6 +1125,21 @@ describe("animations", function() {
11181125
expect(doneHandler).toHaveBeenCalled();
11191126
}));
11201127

1128+
it('should merge a follow-up animation that does not add classes into the previous animation (pre-digest)',
1129+
inject(function($animate, $rootScope) {
1130+
1131+
$animate.enter(element, parent);
1132+
$animate.animate(element, {height: 0}, {height: '100px'});
1133+
1134+
$rootScope.$digest();
1135+
expect(capturedAnimation).toBeTruthy();
1136+
expect(capturedAnimation[1]).toBe('enter'); // make sure the enter animation is present
1137+
1138+
// fake the style setting (because $$animation is mocked)
1139+
applyAnimationStyles(element, capturedAnimation[2]);
1140+
expect(element.css('height')).toContain('100px');
1141+
}));
1142+
11211143
it('should immediately skip the class-based animation if there is an active structural animation',
11221144
inject(function($animate, $rootScope) {
11231145

‎test/ngAnimate/animationHelperFunctionsSpec.js‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ describe("animation option helper functions", function() {
110110
}));
111111
});
112112

113-
describe('mergeAnimationOptions', function() {
113+
describe('mergeAnimationDetails', function() {
114114
it('should merge in new options', inject(function() {
115115
element.attr('class', 'blue');
116116
var options = prepareAnimationOptions({
@@ -120,11 +120,16 @@ describe("animation option helper functions", function() {
120120
removeClass: 'blue gold'
121121
});
122122

123-
mergeAnimationOptions(element, options, {
124-
age: 29,
125-
addClass: 'gold brown',
126-
removeClass: 'orange'
127-
});
123+
var animation1 = { options: options };
124+
var animation2 = {
125+
options: {
126+
age: 29,
127+
addClass: 'gold brown',
128+
removeClass: 'orange'
129+
}
130+
};
131+
132+
mergeAnimationDetails(element, animation1, animation2);
128133

129134
expect(options.name).toBe('matias');
130135
expect(options.age).toBe(29);

‎test/ngAnimate/integrationSpec.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,32 @@ describe('ngAnimate integration tests', function() {
2525
ss.destroy();
2626
});
2727

28+
it('should cancel a running and started removeClass animation when a follow-up addClass animation adds the same class',
29+
inject(function($animate, $rootScope, $$rAF, $document, $rootElement) {
30+
31+
jqLite($document[0].body).append($rootElement);
32+
element = jqLite('<div></div>');
33+
$rootElement.append(element);
34+
35+
element.addClass('active-class');
36+
37+
var runner = $animate.removeClass(element, 'active-class');
38+
$rootScope.$digest();
39+
40+
var doneHandler = jasmine.createSpy('addClass done');
41+
runner.done(doneHandler);
42+
43+
$$rAF.flush(); // Trigger the actual animation
44+
45+
expect(doneHandler).not.toHaveBeenCalled();
46+
47+
$animate.addClass(element, 'active-class');
48+
$rootScope.$digest();
49+
50+
// Cancelling the removeClass animation triggers the done callback
51+
expect(doneHandler).toHaveBeenCalled();
52+
}));
53+
2854
describe('CSS animations', function() {
2955
if (!browserSupportsCssAnimations()) return;
3056

0 commit comments

Comments
 (0)
X Tutup