Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngAnimate): run structural animations with cancelled out class ch… #14259

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/ngAnimate/animateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
});

rules.cancel.push(function(element, newAnimation, currentAnimation) {
// cancel the animation if classes added / removed in both animation cancel each other out,
// but only if the current animation isn't structural

if (currentAnimation.structural) return false;

var nA = newAnimation.addClass;
var nR = newAnimation.removeClass;
var cA = currentAnimation.addClass;
Expand Down
29 changes: 27 additions & 2 deletions test/ngAnimate/animateSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,8 @@ describe("animations", function() {
$animate.removeClass(element, 'active-class');
$rootScope.$digest();

expect(doneHandler).toHaveBeenCalled();
// true = rejected
expect(doneHandler).toHaveBeenCalledWith(true);
}));

it('should cancel the previously running removeClass animation if a follow-up addClass animation is using the same class value',
Expand All @@ -1123,7 +1124,8 @@ describe("animations", function() {
$animate.addClass(element, 'active-class');
$rootScope.$digest();

expect(doneHandler).toHaveBeenCalled();
// true = rejected
expect(doneHandler).toHaveBeenCalledWith(true);
}));

it('should merge a follow-up animation that does not add classes into the previous animation (pre-digest)',
Expand Down Expand Up @@ -1198,6 +1200,29 @@ describe("animations", function() {

expect(capturedAnimation[2].addClass).toBe('blue');
}));

it('should NOT cancel a previously joined addClass+structural animation if a follow-up ' +
'removeClass animation is using the same class value (pre-digest)',
inject(function($animate, $rootScope) {

var runner = $animate.enter(element, parent);
$animate.addClass(element, 'active-class');

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

expect(doneHandler).not.toHaveBeenCalled();

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

expect(capturedAnimation[1]).toBe('enter');
expect(capturedAnimation[2].addClass).toBe(null);
expect(capturedAnimation[2].removeClass).toBe(null);

expect(doneHandler).not.toHaveBeenCalled();
}));

});

describe('should merge', function() {
Expand Down
38 changes: 38 additions & 0 deletions test/ngAnimate/integrationSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -756,5 +756,43 @@ describe('ngAnimate integration tests', function() {
expect(child.attr('style')).toContain('50px');
});
});


it('should execute the enter animation on a <form> with ngIf that has an ' +
'<input type="email" required>', function() {

var animationSpy = jasmine.createSpy();

module(function($animateProvider) {
$animateProvider.register('.animate-me', function() {
return {
enter: function(element, done) {
animationSpy();
done();
}
};
});
});

inject(function($animate, $rootScope, $compile) {

element = jqLite(
'<div>' +
'<form class="animate-me" ng-if="show">' +
'<input ng-model="myModel" type="email" required />' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will be ineffective if that other PR lands that will change the $$parserName for input[email].
It would be better to have a test that explicitly reproduces the previously problematic situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test in animateSpec.js explicitly reproduces setting the classes. I always try to have one test that reproduces the actual directives etc. and one that uses the involved APIs directly. And I'm not sure about the test being ineffective, it's just very specific (and we don't know if the behavior will break at a later point in time again, maybe for other reasons)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough !
Since there is already a test that reproduces the issue using the low-level APIs, I'm fine with this 😃

'</form>' +
'</div>');

html(element);

$compile(element)($rootScope);

$rootScope.show = true;
$rootScope.$digest();

$animate.flush();
expect(animationSpy).toHaveBeenCalled();
});
});
});
});