IE11/10 multiple argument remove and add not working due to SVG test about classlist.js HOT 16 CLOSED

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

from classlist.js.

@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.

from classlist.js.

@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.

from classlist.js.

Already done: #53

from classlist.js.

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.

from classlist.js.

@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?

from classlist.js.

@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.

from classlist.js.

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

from classlist.js.

Isn't this issue a duplicate of: #44

from classlist.js.

@beck Yes

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

from classlist.js.

@beck

1. Yes, you are correct this appears to be a duplicate of #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.)

from classlist.js.

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.

from classlist.js.

@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

from classlist.js.

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

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

from classlist.js.

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

Thanks.

from classlist.js.

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

from classlist.js.