fix(jqLite): fix add/remove class in IE9 #9474
Conversation
Condensed logic, changed toEqual to toBe
|
Thanks for your submission, @btford. I will give this a thorough review. |
|
You are very welcome Jeff Brian Cross. I look forward to your continued help in landing this PR. |
| }); | ||
|
|
||
| (msie === 9 && !(element instanceof SVGElement)) ? element.className = trim(existingClasses) : |
There was a problem hiding this comment.
does IE9 parse classes when the attribute is set for SVG elements but not for other elements? (I know className is an object in SVG, but it's weird that this is necessary at all). Would this break SVG class operations for IE9?
In either case, there is a lot of branching happening in this function, what if we just had two versions of the function?
There was a problem hiding this comment.
what if we just had two versions of the function?
Considered it; this would also make it easy to drop support for IE9 in the future. Let me put together an alternate PR and see what that looks like.
| @@ -836,6 +845,17 @@ describe('jqLite', function() { | |||
| }); | |||
|
|
|||
|
|
|||
| it('should allow removal of class in IE9', function() { | |||
| if (!(jqLite(a).setAttribute && jqLite(a).getAttribute)) return; // IE9 doesn't support node.setAttribute | |||
There was a problem hiding this comment.
According to http://msdn.microsoft.com/en-us/library/ie/ms536739(v=vs.85).aspx,
Windows Internet Explorer 8 and later. IE8 Standards mode enables several enhancements to the setAttribute, getAttribute, and removeAttribute methods that are not available when pages are displayed in earlier document modes.
There was a problem hiding this comment.
but besides that, jqLite(a) is gonna be a jQuery/jqLite object --- so it's not going to have getAttribute or setAttribute anyways, only attr()
|
I agree with @caitp |
|
thanks jeff |
|
I've tried a few variations on SL, this was the only one I could make work consistently for SVG and non-SVG: http://jsbin.com/cidubareyuko/1/edit (although in reality, we don't need to use classList for SVG, because we can use className.baseVal instead --- this would be a lot simpler) |
jeffbcross
force-pushed
the
master
branch
2 times, most recently
from
abdaab7 to
30996f8
Compare
|
closing per #5001 (comment) |
|
I sort of disagree with the perf thing, I think we could do this, the real cost is an extra 5-10 lines of code. It's not free, but this should be doable. An alternative to this would be removing support for the weird xml-namespace-ish syntax, wdyt? |
IE is not my favorite web browser.
this is my take on #5694