From 7b41285ddf17a3f1ad750fa165b403f55ace14fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 14 Nov 2013 15:36:07 -0500 Subject: [PATCH 1/2] fix($animate): ensure the DOM operation isn't run twice 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 #4949 --- src/ngAnimate/animate.js | 18 ++++++++++++++---- test/ngAnimate/animateSpec.js | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index 3b3c29b92990..335fac9b8960 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -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; } @@ -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; } @@ -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; } @@ -627,6 +627,7 @@ angular.module('ngAnimate', ['ng']) element.data(NG_ANIMATE_STATE, { running:true, + className:className, structural:!isClassBased, animations:animations, done:onBeforeAnimationsComplete @@ -637,7 +638,7 @@ angular.module('ngAnimate', ['ng']) invokeRegisteredAnimationFns(animations, 'before', onBeforeAnimationsComplete); function onBeforeAnimationsComplete(cancelled) { - domOperation(); + fireDOMOperation(); if(cancelled === true) { closeAnimation(); return; @@ -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; diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index 8ad7ed796686..46bb9538a5df 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -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('
')($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); + })); + }); From b0fdadd04c25c0aa6fae162d1d65a3d48b597839 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 14 Nov 2013 23:45:22 -0500 Subject: [PATCH 2/2] fix(ngClass): ensure that ngClass only adds/removes the changed classes ngClass works by removing all the former classes and then adding all the new classes to the element during each watch change operation. This may cause transition animations to never render. The ngClass directive will now only add and remove the classes that change during each watch operation. Closes #4960 --- src/.jshintrc | 3 ++- src/Angular.js | 24 +++++++++++++++++- src/ng/compile.js | 20 ++------------- src/ng/directive/ngClass.js | 24 +++++++++++++----- test/ng/directive/ngClassSpec.js | 43 ++++++++++++++++++++++++++++++++ 5 files changed, 88 insertions(+), 26 deletions(-) diff --git a/src/.jshintrc b/src/.jshintrc index e29b09f3a848..2467d667e896 100644 --- a/src/.jshintrc +++ b/src/.jshintrc @@ -100,6 +100,7 @@ "assertNotHasOwnProperty": false, "getter": false, "getBlockElements": false, + "tokenDifference": false, /* AngularPublic.js */ "version": false, @@ -162,4 +163,4 @@ "nullFormCtrl": false } -} \ No newline at end of file +} diff --git a/src/Angular.js b/src/Angular.js index 797e4b811ed5..9a8b35b9f082 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -80,7 +80,8 @@ -assertArgFn, -assertNotHasOwnProperty, -getter, - -getBlockElements + -getBlockElements, + -tokenDifference */ @@ -1338,3 +1339,24 @@ function getBlockElements(block) { return jqLite(elements); } + +/** + * Return the string difference between tokens of the original string compared to the old string + * @param {str1} string original string value + * @param {str2} string new string value + */ +function tokenDifference(str1, str2) { + var values = '', + tokens1 = str1.split(/\s+/), + tokens2 = str2.split(/\s+/); + + outer: + for(var i=0;i 0 ? ' ' : '') + token; + } + return values; +} diff --git a/src/ng/compile.js b/src/ng/compile.js index 0b81853542ee..c9df080ca506 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -677,8 +677,8 @@ function $CompileProvider($provide) { if(key == 'class') { value = value || ''; var current = this.$$element.attr('class') || ''; - this.$removeClass(tokenDifference(current, value).join(' ')); - this.$addClass(tokenDifference(value, current).join(' ')); + this.$removeClass(tokenDifference(current, value)); + this.$addClass(tokenDifference(value, current)); } else { var booleanKey = getBooleanAttrName(this.$$element[0], key), normalizedVal, @@ -736,22 +736,6 @@ function $CompileProvider($provide) { $exceptionHandler(e); } }); - - function tokenDifference(str1, str2) { - var values = [], - tokens1 = str1.split(/\s+/), - tokens2 = str2.split(/\s+/); - - outer: - for(var i=0;i 0) { + removeClass(toRemove); + } + + var toAdd = tokenDifference(newClasses, oldClasses); + if(toAdd.length > 0) { + addClass(toAdd); + } + } + else { + addClass(newClasses); } - addClass(newVal); } oldVal = copy(newVal); } function removeClass(classVal) { - attr.$removeClass(flattenClasses(classVal)); + attr.$removeClass(classVal); } function addClass(classVal) { - attr.$addClass(flattenClasses(classVal)); + attr.$addClass(classVal); } function flattenClasses(classVal) { diff --git a/test/ng/directive/ngClassSpec.js b/test/ng/directive/ngClassSpec.js index ab996e4d6f1e..62733c85beb7 100644 --- a/test/ng/directive/ngClassSpec.js +++ b/test/ng/directive/ngClassSpec.js @@ -411,4 +411,47 @@ describe('ngClass animations', function() { expect(enterComplete).toBe(true); }); }); + + it("should not remove classes if they're going to be added back right after", function() { + module('mock.animate'); + + inject(function($rootScope, $compile, $animate) { + var className; + + $rootScope.one = true; + $rootScope.two = true; + $rootScope.three = true; + + var element = angular.element('
'); + $compile(element)($rootScope); + $rootScope.$digest(); + + //this fires twice due to the class observer firing + className = $animate.flushNext('addClass').params[1]; + className = $animate.flushNext('addClass').params[1]; + expect(className).toBe('one two three'); + + expect($animate.queue.length).toBe(0); + + $rootScope.three = false; + $rootScope.$digest(); + + className = $animate.flushNext('removeClass').params[1]; + expect(className).toBe('three'); + + expect($animate.queue.length).toBe(0); + + $rootScope.two = false; + $rootScope.three = true; + $rootScope.$digest(); + + className = $animate.flushNext('removeClass').params[1]; + expect(className).toBe('two'); + + className = $animate.flushNext('addClass').params[1]; + expect(className).toBe('three'); + + expect($animate.queue.length).toBe(0); + }); + }); });