-
Notifications
You must be signed in to change notification settings - Fork 295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
setAttributeNode and setAttributeNodeNS can be an aliases #117
Comments
So the basic proposal is that 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? |
Yes, keeping only the Only This all seems similar in risk to making |
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. |
Good catch, Usage is actually lower than for |
I have reviewed the spec change, LGTM! |
This needs web platform tests, right? |
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) |
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... |
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. |
Yeah, those tests look like they need updating. Looks like |
Right, since that's what |
Ha, I'd already purged that from my memory and was surprised all over again :) |
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:
createAttribute
will lowercase the input, so also lowercasingattr.localName
is unlikely to be needed for compat.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
The text was updated successfully, but these errors were encountered: