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

matsko · matsko · commit 0cd7e8f22721 · 2013-11-21T20:47:44.000-05:00
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.
diff --git 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
@@ -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
@@ -672,6 +672,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.
@@ -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
@@ -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
@@ -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
@@ -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
@@ -656,7 +656,6 @@ describe('ngView animations', function() {
 
         item = $animate.flushNext('enter').element;
 
-        $animate.flushNext('addClass').element;
         $animate.flushNext('addClass').element;
 
         expect(item.hasClass('classy')).toBe(true);
@@ -676,7 +675,6 @@ describe('ngView animations', function() {
         $animate.flushNext('enter').element;
         item = $animate.flushNext('leave').element;
 
-        $animate.flushNext('addClass').element;
         $animate.flushNext('addClass').element;
 
         expect(item.hasClass('boring')).toBe(true);
