setAttributeNode and setAttributeNodeNS can be an aliases

[foolip]

Another thing I found when investigating https://code.google.com/p/chromium/issues/detail?id=502301

In Blink/WebKit, setAttributeNode and setAttributeNodeNS are simply aliases. This means that there is no namespace and local name flag, and a case-insensitive search for an existing attribute is always done, for HTML documents. (Per spec it would be lowercasing attr.localName instead.)

An ad-hoc test, where Edge behaves like Blink/WebKit, while Gecko is the only engine to end up with two attributes:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3761

What the spec says is probably web compatible, but with some strangeness:

1. As of Make createAttribute() lowercase the input in HTML documents #75, createAttribute will lowercase the input, so also lowercasing attr.localName is unlikely to be needed for compat.
2. By doing get an attribute by name and removing the found attribute, there can still be more attributes by the same name, so no interesting invariant is upheld: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3762

So what to do?

My proposal is to make setAttributeNode do the equivalent of getAttributeNodeNS(attr.namespaceURI, attr.localName) with no lowercasing or case-insensitive lookup. setAttributeNodeNS would be an exact alias, and we could consider removing it. Usage is super low.

CC @bzbarsky @Ms2ger

[bzbarsky]

So the basic proposal is that setAttributeNode have the behavior setAttributeNodeNS currently does, right?

I guess that would be OK with me, generally speaking.

Is it true that this change of behavior only matters in cases when someone previously set an attribute via setAttribute (or perhaps setAttributeNode using a different Attr) and now is trying to reset the same attribute, but using a different case, using setAttributeNode? Or are any other cases affected?

[foolip]

Yes, keeping only the setAttributeNodeNS behavior sums it up nicely.

Only setAttributeNode calls where the Attr object has a non-lowercase localName would be affected, and you can get those from createAttributeNS or cloning attributes from a non-HTML element/document. Then the risk would be that the code depends on other attributes being removed, e.g. by calling getAttribute and assuming it will return the new attribute's value.

This all seems similar in risk to making createAttribute lowercase its input, i.e. a bit far-fetched.

[ArkadiuszMichalski]

So this change will automaticaly apply to NamedNodeMap.setNamedItem(), who share the same algorithm "set an attribute"? Usage for them is slightly higher (https://www.chromestatus.com/metrics/feature/timeline/popularity/307), but still close to 0.

[foolip]

Good catch, setNamedItem and setNamedItemNS are indeed aliases of setAttributeNode and setAttributeNodeNS in Blink. I've checked and there aren't any other internal uses.

Usage is actually lower than for setAttributeNode, here are the counters for all four APIs:

• setAttributeNode
• setAttributeNodeNS
• setNamedItem
• setNamedItemNS

[annevk]

Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1227458.

[foolip]

I have reviewed the spec change, LGTM!

[bzbarsky]

This needs web platform tests, right?

[foolip]

Yep. I was hoping to use wpt, updating as needed, when implementing the new Attr model in Blink, but I've paused that work waiting for #116 (mostly an excuse, it need not really block since it's so corner-case-y)

[bzbarsky]

OK. Gecko has some tests for this stuff, but they're pretty messy and might not test all the current edge cases (not least because they have had to evolve with spec changes, which removed old edge cases while adding new ones) so I'm not 100% sure about moving the to wpt as-is...

[bzbarsky]

In particular, see http://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug1075702.html?force=1 and http://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug469304.html?force=1 but note that they won't be updated to this change until https://bugzilla.mozilla.org/show_bug.cgi?id=1227458 lands.

[foolip]

Yeah, those tests look like they need updating. Looks like testGetAttribute() is checking that an attribute with localName 'AA' will replace an attribute with localName 'aa', but I guess that's a known issue?

[bzbarsky]

Looks like testGetAttribute() is checking that an attribute with localName 'AA' will replace an attribute with localName 'aa'

Right, since that's what setAttributeNode used to do.

[foolip]

Ha, I'd already purged that from my memory and was surprised all over again :)