From 2a8fe56997fddbad673748ce02abf649a709c4ca Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 1 Sep 2011 01:57:49 -0700 Subject: [PATCH] fix(ng:class): make ng:class friendly towards other code adding/removing classes ng:class as well as ng:class-odd and ng:class-even always reset the class list to whatever it was before compilation, this makes it impossible to create another directive which adds its own classes on the element on which ng:class was applied. the fix simply removes all classes that were added previously by ng:class and add classes that the ng:class expression evaluates to. we can now guarantee that we won't clobber stuff added before or after compilation as long as all class names are unique. in order to implement this I had to beef up jqLite#addClass and jqLite#removeClass to be able to add/remove multiple classes without creating duplicates. --- src/directives.js | 45 ++++++++++++--------- src/jqLite.js | 22 ++++++---- test/BinderSpec.js | 8 ---- test/directivesSpec.js | 92 +++++++++++++++++++++++++++++++++++++++++- test/jqLiteSpec.js | 37 ++++++++++++++++- 5 files changed, 167 insertions(+), 37 deletions(-) diff --git a/src/directives.js b/src/directives.js index b3584a3729ac..2e1040c0080a 100644 --- a/src/directives.js +++ b/src/directives.js @@ -549,16 +549,11 @@ angularDirective("ng:submit", function(expression, element) { function ngClass(selector) { return function(expression, element) { - var existing = element[0].className + ' '; return function(element) { - this.$watch(function(scope) { + this.$watch(expression, function(scope, newVal, oldVal) { if (selector(scope.$index)) { - var ngClassVal = scope.$eval(element.attr('ng:class')); - if (isArray(ngClassVal)) ngClassVal = ngClassVal.join(' '); - var value = scope.$eval(expression); - if (isArray(value)) value = value.join(' '); - if (ngClassVal && ngClassVal !== value) value = value + ' ' + ngClassVal; - element[0].className = trim(existing + value); + element.removeClass(isArray(oldVal) ? oldVal.join(' ') : oldVal) + element.addClass(isArray(newVal) ? newVal.join(' ') : newVal); } }); }; @@ -571,11 +566,17 @@ function ngClass(selector) { * @name angular.directive.ng:class * * @description - * The `ng:class` allows you to set CSS class on HTML element - * conditionally. + * The `ng:class` allows you to set CSS class on HTML element dynamically by databinding an + * expression that represents all classes to be added. + * + * The directive won't add duplicate classes if a particular class was already set. + * + * When the expression changes, the previously added classes are removed and only then the classes + * new classes are added. * * @element ANY - * @param {expression} expression {@link guide/dev_guide.expressions Expression} to eval. + * @param {expression} expression {@link guide/dev_guide.expressions Expression} to eval. The result + * of the evaluation can be a string representing space delimited class names or an array. * * @example @@ -612,12 +613,15 @@ angularDirective("ng:class", ngClass(function(){return true;})); * * @description * The `ng:class-odd` and `ng:class-even` works exactly as - * `ng:class`, except it works in conjunction with `ng:repeat` - * and takes affect only on odd (even) rows. + * {@link angular.directive.ng:class ng:class}, except it works in conjunction with `ng:repeat` and + * takes affect only on odd (even) rows. + * + * This directive can be applied only within a scope of an + * {@link angular.widget.@ng:repeat ng:repeat}. * * @element ANY - * @param {expression} expression {@link guide/dev_guide.expressions Expression} to eval. Must be - * inside `ng:repeat`. + * @param {expression} expression {@link guide/dev_guide.expressions Expression} to eval. The result + * of the evaluation can be a string representing space delimited class names or an array. * * @example @@ -650,12 +654,15 @@ angularDirective("ng:class-odd", ngClass(function(i){return i % 2 === 0;})); * * @description * The `ng:class-odd` and `ng:class-even` works exactly as - * `ng:class`, except it works in conjunction with `ng:repeat` - * and takes affect only on odd (even) rows. + * {@link angular.directive.ng:class ng:class}, except it works in conjunction with `ng:repeat` and + * takes affect only on odd (even) rows. + * + * This directive can be applied only within a scope of an + * {@link angular.widget.@ng:repeat ng:repeat}. * * @element ANY - * @param {expression} expression {@link guide/dev_guide.expressions Expression} to eval. Must be - * inside `ng:repeat`. + * @param {expression} expression {@link guide/dev_guide.expressions Expression} to eval. The result + * of the evaluation can be a string representing space delimited class names or an array. * * @example diff --git a/src/jqLite.js b/src/jqLite.js index 8cef64daaba3..122684a5a53a 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -155,16 +155,24 @@ function JQLiteHasClass(element, selector, _) { } function JQLiteRemoveClass(element, selector) { - element.className = trim( - (" " + element.className + " ") - .replace(/[\n\t]/g, " ") - .replace(" " + selector + " ", " ") - ); + if (selector) { + forEach(selector.split(' '), function(cssClass) { + element.className = trim( + (" " + element.className + " ") + .replace(/[\n\t]/g, " ") + .replace(" " + trim(cssClass) + " ", " ") + ); + }); + } } function JQLiteAddClass(element, selector) { - if (selector && !JQLiteHasClass(element, selector)) { - element.className = trim(element.className + ' ' + selector); + if (selector) { + forEach(selector.split(' '), function(cssClass) { + if (!JQLiteHasClass(element, cssClass)) { + element.className = trim(element.className + ' ' + trim(cssClass)); + } + }); } } diff --git a/test/BinderSpec.js b/test/BinderSpec.js index ecfe0682b319..68513f625b34 100644 --- a/test/BinderSpec.js +++ b/test/BinderSpec.js @@ -393,14 +393,6 @@ describe('Binder', function(){ assertHidden(scope.$element); }); - it('BindClassUndefined', function(){ - var scope = this.compile('
'); - scope.$apply(); - - assertEquals( - '
', - sortedHtml(scope.$element)); - }); it('BindClass', function(){ var scope = this.compile('
'); diff --git a/test/directivesSpec.js b/test/directivesSpec.js index 1d26973c7469..314f9a886ab0 100644 --- a/test/directivesSpec.js +++ b/test/directivesSpec.js @@ -196,13 +196,103 @@ describe("directive", function(){ expect(element.hasClass('B')).toBe(false); }); - it('should support adding multiple classes', function(){ + + it('should support adding multiple classes via an array', function(){ var scope = compile('
'); scope.$digest(); expect(element.hasClass('existing')).toBeTruthy(); expect(element.hasClass('A')).toBeTruthy(); expect(element.hasClass('B')).toBeTruthy(); }); + + + it('should support adding multiple classes via a space delimited string', function(){ + var scope = compile('
'); + scope.$digest(); + expect(element.hasClass('existing')).toBeTruthy(); + expect(element.hasClass('A')).toBeTruthy(); + expect(element.hasClass('B')).toBeTruthy(); + }); + + + it('should preserve class added post compilation with pre-existing classes', function() { + var scope = compile('
'); + scope.dynClass = 'A'; + scope.$digest(); + expect(element.hasClass('existing')).toBe(true); + + // add extra class, change model and eval + element.addClass('newClass'); + scope.dynClass = 'B'; + scope.$digest(); + + expect(element.hasClass('existing')).toBe(true); + expect(element.hasClass('B')).toBe(true); + expect(element.hasClass('newClass')).toBe(true); + }); + + + it('should preserve class added post compilation without pre-existing classes"', function() { + var scope = compile('
'); + scope.dynClass = 'A'; + scope.$digest(); + expect(element.hasClass('A')).toBe(true); + + // add extra class, change model and eval + element.addClass('newClass'); + scope.dynClass = 'B'; + scope.$digest(); + + expect(element.hasClass('B')).toBe(true); + expect(element.hasClass('newClass')).toBe(true); + }); + + + it('should preserve other classes with similar name"', function() { + var scope = compile('
'); + scope.dynCls = 'panel'; + scope.$digest(); + scope.dynCls = 'foo'; + scope.$digest(); + expect(element.attr('class')).toBe('ui-panel ui-selected ng-directive foo'); + }); + + + it('should not add duplicate classes', function() { + var scope = compile('
'); + scope.dynCls = 'panel'; + scope.$digest(); + expect(element.attr('class')).toBe('panel bar ng-directive'); + }); + + + it('should remove classes even if it was specified via class attribute', function() { + var scope = compile('
'); + scope.dynCls = 'panel'; + scope.$digest(); + scope.dynCls = 'window'; + scope.$digest(); + expect(element.attr('class')).toBe('bar ng-directive window'); + }); + + + it('should remove classes even if they were added by another code', function() { + var scope = compile('
'); + scope.dynCls = 'foo'; + scope.$digest(); + element.addClass('foo'); + scope.dynCls = ''; + scope.$digest(); + expect(element.attr('class')).toBe('ng-directive'); + }); + + + it('should convert undefined and null values to an empty string', function() { + var scope = compile('
'); + scope.dynCls = [undefined, null]; + scope.$digest(); + expect(element.attr('class')).toBe('ng-directive'); + }); }); diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js index f2a99943250e..673bee781373 100644 --- a/test/jqLiteSpec.js +++ b/test/jqLiteSpec.js @@ -190,14 +190,15 @@ describe('jqLite', function(){ }); - describe('addClass', function(){ - it('should allow adding of class', function(){ + describe('addClass', function() { + it('should allow adding of class', function() { var selector = jqLite([a, b]); expect(selector.addClass('abc')).toEqual(selector); expect(jqLite(a).hasClass('abc')).toEqual(true); expect(jqLite(b).hasClass('abc')).toEqual(true); }); + it('should ignore falsy values', function() { var jqA = jqLite(a); expect(jqA[0].className).toBe(''); @@ -211,6 +212,28 @@ describe('jqLite', function(){ jqA.addClass(false); expect(jqA[0].className).toBe(''); }); + + + it('should allow multiple classes to be added in a single string', function() { + var jqA = jqLite(a); + expect(a.className).toBe(''); + + jqA.addClass('foo bar baz'); + expect(a.className).toBe('foo bar baz'); + }); + + + it('should not add duplicate classes', function() { + var jqA = jqLite(a); + expect(a.className).toBe(''); + + a.className = 'foo'; + jqA.addClass('foo'); + expect(a.className).toBe('foo'); + + jqA.addClass('bar foo baz'); + expect(a.className).toBe('foo bar baz'); + }); }); @@ -246,6 +269,7 @@ describe('jqLite', function(){ expect(jqLite(b).hasClass('abc')).toEqual(false); }); + it('should correctly remove middle class', function() { var element = jqLite('
'); expect(element.hasClass('bar')).toBe(true); @@ -256,6 +280,15 @@ describe('jqLite', function(){ expect(element.hasClass('bar')).toBe(false); expect(element.hasClass('baz')).toBe(true); }); + + + it('should remove multiple classes specified as one string', function() { + var jqA = jqLite(a); + + a.className = 'foo bar baz'; + jqA.removeClass('foo baz noexistent'); + expect(a.className).toBe('bar'); + }); }); });