#5001: Fixes jqLite not correctly adding & removing classes in IE9 #5694
Conversation
ghost
assigned 
|
Hi, |
|
And your tests should run in all browsers, so no need to check for ie there in specific! |
|
Hm, but I am testing for an IE specific issue - should I have not written that particular unit test then? The one just prior should presumably catch all cases, but for some reason it misses the problem with IE9. I'm not quite sure how to write the appropriate test given that the current one seems to miss this bug |
|
What we need: A unit test that fails without your patch but works with it. |
| @@ -579,6 +579,14 @@ describe('jqLite', function() { | |||
| expect(jqLite(b).hasClass('abc')).toEqual(true); | |||
| }); | |||
|
|
|||
| it('should allow adding of class in IE9', function() { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Alright, I'll go through those steps next week to verify 100% & clean up the PR with a rebase - I'm out of commission until Monday, but I believe I have a fix. I do want to do some more investigation though, because I suspect usage of |
|
I believe this PR is good for a review now. |
| @@ -312,7 +313,7 @@ function jqLiteRemoveClass(element, cssClasses) { | |||
|
|
|||
| function jqLiteAddClass(element, cssClasses) { | |||
| if (cssClasses && element.setAttribute) { | |||
| var existingClasses = (' ' + (element.getAttribute('class') || '') + ' ') | |||
| var existingClasses = (' ' + ((element.getAttribute ? element.getAttribute('class') : element.className) || '') + ' ') | |||
There was a problem hiding this comment.
Don't take this comment too seriously, since it's already been pointed out that jQuery doesn't worry about this either, but this is going to cause issues for SVG elements, where the className is an object.
|
@caitp I think I fixed all of the tests that my change broke - the current master has broken tests though due to something fishy going on with $animate and ngRoute though. Edit: Nevermind, must've been something weird going on locally. This PR is solid test-wise now with suggested changes. |
Condensed logic, changed toEqual to toBe
wesleycho
force-pushed
the
jqlite_ie9_class_fix_refactor
branch
from
1aee0e3 to
9ea4b24
Compare
petebacondarwin
force-pushed
the
master
branch
from
02dc2aa to
fd2d6c0
Compare
|
This fix seems to work for adding classes, but not for removing them. |
|
See #5001 (comment) |
This PR is a cleaned up version of #5686. This is for #5001.
Somehow my history got polluted due to bouncing between two different computers.