Skip to content
Permalink
Browse files

fix(ngAnimate): properly cancel-out previously running class-based an…

…imations

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 #10156
Closes #13822
  • Loading branch information...
matsko authored and Narretz committed Jan 21, 2016
1 parent 6406e3b commit 20b8ece444408a64ac69f7b5d45ddb3af0c418a0
@@ -49,7 +49,7 @@
"assertArg": false,
"isPromiseLike": false,
"mergeClasses": false,
"mergeAnimationOptions": false,
"mergeAnimationDetails": false,
"prepareAnimationOptions": false,
"applyAnimationStyles": false,
"applyAnimationFromStyles": false,
@@ -42,22 +42,21 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
});
}

function hasAnimationClasses(options, and) {
options = options || {};
var a = (options.addClass || '').length > 0;
var b = (options.removeClass || '').length > 0;
function hasAnimationClasses(animation, and) {
var a = (animation.addClass || '').length > 0;
var b = (animation.removeClass || '').length > 0;
return and ? a && b : a || b;
}

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

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

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

rules.cancel.push(function(element, newAnimation, currentAnimation) {


var nA = newAnimation.options.addClass;
var nR = newAnimation.options.removeClass;
var cA = currentAnimation.options.addClass;
var cR = currentAnimation.options.removeClass;
var nA = newAnimation.addClass;
var nR = newAnimation.removeClass;
var cA = currentAnimation.addClass;
var cR = currentAnimation.removeClass;

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

return (hasMatchingClasses(nA, cR)) || (hasMatchingClasses(nR, cA));
return hasMatchingClasses(nA, cR) || hasMatchingClasses(nR, cA);
});

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

var applyAnimationClasses = applyAnimationClassesFactory($$jqLite);

function normalizeAnimationOptions(element, options) {
return mergeAnimationOptions(element, options, {});
function normalizeAnimationDetails(element, animation) {
return mergeAnimationDetails(element, animation, {});
}

// IE9-11 has no method "contains" in SVG element and in Node.prototype. Bug #10259.
@@ -362,6 +359,8 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
structural: isStructural,
element: element,
event: event,
addClass: options.addClass,
removeClass: options.removeClass,
close: close,
options: options,
runner: runner
@@ -374,11 +373,10 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
close();
return runner;
} else {
mergeAnimationOptions(element, existingAnimation.options, options);
mergeAnimationDetails(element, existingAnimation, newAnimation);
return existingAnimation.runner;
}
}

var cancelAnimationFlag = isAllowed('cancel', element, newAnimation, existingAnimation);
if (cancelAnimationFlag) {
if (existingAnimation.state === RUNNING_STATE) {
@@ -393,7 +391,8 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
existingAnimation.close();
} else {
// this will merge the new animation options into existing animation options
mergeAnimationOptions(element, existingAnimation.options, newAnimation.options);
mergeAnimationDetails(element, existingAnimation, newAnimation);

return existingAnimation.runner;
}
} else {
@@ -403,12 +402,12 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
var joinAnimationFlag = isAllowed('join', element, newAnimation, existingAnimation);
if (joinAnimationFlag) {
if (existingAnimation.state === RUNNING_STATE) {
normalizeAnimationOptions(element, options);
normalizeAnimationDetails(element, newAnimation);
} else {
applyGeneratedPreparationClasses(element, isStructural ? event : null, options);

event = newAnimation.event = existingAnimation.event;
options = mergeAnimationOptions(element, existingAnimation.options, newAnimation.options);
options = mergeAnimationDetails(element, existingAnimation, newAnimation);

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

// 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) {
if (!isValidAnimation) {
// animate (from/to) can be quickly checked first, otherwise we check if any classes are present
isValidAnimation = (newAnimation.event === 'animate' && Object.keys(newAnimation.options.to || {}).length > 0)
|| hasAnimationClasses(newAnimation.options);
|| hasAnimationClasses(newAnimation);
}

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

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

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

@@ -218,7 +218,10 @@ function applyAnimationToStyles(element, options) {
}
}

function mergeAnimationOptions(element, target, newOptions) {
function mergeAnimationDetails(element, oldAnimation, newAnimation) {
var target = oldAnimation.options || {};
var newOptions = newAnimation.options || {};

var toAdd = (target.addClass || '') + ' ' + (newOptions.addClass || '');
var toRemove = (target.removeClass || '') + ' ' + (newOptions.removeClass || '');
var classes = resolveElementClasses(element.attr('class'), toAdd, toRemove);
@@ -250,6 +253,9 @@ function mergeAnimationOptions(element, target, newOptions) {
target.removeClass = null;
}

oldAnimation.addClass = target.addClass;
oldAnimation.removeClass = target.removeClass;

return target;
}

@@ -3,7 +3,7 @@
"browser": true,
"newcap": false,
"globals": {
"mergeAnimationOptions": false,
"mergeAnimationDetails": false,
"prepareAnimationOptions": false,
"applyAnimationStyles": false,
"applyAnimationFromStyles": false,
@@ -6,11 +6,18 @@ describe("animations", function() {
beforeEach(module('ngAnimateMock'));

var element, applyAnimationClasses;

beforeEach(module(function() {
return function($$jqLite) {
applyAnimationClasses = applyAnimationClassesFactory($$jqLite);
};
}));

afterEach(inject(function($$jqLite) {
applyAnimationClasses = applyAnimationClassesFactory($$jqLite);
dealoc(element);
}));


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

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

it('should merge a follow-up animation that does not add classes into the previous animation (pre-digest)',
inject(function($animate, $rootScope) {

$animate.enter(element, parent);
$animate.animate(element, {height: 0}, {height: '100px'});

$rootScope.$digest();
expect(capturedAnimation).toBeTruthy();
expect(capturedAnimation[1]).toBe('enter'); // make sure the enter animation is present

// fake the style setting (because $$animation is mocked)
applyAnimationStyles(element, capturedAnimation[2]);
expect(element.css('height')).toContain('100px');
}));

it('should immediately skip the class-based animation if there is an active structural animation',
inject(function($animate, $rootScope) {

@@ -110,7 +110,7 @@ describe("animation option helper functions", function() {
}));
});

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

mergeAnimationOptions(element, options, {
age: 29,
addClass: 'gold brown',
removeClass: 'orange'
});
var animation1 = { options: options };
var animation2 = {
options: {
age: 29,
addClass: 'gold brown',
removeClass: 'orange'
}
};

mergeAnimationDetails(element, animation1, animation2);

expect(options.name).toBe('matias');
expect(options.age).toBe(29);
@@ -25,6 +25,32 @@ describe('ngAnimate integration tests', function() {
ss.destroy();
});

it('should cancel a running and started removeClass animation when a follow-up addClass animation adds the same class',
inject(function($animate, $rootScope, $$rAF, $document, $rootElement) {

jqLite($document[0].body).append($rootElement);
element = jqLite('<div></div>');
$rootElement.append(element);

element.addClass('active-class');

var runner = $animate.removeClass(element, 'active-class');
$rootScope.$digest();

var doneHandler = jasmine.createSpy('addClass done');
runner.done(doneHandler);

$$rAF.flush(); // Trigger the actual animation

expect(doneHandler).not.toHaveBeenCalled();

$animate.addClass(element, 'active-class');
$rootScope.$digest();

// Cancelling the removeClass animation triggers the done callback
expect(doneHandler).toHaveBeenCalled();
}));

describe('CSS animations', function() {
if (!browserSupportsCssAnimations()) return;

0 comments on commit 20b8ece

Please sign in to comment.
You can’t perform that action at this time.