If you look at the algorithms it seems somewhat hard to imagine them being aliased.

Hello, this two bugs frome Firefox exist in Blink too (you can check the test cases):

https://bugzilla.mozilla.org/show_bug.cgi?id=1061234
https://bugzilla.mozilla.org/show_bug.cgi?id=1075702

Some works on Gecko was started to fix them.

Blink has wrong behaviour for more attribute's methods around NamedNodeMap and Element interfaces (I test all some time ago). IE is worst here more then Firefox and Chrome together:)

Here we have explanation why all methods have been restored (even if they are ugly and in most cases has small usage):
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25016#c13

(In reply to Anne from comment #1)
> If you look at the algorithms it seems somewhat hard to imagine them being
> aliased.

I noticed that they were different, but since they're aliases that difference doesn't seem needed for Web compat. Isn't this just like the removeAttributeNode()/removeAttributeNodeNS() situation? removeAttributeNodeNS() was a simple alias in Blink, and it was removed.

From some quick greping, it looks like setAttributeNodeNS() was an alias of setAttributeNode() in Presto as well, whereas Gecko does something more like the spec in `SetNamedItemInternal(Attr& aNode, bool aWithNS, ErrorResult& aError)`.

(In reply to Arkadiusz Michalski (Spirit) from comment #2)
> Hello, this two bugs frome Firefox exist in Blink too (you can check the
> test cases):
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1061234
> https://bugzilla.mozilla.org/show_bug.cgi?id=1075702
> 
> Some works on Gecko was started to fix them.
> 
> Blink has wrong behaviour for more attribute's methods around NamedNodeMap
> and Element interfaces (I test all some time ago). IE is worst here more
> then Firefox and Chrome together:)

I trust that Blink is not per the current DOM Standard in many respects. If there are any particularly bad cases that give you trouble, let me know and I'll look into fixing them. Given the usage, I can't imagine setAttributeNodeNS() is such a case.

> Here we have explanation why all methods have been restored (even if they
> are ugly and in most cases has small usage):
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=25016#c13

Did you paste the wrong link? It's true that a bunch of Attr-related methods were removed from the spec and then restored, but I don't think any *NS() method played any part.

> Did you paste the wrong link? It's true that a bunch of Attr-related methods
> were removed from the spec and then restored, but I don't think any *NS()
> method played any part.

No, it's correct, before all this methods have been restored I analyzed your counters and see that *NS() + several other methods have low usage, but event that they are all bring back. So I thought that this is a reason:
"As discussed on IRC it probably makes no sense to try to remove any of these as the complexity in all of them is the concept of attributes holding a reference to their element and we cannot get rid of that."

> I trust that Blink is not per the current DOM Standard in many respects. If
> there are any particularly bad cases that give you trouble, let me know and
> I'll look into fixing them. Given the usage, I can't imagine
> setAttributeNodeNS() is such a case.
 
In my perspective Blink isn't so bad here because I don't use all this old stuff, just only test how it's looks like after restored to DOM (in Firefox, Chrome and IE). Firefox is the closest, Chrome has some problems, mainly with the "get an attribute by name" algorithm:
https://dom.spec.whatwg.org/#concept-element-attributes-get-by-name
which is often used in other algorithms and methods. This causes that even Element.getAttribute() method is not correct in Blink (per actual sepc).

Of course I'm not saying that these *NS() methods should stay and if other want kick them, then I'll be happy:). I remember that other methods have the same low counter usage (like Document.createAttributeNS or NamedNodeMap.getNamedItemNS). After taking decision about future of this methods I can look again to my notes and write exactly which methods are wrong in Blink (if you still interested).

(In reply to Philip Jägenstedt from comment #3)
> I noticed that they were different, but since they're aliases that
> difference doesn't seem needed for Web compat. Isn't this just like the
> removeAttributeNode()/removeAttributeNodeNS() situation?
> removeAttributeNodeNS() was a simple alias in Blink, and it was removed.

Existence removeAttributeNodeNS () does not make sense by itself:
http://www.w3.org/DOM/faq.html#removeAttributeNodeNS

(In reply to Arkadiusz Michalski (Spirit) from comment #6)
> (In reply to Philip Jägenstedt from comment #3)
> > I noticed that they were different, but since they're aliases that
> > difference doesn't seem needed for Web compat. Isn't this just like the
> > removeAttributeNode()/removeAttributeNodeNS() situation?
> > removeAttributeNodeNS() was a simple alias in Blink, and it was removed.
> 
> Existence removeAttributeNodeNS () does not make sense by itself:
> http://www.w3.org/DOM/faq.html#removeAttributeNodeNS

I'm saying that setAttributeNodeNS() also does not make sense by itself, because as implemented in at least Blink and Presto (now dead) it's just an alias of setAttributeNode().

Per specification the difference between setAttributeNode and setAttributeNodeNS is how the attribute to be replaced is found.

E.g. if you have

  xml:lang (no namespace)
  xml:lang (some namespace)

on an element, those methods would differ in which of the two attributes get replaced.

Yes, that's what the spec says :) Here's a test case:
http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=3307

In Firefox and IE, the existing attribute is replaced even though the namespace doesn't match. In Blink, Presto and presumably WebKit, the attribute isn't replaced, resulting in two attributes.

There are a few paths forward that I can see:

1. Blink/WebKit changes setAttributeNode() to ignore the namespace. (This seems a little silly if not needed for Web compat.)

2. Spec/Firefox/IE changes setAttributeNode() to be just like setAttributeNodeNS().

3. 2 + remove setAttributeNodeNS() from spec and implementations

I filed the bug with option 3 in mind.

It's also silly if setAttribute() and setAttributeNode() are different, no?

Note that 1 is not just about ignoring namespaces, it is also not looking at the local name, but rather at the qualified name.

It's not clear to me what is right here. Ideally all these things die in a fire, but ...

Well yes, silly is only an argument when we're not constrained by Web compat.

Blink does use the qualified name when looking for an existing attribute, FWIW. It also ignores case for HTML elements, which isn't per spec. There are probably other minor differences as well.

Just to derail the discussion further, here's the simplest possible end state that seems possible:

Element.prototype.setAttributeNode = function (newAttr) {
  var oldAttr = this.getAttributeNodeNS(newAttr.namespaceURI, newAttr.localName);
  this.setAttributeNS(newAttr.namespaceURI, newAttr.localName, newAttr.value);
  return oldAttr;
}

(In reply to Philip Jägenstedt from comment #11)
> Just to derail the discussion further, here's the simplest possible end
> state that seems possible:
> 
> Element.prototype.setAttributeNode = function (newAttr) {
>   var oldAttr = this.getAttributeNodeNS(newAttr.namespaceURI,
> newAttr.localName);
>   this.setAttributeNS(newAttr.namespaceURI, newAttr.localName,
> newAttr.value);
>   return oldAttr;
> }

Let's pretend I didn't write this and look for the shortest path to agreement :)

(In reply to Anne from comment #10)
> It's also silly if setAttribute() and setAttributeNode() are different, no?

This also relates to NamedNodeMaps methods which in some cases have the same effect. Changing behaviour only for one method (like in above comment12) may unnecessary introduce a bigger mess than we have today. Will be better if all methods (excluding *NS) have the same behavior when they finding attribute (ie. for HTML namepsace passing name is convert to small ASCII, etc.). The only exception might be for situation when change is necessary because of the compatibility (but it will probably affect the oldest code).

In Blink, NamedNodeMap.setNamedItem() already forwards to Element.setAttributeNode() and NamedNodeMap.setNamedItemNS() is just an alias of NamedNodeMap.setNamedItem() so effectively there are 4 methods that do exactly the same thing.

Obviously I do not propose changing only one of these, but making them all do the simplest thing that's Web compatible and removing the ones that have close to zero usage.

So, this bug is really blocked on getting agreement on a path forward from several implementors. Who's the relevant Gecko developer to get feedback from?

Olli I suspect. Maybe Ehsan? It's a bit unclear to me who maintains this code.

OK, so if any Mozilla developer wants to weigh in on comment #9, that'd be great :)

I think removing setAttributeNodeNS would be good.

Why was the usage still significantly higher earlier this year
https://www.chromestatus.com/metrics/feature/timeline/popularity/216 ?

There was an attempt to remove setAttributeNodeNS that was reverted, but it looks like it was gone in M34, which was released on April 8. Then it was back in M35. Perhaps that scare can explain the drop on June 24. What happened in February I just don't know.

If Gecko were to remove setAttributeNodeNS(), would you also change setAttributeNode() to be like the current setAttributeNodeNS()? Using the qualified name seems Web compatible, because Blink/WebKit do that.

(In reply to Philip Jägenstedt from comment #19)
> There was an attempt to remove setAttributeNodeNS that was reverted
Why was is reverted?


> If Gecko were to remove setAttributeNodeNS(), would you also change
> setAttributeNode() to be like the current setAttributeNodeNS()? Using the
> qualified name seems Web compatible, because Blink/WebKit do that.
Do we have data about setAttributeNode usage for blink?
(We don't have for Gecko)

(In reply to Olli Pettay from comment #20)
> (In reply to Philip Jägenstedt from comment #19)
> > There was an attempt to remove setAttributeNodeNS that was reverted
> Why was is reverted?

It was reverted in https://codereview.chromium.org/243333003 together with Document.createAttributeNS() and Attr.ownerElement. I don't know which API caused the most breakage, but Attr.ownerElement has the highest usage of them:
https://www.chromestatus.com/metrics/feature/timeline/popularity/160

Document.createAttributeNS() is close to zero and has been from the start:
https://www.chromestatus.com/metrics/feature/timeline/popularity/112

> > If Gecko were to remove setAttributeNodeNS(), would you also change
> > setAttributeNode() to be like the current setAttributeNodeNS()? Using the
> > qualified name seems Web compatible, because Blink/WebKit do that.
> Do we have data about setAttributeNode usage for blink?
> (We don't have for Gecko)

We do:
https://www.chromestatus.com/metrics/feature/timeline/popularity/108

The relatively high usage is why I guess nobody can be exciting about changing setAttributeNode(), but setAttributeNode() doesn't work the same with namespaces in Gecko and Blink, the difference likely doesn't matter for Web compat.

At this point, I think only options 1 and 3 in comment #9 make sense. Option 1 is likely less risky. Do you have a preferred course of action?

This WebKit bug is about case-insensitive matching in setAttributeNode():
https://bugs.webkit.org/show_bug.cgi?id=90341

This makes it rather doubtful that the spec could make setAttributeNode() behave like setAttributeNodeNS() :(

What Blink currently does is an odd combination where the qualified name is used, but case is ignored depending only on the HTML-ness of the element and document, not depending on which of the setAttributeNode*() functions were called.

I found the bug that caused the revert in Blink:
https://code.google.com/p/chromium/issues/detail?id=364028

So it was createAttributeNS() and setAttributeNodeNS().

Sigh. The most sane path forward I can see right now is actually for Blink to try aligning with the spec. Are there any known cases where the spec works differently from what Gecko and/or IE does?

(In reply to Philip Jägenstedt from comment #23)
>Are there any known cases where the spec
>works differently from what Gecko and/or IE does?

The differences between actual spec and Gecko within the old attr's methods:

https://bugzilla.mozilla.org/show_bug.cgi?id=1061234
https://bugzilla.mozilla.org/show_bug.cgi?id=1075702
https://bugzilla.mozilla.org/show_bug.cgi?id=706770
https://bugzilla.mozilla.org/show_bug.cgi?id=1060938
https://bugzilla.mozilla.org/show_bug.cgi?id=1070015
https://bugzilla.mozilla.org/show_bug.cgi?id=1055465

Maybe there are more, but at the moment only this were catch. The rest work the same what we have in spec.

IE is more difficult to analyze because IE has wrong behaviour for more methods (including *NS version what is not happend in Firefox and Chrome).

(In reply to Olli Pettay from comment #18)
> I think removing setAttributeNodeNS would be good.

Sorry for jumping in late.  I assume this is moot now given comment 23, right? :(

Yeah, I think attempting removal isn't worth the trouble given the history.

I'm closing this as WONTFIX, but we still need to converge on what setAttributeNode() and setAttributeNodeNS() actually do. Comment #24 looks like a good starting point.