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

#5001: Fixes jqLite not correctly adding & removing classes in IE9 #5694

Closed
wants to merge 3 commits into from
+15 −5
Closed

#5001: Fixes jqLite not correctly adding & removing classes in IE9 #5694

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4259fd
Fixed code style, added test
Jan 8, 2014
ff77e1a
Fixed jshint errors
wesleycho Jan 10, 2014
9ea4b24
Tweaked test, fixed error
wesleycho Jan 10, 2014
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
12 changes: 7 additions & 5 deletions src/jqLite.js
View file
Original file line number Diff line number Diff line change
@@ -358,10 +358,11 @@ function jqLiteHasClass(element, selector) {
}

function jqLiteRemoveClass(element, cssClasses) {
if (cssClasses && element.setAttribute) {
var setter = element.setAttribute ? function(value) { element.setAttribute('class', value); } : function(value) { element.className = value; };
Copy link
Contributor

Choose a reason for hiding this comment

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

We could cache this:

var setClass = window.Element.prototype.setAttribute === 'function' ? // Need to be sure that window.Element exists and has a prototype though
  function(domNode, applicableClasses) {
    domNode.setAttribute('class', applicableClasses);
  } :
  function(domNode, applicableClasses) {
    domNode.className = applicableClasses;
  };

function jqLiteRemoveClass(element, cssClasses) {
  ...

This is slightly preferable (to me) compared with re-calculating and creating a new closure all the time. But someone else might have a word about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this change would break these unit tests:

jqLite class should properly do with SVG elements
$animate without animation should add and remove classes on SVG elements
ngAnimate should block and unblock transitions before the dom operation occurs

If I had to suspect, SVG elements might be a little special, and a higher level function which checks whether the element is an SVG element or regular element may be necessary here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, in order to maintain compat with jQuery, we should simply not care if it's an SVG element or not. jQuery seems to not distinguish between them, although there's no reason why they couldn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have spent a lot of time on trying to refactor & getting the unit tests to pass/remove SVG related unit tests - I am having a difficult time figuring out how to fix ngAnimate should block and unblock transitions before the dom operation occurs breaking. It would appear that $animate is using node.setAttribute implicitly somewhere while not calling element.addClass, although I'm not sure how - my first suspicion is with element.attr with jqLite, but it doesn't seem to be the culprit.

if (cssClasses && (element.setAttribute || msie === 9)) {
forEach(cssClasses.split(' '), function(cssClass) {
element.setAttribute('class', trim(
(" " + (element.getAttribute('class') || '') + " ")
setter(trim(
(" " + (element.getAttribute('class') || element.className || '') + " ")
.replace(/[\n\t]/g, " ")
.replace(" " + trim(cssClass) + " ", " "))
);
@@ -371,7 +372,7 @@ function jqLiteRemoveClass(element, cssClasses) {

function jqLiteAddClass(element, cssClasses) {
if (cssClasses && element.setAttribute) {
var existingClasses = (' ' + (element.getAttribute('class') || '') + ' ')
var existingClasses = (' ' + (element.getAttribute('class') || element.className || '') + ' ')
.replace(/[\n\t]/g, " ");

forEach(cssClasses.split(' '), function(cssClass) {
@@ -381,7 +382,8 @@ function jqLiteAddClass(element, cssClasses) {
}
});

element.setAttribute('class', trim(existingClasses));
(msie === 9 && !(element instanceof SVGElement)) ? element.className = trim(existingClasses) :
Copy link
Contributor

Choose a reason for hiding this comment

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

instanceof SVGElement potentially results in a deep prototype lookup, and that's not totally ideal for performance reasons, for now it might be better to just ignore the possibility of SVG, as jQuery does... I think we should make jqLite more or less match jQuery's behaviour since that's what people expect, but maybe handling special cases for SVG elements can be done in the HTML compiler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, without that line, an SVG test breaks though.

Copy link
Contributor

Choose a reason for hiding this comment

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

which test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it was this one: https://github.com/wesleycho/angular.js/blob/06dd5600eebe494c1e233eb3a10128816fefaccd/test/jqLiteSpec.js#L527

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, well regardless, this change looks a bit weird, the logic is like

if we are IE9 and this is not an SVGElement, use className, otherwise use setAttribute

But the logic that would make sense is more like

If we have setAttribute, use setAttribute, otherwise if element.className is a string, use element.className

This should be more performant than the instanceof check, and feature-checking is generally a better way to work than relying on browser sniffing.

I would look at writing this like

existingClasses = trim(existingClasses);

if (element.setAttribute) {
  element.setAttribute('class', existingClasses);
} else if (isString(element.className)) {
  element.className = existingClasses;
}

This could be obfuscated/shrunk with clever operator use, but it basically comes down to feature detection rather than sniffing, and avoiding the prototype lookup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll make the fix - definitely sounds better than what I wrote

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

8 changes: 8 additions & 0 deletions test/jqLiteSpec.js
View file
Original file line number Diff line number Diff line change
@@ -713,6 +713,14 @@ describe('jqLite', function() {
expect(jqLite(b).hasClass('abc')).toEqual(true);
});

it('should allow adding of class in IE9', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the issue was reported as only affecting nodes that had both ng:class and ng:click directives in that xml style, or maybe some combination of similar circumstances.

Does this patch fix that? If so, that's probably what should be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually turns out to be a broader issue - I'm not sure why it fails only in this particular case. ng:class fails alone without ng-click or ng:click.

if (!(jqLite(a).setAttribute && jqLite(a).getAttribute)) return; // IE9 doesn't support node.setAttribute
var selector = jqLite([a, b]);
expect(selector.addClass('abc')).toBe(selector);
expect(jqLite(a).hasClass('abc')).toBe(true);
expect(jqLite(b).hasClass('abc')).toBe(true);
});


it('should ignore falsy values', function() {
var jqA = jqLite(a);