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

fix(jqLite): use get/setAttribute so that jqLite works on SVG #4129

Closed
wants to merge 1 commit into from
Closed

fix(jqLite): use get/setAttribute so that jqLite works on SVG #4129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/jqLite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,29 +279,35 @@ function JQLiteData(element, key, value) {
}

function JQLiteHasClass(element, selector) {
return ((" " + element.className + " ").replace(/[\n\t]/g, " ").
return ((" " + (element.getAttribute('class') || '') + " ").replace(/[\n\t]/g, " ").
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not necessary, reading className property is not an issue for us and the property is updated with every setAttribute call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is; SVG Nodes are special. className is an object for them :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@btford is right here. In this case, the className is of type SVGAnimatedString. This has a property baseVal that you can read if you like but writing to it has no affect on the actual CSS class attribute of the element. getAttribute and setAttribute seem like the only reasonable solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

indexOf( " " + selector + " " ) > -1);
}

function JQLiteRemoveClass(element, cssClasses) {
if (cssClasses) {
forEach(cssClasses.split(' '), function(cssClass) {
element.className = trim(
(" " + element.className + " ")
element.setAttribute('class', trim(
(" " + (element.getAttribute('class') || '') + " ")
.replace(/[\n\t]/g, " ")
.replace(" " + trim(cssClass) + " ", " ")
.replace(" " + trim(cssClass) + " ", " "))
);
});
}
}

function JQLiteAddClass(element, cssClasses) {
if (cssClasses) {
var existingClasses = (' ' + (element.getAttribute('class') || '') + ' ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Igor's comment about reading className is correct then this could be done without getAttribute too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAttribute is needed here because svg elements return a special objects for className property and not a string.

.replace(/[\n\t]/g, " ");

forEach(cssClasses.split(' '), function(cssClass) {
if (!JQLiteHasClass(element, cssClass)) {
element.className = trim(element.className + ' ' + trim(cssClass));
cssClass = trim(cssClass);
if (existingClasses.indexOf(' ' + cssClass + ' ') === -1) {
existingClasses += cssClass + ' ';
}
});

element.setAttribute('class', trim(existingClasses));
}
}

Expand Down
8 changes: 6 additions & 2 deletions src/ng/animate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,9 @@ var $AnimateProvider = ['$provide', function($provide) {
className = isString(className) ?
className :
isArray(className) ? className.join(' ') : '';
element.addClass(className);
forEach(element, function (element) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't forEach try to iterate over element properties if element is not an array? I suspect that we need an isArray check here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The element passed to addClass should be a jQuery/jqLite element, so it's always an ArrayLike object, making forEach treat is as an array instead of an object

So forEach'ing it allows you to iterate over each DOM node inside the collection.

Maybe a change in the naming of the variable would make this more clear?
That's related to style more than anything else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what @rodyhaddad said.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh. ok. that makes sense.

JQLiteAddClass(element, className);
});
done && $timeout(done, 0, false);
},

Expand All @@ -177,7 +179,9 @@ var $AnimateProvider = ['$provide', function($provide) {
className = isString(className) ?
className :
isArray(className) ? className.join(' ') : '';
element.removeClass(className);
forEach(element, function (element) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

JQLiteRemoveClass(element, className);
});
done && $timeout(done, 0, false);
},

Expand Down
9 changes: 8 additions & 1 deletion test/helpers/matchers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,14 @@ beforeEach(function() {
}

function isNgElementHidden(element) {
return angular.element(element).hasClass('ng-hide');
// we need to check element.getAttribute for SVG nodes
var hidden = true;
forEach(angular.element(element), function (element) {
if ((' ' +(element.getAttribute('class') || '') + ' ').indexOf(' ng-hide ') === -1) {
hidden = false;
}
});
return hidden;
};

this.addMatchers({
Expand Down
12 changes: 12 additions & 0 deletions test/ng/animateSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ describe("$animate", function() {
expect(element).toBeHidden();
}));

it("should add and remove classes on SVG elements", inject(function($animate) {
if (!window.SVGElement) return;
var svg = jqLite('<svg><rect></rect></svg>');
var rect = svg.children();
$animate.enabled(false);
expect(rect).toBeShown();
$animate.addClass(rect, 'ng-hide');
expect(rect).toBeHidden();
$animate.removeClass(rect, 'ng-hide');
expect(rect).not.toBeHidden();
}));

it("should throw error on wrong selector", function() {
module(function($animateProvider) {
expect(function() {
Expand Down