IE11/10 multiple argument remove and add not working due to SVG test

[mjschlot]

Near the top of the code you check for:
!("classList" in document.createElementNS("http://www.w3.org/2000/svg","g"))

IE11 and 10 will fail that test and therefore should run the code for the polyfill. But, IE11 and 10 do provide native support but lacks SVG support and lacks multiple classname arguments for add() and remove(). (Reference: http://caniuse.com/#search=classlist)

The code within the else of that same if statement contains a polyfill to add multiple argument support to add() and remove(). There is a comment in that block of code that says "Polyfill for IE 10/11 and Firefox <26." But IE11 does not get to that point.

Are you still maintaining this library? If you are no longer maintaining, please let me know if there is a reason you stopped other than limited time to so do. If you believe addressing this potential bug is needed but you don't have time, let me know and I'd be happy to dig in deeper.

BTW, I took a quick look at the unit tests and it appears we may be should add testing of multiple arguments for add() and remove() as well.

[eligrey]

@mjschlot Yeah, I've been busy as of late. I will review any pull requests you give me; just make sure to mention @eligrey in the pullreq.

[mjschlot]

@eligrey Same here. This entire year has been very busy at work, but I think I can work on this next week. I'll create a pull request when ready.

[stevenvachon]

Already done: #53

[mjschlot]

Morning @stevenvachon. Thank you for pointing out issue #44. I had not realized that issue is related.

I looked into the IE10 / 11 problem this past weekend and have a good idea of the cause. Issue #33's discussion does not appear to completely understand the root cause of the problem. I also reviewed your code this morning while on the subway, and I like a number of changes you made, but do not see you addressing the IE10 / 11 issue. Could you explain?

Your modification to the if statement for the first block has a small mistake which will cause major issues. You modified the if to say:
document.createElementNS && !("classList" in document.createElementNS("http://www.w3.org/2000/svg","svg").appendChild(document.createElement("g")))

The problem is that you are creating the g element without the namespace. It should be:
document.createElementNS && !("classList" in document.createElementNS("http://www.w3.org/2000/svg","svg").appendChild(document.createElementNS("http://www.w3.org/2000/svg","g")))

Your code is actually testing if there is classList support in HTMLElement which is the same as what the first part of the if is doing. As a result of your new if statement IE 10 and 11 do not run the first block of code, and get to the else instead. While this will add multiple argument support to add() and 'remove()' it will break the shim adding support for classList to SVGs.

@eligrey do you want to merge in @stevenvachon changes (minus the if statement) anyway? I should still have time later this week to address the IE10/11 issue. I know what the issue is but want to think through the solution a little more first before submitting a pull request.

[stevenvachon]

@mjschlot #33 appears to be unrelated (?).

I have passing tests for svg elements using the above line that you pointed out. They pass in Electron and in IE (if manually ran). Also, why create a namespace on the <g> when its parent already has it?

[mjschlot]

@stevenvachon Oops sorry about that. I meant #53 not 33.

I took a quick look at the tests in your branch and see they have the same flaw which is why the current code is passing the unit tests.

Try running this in IE11 in the console:
document.createElementNS("http://www.w3.org/2000/svg","svg").appendChild(document.createElement("g")) instanceof SVGElement;
That will return false

Then try:
document.createElementNS("http://www.w3.org/2000/svg","svg").appendChild(document.createElement("g")) instanceof HTMLElement;
That returns true.

Without specifying the namespace the element being created is an HTMLElement and not the intended SVGElement. The if statement I have been referring to must specify the namespace, and the tests must as well. Keep in mind that you can mix HTML in SVG and SVG in HTML which is why you are getting this behavior. IE11 is doing is right. If you test in Chrome you will see the same right.

For the unit test I highly recommend putting SVG artwork on the HTML page and test using that instead of generating SVG elements. via Javascript This more "real life" test will be more of an accurate test in my opinion.

BTW, the if statement in @eligrey code is correct (as far as I know.) There is no reason to create a <g> to test for classList support for SVGElement. ... That I know of.

[beck]

I don't see much reason to keep the partial implementation check & upgrade quarantined in the else block. Removing the else and always verifying we have full support is a simple fix.

Pull opened: #57

[beck]

Isn't this issue a duplicate of: #44

[stevenvachon]

@beck Yes

@mjschlot I'll look into this when I have some free time.

[mjschlot]

@beck

1. Yes, you are correct this appears to be a duplicate of No normalization of add/remove/toggle in IE10 and IE11 #44 . My bad.
2. Your fix is exactly the route I was thinking.

For anybody interested:
I was confused at first when testing this solution last weekend as to why the shim in part 1 of the if statement didn't fix IE 11 and 10, but quickly realized the HTMLElement class extends Element. And we shim at Element but it is natively overridden by IE who must implement classList at the HTMLElement level. So really the two blocks of code should not share an if statement. For IE 10 and 11 the first part will then polyfill support at Element level which add classList support for SVGs while the else block builds on top of the native support at the HTMLElement to add multiple argument support for add() and remove().

Let me know if there is anything I can do to help, but it looks like @beck has this handled. If you want me to expand the test cases or anything let me know. (... Otherwise, I'm going to go review the meaning of "shim" vs "polyfill" cause I feel like an idiot since I know I'm using them wrong.)

[stevenvachon]

If @beck's PR is the best solution, I can rebase once merged so that my test suite makes it in. I'll look at #20.

[englishextra]

@eligrey @stevenvachon @beck
In IE11:
wont work - https://jsfiddle.net/englishextra/fhsjpsdt/
using https://raw.githubusercontent.com/eligrey/classList.js/master/classList.min.js

will work - https://jsfiddle.net/englishextra/hru3Lt77/
using https://raw.githubusercontent.com/beck/classlist-polyfill/d94a623c25bc69caf09f7089c0066fd65e760e82/classList.js

Had to use th latter commit, wondering why the current release doesnt get merged with beck's pr #57

[stevenvachon]

Because @beck says he'll do something and doesn't do it.

Edit: sorry, not @beck; I'd meant @eligrey

[thomasw]

@beck is the worst though (for reasons unrelated to this conversation).

[englishextra]

@stevenvachon Yes. eligrey. Thanks for your response.
@thomasw Oh yeah. Men, My concern is production.

Thanks.

[beck]

Fix merged. Published at: https://github.com/yola/classlist-polyfill