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

Commit 0cd7e8f

Browse files
matskomatsko
committed
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.
1 parent ba1b47f commit 0cd7e8f

File tree

7 files changed

+93
-102
lines changed

7 files changed

+93
-102
lines changed

‎src/.jshintrc

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,6 @@
100100
"assertNotHasOwnProperty": false,
101101
"getter": false,
102102
"getBlockElements": false,
103-
"tokenDifference": false,
104103

105104
/* AngularPublic.js */
106105
"version": false,

‎src/Angular.js

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,6 @@
8181
-assertNotHasOwnProperty,
8282
-getter,
8383
-getBlockElements,
84-
-tokenDifference
8584
8685
*/
8786

@@ -1351,24 +1350,3 @@ function getBlockElements(block) {
13511350

13521351
return jqLite(elements);
13531352
}
1354-
1355-
/**
1356-
* Return the string difference between tokens of the original string compared to the old string
1357-
* @param {str1} string original string value
1358-
* @param {str2} string new string value
1359-
*/
1360-
function tokenDifference(str1, str2) {
1361-
var values = '',
1362-
tokens1 = str1.split(/\s+/),
1363-
tokens2 = str2.split(/\s+/);
1364-
1365-
outer:
1366-
for(var i=0;i<tokens1.length;i++) {
1367-
var token = tokens1[i];
1368-
for(var j=0;j<tokens2.length;j++) {
1369-
if(token == tokens2[j]) continue outer;
1370-
}
1371-
values += (values.length > 0 ? ' ' : '') + token;
1372-
}
1373-
return values;
1374-
}

‎src/ng/compile.js

Lines changed: 85 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -672,6 +672,24 @@ function $CompileProvider($provide) {
672672
}
673673
},
674674

675+
/**
676+
* @ngdoc function
677+
* @name ng.$compile.directive.Attributes#$updateClass
678+
* @methodOf ng.$compile.directive.Attributes
679+
* @function
680+
*
681+
* @description
682+
* Adds and removes the appropriate CSS class values to the element based on the difference
683+
* between the new and old CSS class values (specified as newClasses and oldClasses).
684+
*
685+
* @param {string} newClasses The current CSS className value
686+
* @param {string} oldClasses The former CSS className value
687+
*/
688+
$updateClass : function(newClasses, oldClasses) {
689+
this.$removeClass(tokenDifference(oldClasses, newClasses));
690+
this.$addClass(tokenDifference(newClasses, oldClasses));
691+
},
692+
675693
/**
676694
* Set a normalized attribute on the element in a way such that all directives
677695
* can share the attribute. This function properly handles boolean attributes.
@@ -682,59 +700,53 @@ function $CompileProvider($provide) {
682700
* @param {string=} attrName Optional none normalized name. Defaults to key.
683701
*/
684702
$set: function(key, value, writeAttr, attrName) {
685-
//special case for class attribute addition + removal
686-
//so that class changes can tap into the animation
687-
//hooks provided by the $animate service
688-
if(key == 'class') {
689-
value = value || '';
690-
var current = this.$$element.attr('class') || '';
691-
this.$removeClass(tokenDifference(current, value));
692-
this.$addClass(tokenDifference(value, current));
693-
} else {
694-
var booleanKey = getBooleanAttrName(this.$$element[0], key),
695-
normalizedVal,
696-
nodeName;
703+
// TODO: decide whether or not to throw an error if "class"
704+
//is set through this function since it may cause $updateClass to
705+
//become unstable.
697706

698-
if (booleanKey) {
699-
this.$$element.prop(key, value);
700-
attrName = booleanKey;
701-
}
707+
var booleanKey = getBooleanAttrName(this.$$element[0], key),
708+
normalizedVal,
709+
nodeName;
702710

703-
this[key] = value;
711+
if (booleanKey) {
712+
this.$$element.prop(key, value);
713+
attrName = booleanKey;
714+
}
704715

705-
// translate normalized key to actual key
706-
if (attrName) {
707-
this.$attr[key] = attrName;
708-
} else {
709-
attrName = this.$attr[key];
710-
if (!attrName) {
711-
this.$attr[key] = attrName = snake_case(key, '-');
712-
}
716+
this[key] = value;
717+
718+
// translate normalized key to actual key
719+
if (attrName) {
720+
this.$attr[key] = attrName;
721+
} else {
722+
attrName = this.$attr[key];
723+
if (!attrName) {
724+
this.$attr[key] = attrName = snake_case(key, '-');
713725
}
726+
}
714727

715-
nodeName = nodeName_(this.$$element);
716-
717-
// sanitize a[href] and img[src] values
718-
if ((nodeName === 'A' && key === 'href') ||
719-
(nodeName === 'IMG' && key === 'src')) {
720-
// NOTE: urlResolve() doesn't support IE < 8 so we don't sanitize for that case.
721-
if (!msie || msie >= 8 ) {
722-
normalizedVal = urlResolve(value).href;
723-
if (normalizedVal !== '') {
724-
if ((key === 'href' && !normalizedVal.match(aHrefSanitizationWhitelist)) ||
725-
(key === 'src' && !normalizedVal.match(imgSrcSanitizationWhitelist))) {
726-
this[key] = value = 'unsafe:' + normalizedVal;
727-
}
728+
nodeName = nodeName_(this.$$element);
729+
730+
// sanitize a[href] and img[src] values
731+
if ((nodeName === 'A' && key === 'href') ||
732+
(nodeName === 'IMG' && key === 'src')) {
733+
// NOTE: urlResolve() doesn't support IE < 8 so we don't sanitize for that case.
734+
if (!msie || msie >= 8 ) {
735+
normalizedVal = urlResolve(value).href;
736+
if (normalizedVal !== '') {
737+
if ((key === 'href' && !normalizedVal.match(aHrefSanitizationWhitelist)) ||
738+
(key === 'src' && !normalizedVal.match(imgSrcSanitizationWhitelist))) {
739+
this[key] = value = 'unsafe:' + normalizedVal;
728740
}
729741
}
730742
}
743+
}
731744

732-
if (writeAttr !== false) {
733-
if (value === null || value === undefined) {
734-
this.$$element.removeAttr(attrName);
735-
} else {
736-
this.$$element.attr(attrName, value);
737-
}
745+
if (writeAttr !== false) {
746+
if (value === null || value === undefined) {
747+
this.$$element.removeAttr(attrName);
748+
} else {
749+
this.$$element.attr(attrName, value);
738750
}
739751
}
740752

@@ -1816,9 +1828,19 @@ function $CompileProvider($provide) {
18161828
attr[name] = interpolateFn(scope);
18171829
($$observers[name] || ($$observers[name] = [])).$$inter = true;
18181830
(attr.$$observers && attr.$$observers[name].$$scope || scope).
1819-
$watch(interpolateFn, function interpolateFnWatchAction(value) {
1820-
attr.$set(name, value);
1821-
});
1831+
$watch(interpolateFn, function interpolateFnWatchAction(newValue, oldValue) {
1832+
//special case for class attribute addition + removal
1833+
//so that class changes can tap into the animation
1834+
//hooks provided by the $animate service. Be sure to
1835+
//skip animations when the first digest occurs (when
1836+
//both the new and the old values are the same) since
1837+
//the CSS classes are the non-interpolated values
1838+
if(name === 'class' && newValue != oldValue) {
1839+
attr.$updateClass(newValue, oldValue);
1840+
} else {
1841+
attr.$set(name, newValue);
1842+
}
1843+
});
18221844
}
18231845
};
18241846
}
@@ -1958,3 +1980,19 @@ function directiveLinkingFn(
19581980
/* Element */ rootElement,
19591981
/* function(Function) */ boundTranscludeFn
19601982
){}
1983+
1984+
function tokenDifference(str1, str2) {
1985+
var values = '',
1986+
tokens1 = str1.split(/\s+/),
1987+
tokens2 = str2.split(/\s+/);
1988+
1989+
outer:
1990+
for(var i = 0; i < tokens1.length; i++) {
1991+
var token = tokens1[i];
1992+
for(var j = 0; j < tokens2.length; j++) {
1993+
if(token == tokens2[j]) continue outer;
1994+
}
1995+
values += (values.length > 0 ? ' ' : '') + token;
1996+
}
1997+
return values;
1998+
}

‎src/ng/directive/ngClass.js

Lines changed: 8 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,10 @@ function classDirective(name, selector) {
2020
// jshint bitwise: false
2121
var mod = $index & 1;
2222
if (mod !== old$index & 1) {
23-
if (mod === selector) {
24-
addClass(flattenClasses(scope.$eval(attr[name])));
25-
} else {
26-
removeClass(flattenClasses(scope.$eval(attr[name])));
27-
}
23+
var classes = flattenClasses(scope.$eval(attr[name]));
24+
mod === selector ?
25+
attr.$addClass(classes) :
26+
attr.$removeClass(classes);
2827
}
2928
});
3029
}
@@ -33,34 +32,16 @@ function classDirective(name, selector) {
3332
function ngClassWatchAction(newVal) {
3433
if (selector === true || scope.$index % 2 === selector) {
3534
var newClasses = flattenClasses(newVal || '');
36-
if (oldVal && !equals(newVal,oldVal)) {
37-
var oldClasses = flattenClasses(oldVal);
38-
var toRemove = tokenDifference(oldClasses, newClasses);
39-
if(toRemove.length > 0) {
40-
removeClass(toRemove);
41-
}
42-
43-
var toAdd = tokenDifference(newClasses, oldClasses);
44-
if(toAdd.length > 0) {
45-
addClass(toAdd);
46-
}
47-
} else {
48-
addClass(newClasses);
35+
if(!oldVal) {
36+
attr.$addClass(newClasses);
37+
} else if(!equals(newVal,oldVal)) {
38+
attr.$updateClass(newClasses, flattenClasses(oldVal));
4939
}
5040
}
5141
oldVal = copy(newVal);
5242
}
5343

5444

55-
function removeClass(classVal) {
56-
attr.$removeClass(classVal);
57-
}
58-
59-
60-
function addClass(classVal) {
61-
attr.$addClass(classVal);
62-
}
63-
6445
function flattenClasses(classVal) {
6546
if(isArray(classVal)) {
6647
return classVal.join(' ');

‎test/ng/compileSpec.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4479,7 +4479,6 @@ describe('$compile', function() {
44794479
$compile(element)($rootScope);
44804480

44814481
$rootScope.$digest();
4482-
data = $animate.flushNext('removeClass');
44834482

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

‎test/ng/directive/ngClassSpec.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,6 @@ describe('ngClass animations', function() {
321321
$rootScope.val = 'one';
322322
$rootScope.$digest();
323323
$animate.flushNext('addClass');
324-
$animate.flushNext('addClass');
325324
expect($animate.queue.length).toBe(0);
326325

327326
$rootScope.val = '';
@@ -428,7 +427,6 @@ describe('ngClass animations', function() {
428427

429428
//this fires twice due to the class observer firing
430429
className = $animate.flushNext('addClass').params[1];
431-
className = $animate.flushNext('addClass').params[1];
432430
expect(className).toBe('one two three');
433431

434432
expect($animate.queue.length).toBe(0);

‎test/ngRoute/directive/ngViewSpec.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -656,7 +656,6 @@ describe('ngView animations', function() {
656656

657657
item = $animate.flushNext('enter').element;
658658

659-
$animate.flushNext('addClass').element;
660659
$animate.flushNext('addClass').element;
661660

662661
expect(item.hasClass('classy')).toBe(true);
@@ -676,7 +675,6 @@ describe('ngView animations', function() {
676675
$animate.flushNext('enter').element;
677676
item = $animate.flushNext('leave').element;
678677

679-
$animate.flushNext('addClass').element;
680678
$animate.flushNext('addClass').element;
681679

682680
expect(item.hasClass('boring')).toBe(true);

0 commit comments

Comments
 (0)