Skip to content

Commit c785267

Browse files
btfordIgorMinar
btford
authored and
IgorMinar
committed
fix(jqLite): use get/setAttribute so that jqLite works on SVG nodes
jqLite previously used `elt.className` to add and remove classes from a DOM Node, but because the className property is not writable on SVG elements, it doesn't work with them. This patch replaces accesses to `className` with `get/setAttribute`. `classList` was also considered as a solution, but because only IE10+ supports it, we have to wait. :'( The JqLiteAddClass/JQLiteRemoveClass methods are now also used directly by $animate to work around the jQuery not being able to handle class modifications on SVG elements. Closes angular#3858
1 parent 6aaae06 commit c785267

5 files changed

Lines changed: 52 additions & 9 deletions

File tree

‎src/jqLite.js‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -279,29 +279,35 @@ function JQLiteData(element, key, value) {
279279
}
280280

281281
function JQLiteHasClass(element, selector) {
282-
return ((" " + element.className + " ").replace(/[\n\t]/g, " ").
282+
return ((" " + (element.getAttribute('class') || '') + " ").replace(/[\n\t]/g, " ").
283283
indexOf( " " + selector + " " ) > -1);
284284
}
285285

286286
function JQLiteRemoveClass(element, cssClasses) {
287287
if (cssClasses) {
288288
forEach(cssClasses.split(' '), function(cssClass) {
289-
element.className = trim(
290-
(" " + element.className + " ")
289+
element.setAttribute('class', trim(
290+
(" " + (element.getAttribute('class') || '') + " ")
291291
.replace(/[\n\t]/g, " ")
292-
.replace(" " + trim(cssClass) + " ", " ")
292+
.replace(" " + trim(cssClass) + " ", " "))
293293
);
294294
});
295295
}
296296
}
297297

298298
function JQLiteAddClass(element, cssClasses) {
299299
if (cssClasses) {
300+
var existingClasses = (' ' + (element.getAttribute('class') || '') + ' ')
301+
.replace(/[\n\t]/g, " ");
302+
300303
forEach(cssClasses.split(' '), function(cssClass) {
301-
if (!JQLiteHasClass(element, cssClass)) {
302-
element.className = trim(element.className + ' ' + trim(cssClass));
304+
cssClass = trim(cssClass);
305+
if (existingClasses.indexOf(' ' + cssClass + ' ') === -1) {
306+
existingClasses += cssClass + ' ';
303307
}
304308
});
309+
310+
element.setAttribute('class', trim(existingClasses));
305311
}
306312
}
307313

‎src/ng/animate.js‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,9 @@ var $AnimateProvider = ['$provide', function($provide) {
156156
className = isString(className) ?
157157
className :
158158
isArray(className) ? className.join(' ') : '';
159-
element.addClass(className);
159+
forEach(element, function (element) {
160+
JQLiteAddClass(element, className);
161+
});
160162
done && $timeout(done, 0, false);
161163
},
162164

@@ -177,7 +179,9 @@ var $AnimateProvider = ['$provide', function($provide) {
177179
className = isString(className) ?
178180
className :
179181
isArray(className) ? className.join(' ') : '';
180-
element.removeClass(className);
182+
forEach(element, function (element) {
183+
JQLiteRemoveClass(element, className);
184+
});
181185
done && $timeout(done, 0, false);
182186
},
183187

‎test/helpers/matchers.js‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,14 @@ beforeEach(function() {
3131
}
3232

3333
function isNgElementHidden(element) {
34-
return angular.element(element).hasClass('ng-hide');
34+
// we need to check element.getAttribute for SVG nodes
35+
var hidden = true;
36+
forEach(angular.element(element), function (element) {
37+
if ((' ' +(element.getAttribute('class') || '') + ' ').indexOf(' ng-hide ') === -1) {
38+
hidden = false;
39+
}
40+
});
41+
return hidden;
3542
};
3643

3744
this.addMatchers({

‎test/jqLiteSpec.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -479,6 +479,20 @@ describe('jqLite', function() {
479479

480480
describe('class', function() {
481481

482+
it('should properly do with SVG elements', function() {
483+
// this is a jqLite & SVG only test (jquery doesn't behave this way right now, which is a bug)
484+
if (!window.SVGElement || !_jqLiteMode) return;
485+
var svg = jqLite('<svg><rect></rect></svg>');
486+
var rect = svg.children();
487+
488+
expect(rect.hasClass('foo-class')).toBe(false);
489+
rect.addClass('foo-class');
490+
expect(rect.hasClass('foo-class')).toBe(true);
491+
rect.removeClass('foo-class');
492+
expect(rect.hasClass('foo-class')).toBe(false);
493+
});
494+
495+
482496
describe('hasClass', function() {
483497
it('should check class', function() {
484498
var selector = jqLite([a, b]);

‎test/ng/animateSpec.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,18 @@ describe("$animate", function() {
4040
expect(element).toBeHidden();
4141
}));
4242

43+
it("should add and remove classes on SVG elements", inject(function($animate) {
44+
if (!window.SVGElement) return;
45+
var svg = jqLite('<svg><rect></rect></svg>');
46+
var rect = svg.children();
47+
$animate.enabled(false);
48+
expect(rect).toBeShown();
49+
$animate.addClass(rect, 'ng-hide');
50+
expect(rect).toBeHidden();
51+
$animate.removeClass(rect, 'ng-hide');
52+
expect(rect).not.toBeHidden();
53+
}));
54+
4355
it("should throw error on wrong selector", function() {
4456
module(function($animateProvider) {
4557
expect(function() {

0 commit comments

Comments
 (0)