Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
fix($compile): ensure CSS classes are added and removed only when nec…
Browse files Browse the repository at this point in the history 
…essary

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.
  • Loading branch information
@matsko
matsko committed Nov 22, 2013
1 parent ba1b47f commit 0cd7e8f
Show file tree
Hide file tree
Showing 7 changed files with 93 additions and 102 deletions.
1 change: 0 additions & 1 deletion src/.jshintrc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@
"assertNotHasOwnProperty": false,
"getter": false,
"getBlockElements": false,
"tokenDifference": false,

/* AngularPublic.js */
"version": false,
Expand Down
22 changes: 0 additions & 22 deletions src/Angular.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@
-assertNotHasOwnProperty,
-getter,
-getBlockElements,
-tokenDifference
*/

Expand Down Expand Up @@ -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;
}
132 changes: 85 additions & 47 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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);
}
});
}
};
}
Expand Down Expand Up @@ -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;
}
35 changes: 8 additions & 27 deletions src/ng/directive/ngClass.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
});
}
Expand All @@ -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(' ');
Expand Down
1 change: 0 additions & 1 deletion test/ng/compileSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4479,7 +4479,6 @@ describe('$compile', function() {
$compile(element)($rootScope);

$rootScope.$digest();
data = $animate.flushNext('removeClass');

expect(element.hasClass('fire')).toBe(true);

Expand Down
2 changes: 0 additions & 2 deletions test/ng/directive/ngClassSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '';
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 0 additions & 2 deletions test/ngRoute/directive/ngViewSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down

0 comments on commit 0cd7e8f

Please sign in to comment.