fix($compile): ensure CSS classes are added and removed only when necessary

When $compile interpolates a CSS class attribute expression it will
do so by comparing the CSS class value already present on the element.
This may lead to unexpected results when dealing with ngClass values being
added and removed therefore it is best that both compile and ngClass delegate
addClass/removeClass operations to the same block of code.

7 files changed, 93 insertions, 102 deletions

diff --git a/src/.jshintrc b/src/.jshintrc
index 2467d667..f32caa45 100644
--- a/src/.jshintrc
+++ b/src/.jshintrc
@@ -100,7 +100,6 @@
     "assertNotHasOwnProperty": false,
     "getter": false,
     "getBlockElements": false,
-    "tokenDifference": false,
 
     /* AngularPublic.js */
     "version": false,
diff --git a/src/Angular.js b/src/Angular.js
index b27f4b06..11222118 100644
--- a/src/Angular.js
+++ b/src/Angular.js
@@ -81,7 +81,6 @@
     -assertNotHasOwnProperty,
     -getter,
     -getBlockElements,
-    -tokenDifference
 
 */
 
@@ -1351,24 +1350,3 @@ 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<tokens1.length;i++) {
-    var token = tokens1[i];
-    for(var j=0;j<tokens2.length;j++) {
-      if(token == tokens2[j]) continue outer;
-    }
-    values += (values.length > 0 ? ' ' : '') + token;
-  }
-  return values;
-}
diff --git a/src/ng/compile.js b/src/ng/compile.js
index ce3d0514..d977f173 100644
--- a/src/ng/compile.js
+++ b/src/ng/compile.js
@@ -673,6 +673,24 @@ function $CompileProvider($provide) {
       },
 
       /**
+       * @ngdoc function
+       * @name ng.$compile.directive.Attributes#$updateClass
+       * @methodOf ng.$compile.directive.Attributes
+       * @function
+       *
+       * @description
+       * Adds and removes the appropriate CSS class values to the element based on the difference
+       * between the new and old CSS class values (specified as newClasses and oldClasses).
+       *
+       * @param {string} newClasses The current CSS className value
+       * @param {string} oldClasses The former CSS className value
+       */
+      $updateClass : function(newClasses, oldClasses) {
+        this.$removeClass(tokenDifference(oldClasses, newClasses));
+        this.$addClass(tokenDifference(newClasses, oldClasses));
+      },
+
+      /**
        * Set a normalized attribute on the element in a way such that all directives
        * can share the attribute. This function properly handles boolean attributes.
        * @param {string} key Normalized key. (ie ngAttribute)
@@ -682,59 +700,53 @@ function $CompileProvider($provide) {
        * @param {string=} attrName Optional none normalized name. Defaults to key.
        */
       $set: function(key, value, writeAttr, attrName) {
-        //special case for class attribute addition + removal
-        //so that class changes can tap into the animation
-        //hooks provided by the $animate service
-        if(key == 'class') {
-          value = value || '';
-          var current = this.$$element.attr('class') || '';
-          this.$removeClass(tokenDifference(current, value));
-          this.$addClass(tokenDifference(value, current));
-        } else {
-          var booleanKey = getBooleanAttrName(this.$$element[0], key),
-              normalizedVal,
-              nodeName;
+        // TODO: decide whether or not to throw an error if "class"
+        //is set through this function since it may cause $updateClass to
+        //become unstable.
 
-          if (booleanKey) {
-            this.$$element.prop(key, value);
-            attrName = booleanKey;
-          }
+        var booleanKey = getBooleanAttrName(this.$$element[0], key),
+            normalizedVal,
+            nodeName;
 
-          this[key] = value;
+        if (booleanKey) {
+          this.$$element.prop(key, value);
+          attrName = booleanKey;
+        }
 
-          // translate normalized key to actual key
-          if (attrName) {
-            this.$attr[key] = attrName;
-          } else {
-            attrName = this.$attr[key];
-            if (!attrName) {
-              this.$attr[key] = attrName = snake_case(key, '-');
-            }
+        this[key] = value;
+
+        // translate normalized key to actual key
+        if (attrName) {
+          this.$attr[key] = attrName;
+        } else {
+          attrName = this.$attr[key];
+          if (!attrName) {
+            this.$attr[key] = attrName = snake_case(key, '-');
           }
+        }
 
-          nodeName = nodeName_(this.$$element);
-
-          // sanitize a[href] and img[src] values
-          if ((nodeName === 'A' && key === 'href') ||
-              (nodeName === 'IMG' && key === 'src')) {
-            // NOTE: urlResolve() doesn't support IE < 8 so we don't sanitize for that case.
-            if (!msie || msie >= 8 ) {
-              normalizedVal = urlResolve(value).href;
-              if (normalizedVal !== '') {
-                if ((key === 'href' && !normalizedVal.match(aHrefSanitizationWhitelist)) ||
-                    (key === 'src' && !normalizedVal.match(imgSrcSanitizationWhitelist))) {
-                  this[key] = value = 'unsafe:' + normalizedVal;
-                }
+        nodeName = nodeName_(this.$$element);
+
+        // sanitize a[href] and img[src] values
+        if ((nodeName === 'A' && key === 'href') ||
+            (nodeName === 'IMG' && key === 'src')) {
+          // NOTE: urlResolve() doesn't support IE < 8 so we don't sanitize for that case.
+          if (!msie || msie >= 8 ) {
+            normalizedVal = urlResolve(value).href;
+            if (normalizedVal !== '') {
+              if ((key === 'href' && !normalizedVal.match(aHrefSanitizationWhitelist)) ||
+                  (key === 'src' && !normalizedVal.match(imgSrcSanitizationWhitelist))) {
+                this[key] = value = 'unsafe:' + normalizedVal;
               }
             }
           }
+        }
 
-          if (writeAttr !== false) {
-            if (value === null || value === undefined) {
-              this.$$element.removeAttr(attrName);
-            } else {
-              this.$$element.attr(attrName, value);
-            }
+        if (writeAttr !== false) {
+          if (value === null || value === undefined) {
+            this.$$element.removeAttr(attrName);
+          } else {
+            this.$$element.attr(attrName, value);
           }
         }
 
@@ -1816,9 +1828,19 @@ function $CompileProvider($provide) {
                 attr[name] = interpolateFn(scope);
                 ($$observers[name] || ($$observers[name] = [])).$$inter = true;
                 (attr.$$observers && attr.$$observers[name].$$scope || scope).
-                    $watch(interpolateFn, function interpolateFnWatchAction(value) {
-                      attr.$set(name, value);
-                    });
+                  $watch(interpolateFn, function interpolateFnWatchAction(newValue, oldValue) {
+                    //special case for class attribute addition + removal
+                    //so that class changes can tap into the animation
+                    //hooks provided by the $animate service. Be sure to
+                    //skip animations when the first digest occurs (when
+                    //both the new and the old values are the same) since
+                    //the CSS classes are the non-interpolated values
+                    if(name === 'class' && newValue != oldValue) {
+                      attr.$updateClass(newValue, oldValue);
+                    } else {
+                      attr.$set(name, newValue);
+                    }
+                  });
               }
             };
           }
@@ -1958,3 +1980,19 @@ function directiveLinkingFn(
   /* Element */ rootElement,
   /* function(Function) */ boundTranscludeFn
 ){}
+
+function tokenDifference(str1, str2) {
+  var values = '',
+      tokens1 = str1.split(/\s+/),
+      tokens2 = str2.split(/\s+/);
+
+  outer:
+  for(var i = 0; i < tokens1.length; i++) {
+    var token = tokens1[i];
+    for(var j = 0; j < tokens2.length; j++) {
+      if(token == tokens2[j]) continue outer;
+    }
+    values += (values.length > 0 ? ' ' : '') + token;
+  }
+  return values;
+}
diff --git a/src/ng/directive/ngClass.js b/src/ng/directive/ngClass.js
index 10ef7fd1..21316c57 100644
--- a/src/ng/directive/ngClass.js
+++ b/src/ng/directive/ngClass.js
@@ -20,11 +20,10 @@ function classDirective(name, selector) {
             // jshint bitwise: false
             var mod = $index & 1;
             if (mod !== old$index & 1) {
-              if (mod === selector) {
-                addClass(flattenClasses(scope.$eval(attr[name])));
-              } else {
-                removeClass(flattenClasses(scope.$eval(attr[name])));
-              }
+              var classes = flattenClasses(scope.$eval(attr[name]));
+              mod === selector ?
+                attr.$addClass(classes) :
+                attr.$removeClass(classes);
             }
           });
         }
@@ -33,34 +32,16 @@ function classDirective(name, selector) {
         function ngClassWatchAction(newVal) {
           if (selector === true || scope.$index % 2 === selector) {
             var newClasses = flattenClasses(newVal || '');
-            if (oldVal && !equals(newVal,oldVal)) {
-              var oldClasses = flattenClasses(oldVal);
-              var toRemove = tokenDifference(oldClasses, newClasses);
-              if(toRemove.length > 0) {
-                removeClass(toRemove);
-              }
-
-              var toAdd = tokenDifference(newClasses, oldClasses);
-              if(toAdd.length > 0) {
-                addClass(toAdd);
-              }
-            } else {
-              addClass(newClasses);
+            if(!oldVal) {
+              attr.$addClass(newClasses);
+            } else if(!equals(newVal,oldVal)) {
+              attr.$updateClass(newClasses, flattenClasses(oldVal));
             }
           }
           oldVal = copy(newVal);
         }
 
 
-        function removeClass(classVal) {
-          attr.$removeClass(classVal);
-        }
-
-
-        function addClass(classVal) {
-          attr.$addClass(classVal);
-        }
-
         function flattenClasses(classVal) {
           if(isArray(classVal)) {
             return classVal.join(' ');
diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js
index 85ee17d2..c25c5040 100755
--- a/test/ng/compileSpec.js
+++ b/test/ng/compileSpec.js
@@ -4479,7 +4479,6 @@ describe('$compile', function() {
         $compile(element)($rootScope);
 
         $rootScope.$digest();
-        data = $animate.flushNext('removeClass');
 
         expect(element.hasClass('fire')).toBe(true);
 
diff --git a/test/ng/directive/ngClassSpec.js b/test/ng/directive/ngClassSpec.js
index 62733c85..c7307069 100644
--- a/test/ng/directive/ngClassSpec.js
+++ b/test/ng/directive/ngClassSpec.js
@@ -321,7 +321,6 @@ describe('ngClass animations', function() {
       $rootScope.val = 'one';
       $rootScope.$digest();
       $animate.flushNext('addClass');
-      $animate.flushNext('addClass');
       expect($animate.queue.length).toBe(0);
 
       $rootScope.val = '';
@@ -428,7 +427,6 @@ describe('ngClass animations', function() {
 
       //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);
diff --git a/test/ngRoute/directive/ngViewSpec.js b/test/ngRoute/directive/ngViewSpec.js
index e96da022..533f0b53 100644
--- a/test/ngRoute/directive/ngViewSpec.js
+++ b/test/ngRoute/directive/ngViewSpec.js
@@ -657,7 +657,6 @@ describe('ngView animations', function() {
         item = $animate.flushNext('enter').element;
 
         $animate.flushNext('addClass').element;
-        $animate.flushNext('addClass').element;
 
         expect(item.hasClass('classy')).toBe(true);
 
@@ -677,7 +676,6 @@ describe('ngView animations', function() {
         item = $animate.flushNext('leave').element;
 
         $animate.flushNext('addClass').element;
-        $animate.flushNext('addClass').element;
 
         expect(item.hasClass('boring')).toBe(true);
       }));