Skip to content

Commit

Permalink
fix($animate): ensure the DOM operation isn't run twice
Browse files Browse the repository at this point in the history
Depending on the animations placed on ngClass, the DOM operation may
run twice causing a race condition between addClass and removeClass.
Depending on what classes are removed and added via $compile this may
cause all CSS classes to be removed accidentally from the element
being animated.

Closes angular#4949
  • Loading branch information
matsko committed Nov 15, 2013
1 parent 8425e9f commit 7b41285
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 4 deletions.
18 changes: 14 additions & 4 deletions src/ngAnimate/animate.js
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ angular.module('ngAnimate', ['ng'])
//the animation if any matching animations are not found at all.
//NOTE: IE8 + IE9 should close properly (run closeAnimation()) in case a NO animation is not found.
if (animationsDisabled(element, parentElement) || matches.length === 0) {
domOperation();
fireDOMOperation();
closeAnimation();
return;
}
Expand Down Expand Up @@ -596,7 +596,7 @@ angular.module('ngAnimate', ['ng'])
//this would mean that an animation was not allowed so let the existing
//animation do it's thing and close this one early
if(animations.length === 0) {
domOperation();
fireDOMOperation();
fireDoneCallbackAsync();
return;
}
Expand All @@ -616,7 +616,7 @@ angular.module('ngAnimate', ['ng'])
//is so that the CSS classes present on the element can be properly examined.
if((animationEvent == 'addClass' && element.hasClass(className)) ||
(animationEvent == 'removeClass' && !element.hasClass(className))) {
domOperation();
fireDOMOperation();
fireDoneCallbackAsync();
return;
}
Expand All @@ -627,6 +627,7 @@ angular.module('ngAnimate', ['ng'])

element.data(NG_ANIMATE_STATE, {
running:true,
className:className,
structural:!isClassBased,
animations:animations,
done:onBeforeAnimationsComplete
Expand All @@ -637,7 +638,7 @@ angular.module('ngAnimate', ['ng'])
invokeRegisteredAnimationFns(animations, 'before', onBeforeAnimationsComplete);

function onBeforeAnimationsComplete(cancelled) {
domOperation();
fireDOMOperation();
if(cancelled === true) {
closeAnimation();
return;
Expand Down Expand Up @@ -695,6 +696,15 @@ angular.module('ngAnimate', ['ng'])
doneCallback && $timeout(doneCallback, 0, false);
}

//it is less complicated to use a flag than managing and cancelling
//timeouts containing multiple callbacks.
function fireDOMOperation() {
if(!fireDOMOperation.hasBeenRun) {
fireDOMOperation.hasBeenRun = true;
domOperation();
}
}

function closeAnimation() {
if(!closeAnimation.hasBeenRun) {
closeAnimation.hasBeenRun = true;
Expand Down
34 changes: 34 additions & 0 deletions test/ngAnimate/animateSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2599,4 +2599,38 @@ describe("ngAnimate", function() {
});
});

it('should only perform the DOM operation once',
inject(function($sniffer, $compile, $rootScope, $rootElement, $animate, $timeout) {

if (!$sniffer.transitions) return;

ss.addRule('.base-class', '-webkit-transition:1s linear all;' +
'transition:1s linear all;');

$animate.enabled(true);

var element = $compile('<div class="base-class one two"></div>')($rootScope);
$rootElement.append(element);
jqLite($document[0].body).append($rootElement);

$animate.removeClass(element, 'base-class one two');

//still true since we're before the reflow
expect(element.hasClass('base-class')).toBe(true);

//this will cancel the remove animation
$animate.addClass(element, 'base-class one two');

//the cancellation was a success and the class was added right away
//since there was no successive animation for the after animation
expect(element.hasClass('base-class')).toBe(true);

//the reflow...
$timeout.flush();

//the reflow DOM operation was commenced but it ran before so it
//shouldn't run agaun
expect(element.hasClass('base-class')).toBe(true);
}));

});

0 comments on commit 7b41285

Please sign in to comment.