From 22db8e20baab78063e4810c979056a4e3dcf79fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 2 Jan 2014 14:52:49 -0500 Subject: [PATCH 1/3] fix($animate): avoid accidentally matching substrings when resolving the presence of className tokens --- src/ngAnimate/animate.js | 9 +++++---- test/ngAnimate/animateSpec.js | 31 +++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index 64ed302981a9..62f6381dcda5 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -647,10 +647,11 @@ angular.module('ngAnimate', ['ng']) return; } + var ONE_SPACE = ' '; //this value will be searched for class-based CSS className lookup. Therefore, //we prefix and suffix the current className value with spaces to avoid substring //lookups of className tokens - var futureClassName = ' ' + currentClassName + ' '; + var futureClassName = ONE_SPACE + currentClassName + ONE_SPACE; if(ngAnimateState.running) { //if an animation is currently running on the element then lets take the steps //to cancel that animation and fire any required callbacks @@ -671,8 +672,8 @@ angular.module('ngAnimate', ['ng']) //will be invalid. Therefore the same string manipulation that would occur within the //DOM operation will be performed below so that the class comparison is valid... futureClassName = ngAnimateState.event == 'removeClass' ? - futureClassName.replace(ngAnimateState.className, '') : - futureClassName + ngAnimateState.className + ' '; + futureClassName.replace(ONE_SPACE + ngAnimateState.className + ONE_SPACE, ONE_SPACE) : + futureClassName + ngAnimateState.className + ONE_SPACE; } } @@ -680,7 +681,7 @@ angular.module('ngAnimate', ['ng']) //(on addClass) or doesn't contain (on removeClass) the className being animated. //The reason why this is being called after the previous animations are cancelled //is so that the CSS classes present on the element can be properly examined. - var classNameToken = ' ' + className + ' '; + var classNameToken = ONE_SPACE + className + ONE_SPACE; if((animationEvent == 'addClass' && futureClassName.indexOf(classNameToken) >= 0) || (animationEvent == 'removeClass' && futureClassName.indexOf(classNameToken) == -1)) { fireDOMOperation(); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index c60639df204e..b7e7c786144b 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -2667,6 +2667,37 @@ describe("ngAnimate", function() { expect(element.hasClass('red')).toBe(true); })); + it("should avoid mixing up substring classes during add and remove operations", function() { + var currentAnimation, currentFn; + module(function($animateProvider) { + $animateProvider.register('.on', function() { + return { + beforeAddClass : function(element, className, done) { + currentAnimation = 'addClass'; + currentFn = done; + }, + beforeRemoveClass : function(element, className, done) { + currentAnimation = 'removeClass'; + currentFn = done; + } + }; + }); + }); + inject(function($compile, $rootScope, $animate, $sniffer, $timeout) { + var element = $compile('
')($rootScope); + $rootElement.append(element); + jqLite($document[0].body).append($rootElement); + + $animate.addClass(element, 'on'); + expect(currentAnimation).toBe('addClass'); + currentFn(); + + $animate.removeClass(element, 'on'); + $animate.addClass(element, 'on'); + + expect(currentAnimation).toBe('addClass'); + }); + }); it('should enable and disable animations properly on the root element', function() { var count = 0; From 31c91fd01450a0feede3bc09f0ad5ee847fd9818 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 2 Jan 2014 17:27:13 -0500 Subject: [PATCH 2/3] fix($animate): correctly detect and handle CSS transition changes during class addition and removal When a CSS class containing transition code is added to an element then an animation should kick off. ngAnimate doesn't do this. It only respects transition styles that are already present on the element or on the setup class (but not the addClass animation). --- src/ngAnimate/animate.js | 41 ++++++++++++++++++--- test/ngAnimate/animateSpec.js | 69 ++++++++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 8 deletions(-) diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index 62f6381dcda5..26fe982d6b2a 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -1043,7 +1043,7 @@ angular.module('ngAnimate', ['ng']) return parentID + '-' + extractElementNode(element).className; } - function animateSetup(element, className) { + function animateSetup(element, className, calculationDecorator) { var cacheKey = getCacheKey(element); var eventCacheKey = cacheKey + ' ' + className; var stagger = {}; @@ -1061,9 +1061,16 @@ angular.module('ngAnimate', ['ng']) applyClasses && element.removeClass(staggerClassName); } + /* the animation itself may need to add/remove special CSS classes + * before calculating the anmation styles */ + calculationDecorator = calculationDecorator || + function(fn) { return fn(); }; + element.addClass(className); - var timings = getElementAnimationDetails(element, eventCacheKey); + var timings = calculationDecorator(function() { + return getElementAnimationDetails(element, eventCacheKey); + }); /* there is no point in performing a reflow if the animation timeout is empty (this would cause a flicker bug normally @@ -1228,8 +1235,8 @@ angular.module('ngAnimate', ['ng']) return style; } - function animateBefore(element, className) { - if(animateSetup(element, className)) { + function animateBefore(element, className, calculationDecorator) { + if(animateSetup(element, className, calculationDecorator)) { return function(cancelled) { cancelled && animateClose(element, className); }; @@ -1324,7 +1331,18 @@ angular.module('ngAnimate', ['ng']) }, beforeAddClass : function(element, className, animationCompleted) { - var cancellationMethod = animateBefore(element, suffixClasses(className, '-add')); + var cancellationMethod = animateBefore(element, suffixClasses(className, '-add'), function(fn) { + + /* when a CSS class is added to an element then the transition style that + * is applied is the transition defined on the element when the CSS class + * is added at the time of the animation. This is how CSS3 functions + * outside of ngAnimate. */ + element.addClass(className); + var timings = fn(); + element.removeClass(className); + return timings; + }); + if(cancellationMethod) { afterReflow(element, function() { unblockTransitions(element); @@ -1341,7 +1359,18 @@ angular.module('ngAnimate', ['ng']) }, beforeRemoveClass : function(element, className, animationCompleted) { - var cancellationMethod = animateBefore(element, suffixClasses(className, '-remove')); + var cancellationMethod = animateBefore(element, suffixClasses(className, '-remove'), function(fn) { + /* when classes are removed from an element then the transition style + * that is applied is the transition defined on the element without the + * CSS class being there. This is how CSS3 functions outside of ngAnimate. + * http://plnkr.co/edit/j8OzgTNxHTb4n3zLyjGW?p=preview */ + var klass = element.attr('class'); + element.removeClass(className); + var timings = fn(); + element.attr('class', klass); + return timings; + }); + if(cancellationMethod) { afterReflow(element, function() { unblockTransitions(element); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index b7e7c786144b..6fc9574488a4 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -2803,14 +2803,14 @@ describe("ngAnimate", function() { $animate.removeClass(element, 'base-class one two'); //still true since we're before the reflow - expect(element.hasClass('base-class')).toBe(true); + expect(element.hasClass('base-class')).toBe(false); //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); + expect(element.hasClass('base-class')).toBe(false); //the reflow... $timeout.flush(); @@ -3050,5 +3050,70 @@ describe("ngAnimate", function() { expect(leaveDone).toBe(true); }); }); + + it('should respect the most relevant CSS transition property if defined in multiple classes', + 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;'); + + ss.addRule('.base-class.on', '-webkit-transition:5s linear all;' + + 'transition:5s linear all;'); + + $animate.enabled(true); + + var element = $compile('
')($rootScope); + $rootElement.append(element); + jqLite($document[0].body).append($rootElement); + + var ready = false; + $animate.addClass(element, 'on', function() { + ready = true; + }); + + $timeout.flush(10); + browserTrigger(element, 'transitionend', { timeStamp: Date.now(), elapsedTime: 1 }); + $timeout.flush(1); + expect(ready).toBe(false); + + browserTrigger(element, 'transitionend', { timeStamp: Date.now(), elapsedTime: 5 }); + $timeout.flush(1); + expect(ready).toBe(true); + + ready = false; + $animate.removeClass(element, 'on', function() { + ready = true; + }); + + $timeout.flush(10); + browserTrigger(element, 'transitionend', { timeStamp: Date.now(), elapsedTime: 1 }); + $timeout.flush(1); + expect(ready).toBe(true); + })); + + it('should not apply a transition upon removal of a class that has a transition', + inject(function($sniffer, $compile, $rootScope, $rootElement, $animate, $timeout) { + + if (!$sniffer.transitions) return; + + ss.addRule('.base-class.on', '-webkit-transition:5s linear all;' + + 'transition:5s linear all;'); + + $animate.enabled(true); + + var element = $compile('
')($rootScope); + $rootElement.append(element); + jqLite($document[0].body).append($rootElement); + + var ready = false; + $animate.removeClass(element, 'on', function() { + ready = true; + }); + + $timeout.flush(1); + expect(ready).toBe(true); + })); }); }); From 164b36055ab43e07a3a1b97c04ace77f3d59b74c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Sun, 5 Jan 2014 23:47:18 -0500 Subject: [PATCH 3/3] fix($animate): prevent race conditions for class-based animations when animating on the same CSS class Closes #5588 --- src/ngAnimate/animate.js | 19 +++++++++--- test/ngAnimate/animateSpec.js | 56 ++++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index 26fe982d6b2a..f3a86f8246ed 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -659,12 +659,23 @@ angular.module('ngAnimate', ['ng']) cleanup(element); cancelAnimations(ngAnimateState.animations); + //in the event that the CSS is class is quickly added and removed back + //then we don't want to wait until after the reflow to add/remove the CSS + //class since both class animations may run into a race condition. + //The code below will check to see if that is occurring and will + //immediately remove the former class before the reflow so that the + //animation can snap back to the original animation smoothly + var isFullyClassBasedAnimation = isClassBased && !ngAnimateState.structural; + var isRevertingClassAnimation = isFullyClassBasedAnimation && + ngAnimateState.className == className && + animationEvent != ngAnimateState.event; + //if the class is removed during the reflow then it will revert the styles temporarily //back to the base class CSS styling causing a jump-like effect to occur. This check //here ensures that the domOperation is only performed after the reflow has commenced - if(ngAnimateState.beforeComplete) { + if(ngAnimateState.beforeComplete || isRevertingClassAnimation) { (ngAnimateState.done || noop)(true); - } else if(isClassBased && !ngAnimateState.structural) { + } else if(isFullyClassBasedAnimation) { //class-based animations will compare element className values after cancelling the //previous animation to see if the element properties already contain the final CSS //class and if so then the animation will be skipped. Since the domOperation will @@ -812,10 +823,10 @@ angular.module('ngAnimate', ['ng']) function cancelAnimations(animations) { var isCancelledFlag = true; forEach(animations, function(animation) { - if(!animations.beforeComplete) { + if(!animation.beforeComplete) { (animation.beforeEnd || noop)(isCancelledFlag); } - if(!animations.afterComplete) { + if(!animation.afterComplete) { (animation.afterEnd || noop)(isCancelledFlag); } }); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index 6fc9574488a4..3fa21a68597b 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -2675,10 +2675,16 @@ describe("ngAnimate", function() { beforeAddClass : function(element, className, done) { currentAnimation = 'addClass'; currentFn = done; + return function(cancelled) { + currentAnimation = cancelled ? null : currentAnimation; + } }, beforeRemoveClass : function(element, className, done) { currentAnimation = 'removeClass'; currentFn = done; + return function(cancelled) { + currentAnimation = cancelled ? null : currentAnimation; + } } }; }); @@ -2692,10 +2698,12 @@ describe("ngAnimate", function() { expect(currentAnimation).toBe('addClass'); currentFn(); + currentAnimation = null; + $animate.removeClass(element, 'on'); $animate.addClass(element, 'on'); - expect(currentAnimation).toBe('addClass'); + expect(currentAnimation).toBe(null); }); }); @@ -3115,5 +3123,51 @@ describe("ngAnimate", function() { $timeout.flush(1); expect(ready).toBe(true); })); + + it('should avoid skip animations if the same CSS class is added / removed synchronously before the reflow kicks in', + inject(function($sniffer, $compile, $rootScope, $rootElement, $animate, $timeout) { + + if (!$sniffer.transitions) return; + + ss.addRule('.water-class', '-webkit-transition:2s linear all;' + + 'transition:2s linear all;'); + + $animate.enabled(true); + + var element = $compile('
')($rootScope); + $rootElement.append(element); + jqLite($document[0].body).append($rootElement); + + var signature = ''; + $animate.removeClass(element, 'on', function() { + signature += 'A'; + }); + $animate.addClass(element, 'on', function() { + signature += 'B'; + }); + + $timeout.flush(1); + expect(signature).toBe('AB'); + + signature = ''; + $animate.removeClass(element, 'on', function() { + signature += 'A'; + }); + $animate.addClass(element, 'on', function() { + signature += 'B'; + }); + $animate.removeClass(element, 'on', function() { + signature += 'C'; + }); + + $timeout.flush(1); + expect(signature).toBe('AB'); + + $timeout.flush(10); + browserTrigger(element, 'transitionend', { timeStamp: Date.now(), elapsedTime: 2000 }); + $timeout.flush(1); + + expect(signature).toBe('ABC'); + })); }); });