Make Attr a Node again

[foolip]

Attr's child text nodes have been successfully removed in Blink recently, aligning with Gecko and the spec on this point. The big remaining difference is that Attr is not a Node in the spec, and this is a change no browser engine (except Servo) has made yet. I doubt that it's really worth it for the web platform to divorce Attr from Node.

First and foremost, it's not clear that successfully making the change would be of much value. DocumentType is already a weird kind of Node, so any algorithms dealing with nodes already have to deal with a similar case. In implementations, this is a sunk cost.

A practical issue is that it's hard to measure the risk, as every attribute and method that Attr inherits from Node and EventTarget is a potential problem. Even if that were done, the fact that instanceof Node no longer holds true is a risk that cannot be measured.

Are any implementors interested in attempting the change, and if so when?

[foolip]

No outrage? CC @Ms2ger @smaug---- @bzbarsky :)

[smaug----]

I can live with Attr being a Node. It is a bit ugly API that way (does dispatching events to Attr make any sense ever?) but it has been that way for ages.

[bzbarsky]

I'm basically with @SmauG. I agree it's silly, but I'm not convinced that changing this is worth the risks.

[foolip]

If we make this change, we should also consider if any of namespaceURI, prefix and localName should be moved back to Node. We've successfully moved them to Attr+Element in Blink, but I think no other engine has done it. I've seen one report of breakage where the difference between null and undefined really seems to have mattered, so while I think the change will stick, I wouldn't mind reverting it if nobody is inclined to follow.

[domenic]

I think it would be really sad to make Attr back into a Node. But, authors probably don't care, and implementers seem un-excited by the prospect. (I am a bit surprised nobody is excited about ripping out all the supporting code? But maybe it is just a sunk cost as @foolip says.)

Nevertheless, I would be hesitant to change the specification until Servo or some experiment shows specific evidence that the simpler model is web-incompatible. It seems OK for the spec to have the minimum-complexity model necessary for web-compat, instead of having the majority-implemented overly-complex model...

Maybe sometime in 2016 I could carve out some time to experiment in Blink.

[Ms2ger]

[Insert outrage here :)]

I don't see a reason to change the spec at this point; we'll be sure to file an issue if Servo hits trouble because of it.

[foolip]

If there is still interest in trying this out, then I also think it's worth at least one attempt. @domenic, do you have some idea for how to actually experiment with this? It seems like the very likely course of events here is that it this change will coast through dev and beta, and some time after it reaches stable the very long tail of legacy content will start biting us, and it'll be things that are very hard to find statically in httparchive, so we'll not have a good way to estimate how large the problem is.

This isn't unique to Attr of course, changing any very old API has the same risks.

[domenic]

@foolip my idea was to make minimal IDL-level changes without touching much of the rest of the engine. E.g. maybe just removing the : Node from IDL (but retaining the C++ inheritance relationship) would be enough.

I guess that does not address your concern about how you see this experiment potentially going. It seems like a discussion that would have to happen with API owners, as to whether they think the potential payoff (in actual code cleanup, not just the experimental one I mentioned) would be worth the risk. I admit I don't see that discussion going in favor of the experiment...

[foolip]

Yeah, doing just the IDL change as a first step makes sense. Other than things that implicitly depend on attr instanceof Node being true (throwing TypeError) you might also have things assuming that various attributes and methods of Node being available, and cloneNode() has been mentioned:
https://lists.w3.org/Archives/Public/public-html/2015May/thread.html#msg90

While it does seem risky to just make the change and ship it to stable, something could perhaps be learned by making it in between two branch points to see if there are reports from canary and dev. If there are many reports, then it's just game over, and otherwise we still don't know :)

[ArkadiuszMichalski]

You can add some telemetry for Attr.cloneNode(), and if the changes go forward and this method will be required for web compact, then put some variant of them directly in Attr interface (like other old crap:). It's sad, the same story as CDATA...

[foolip]

I've considered measuring usage of cloneNode(), but I'm not sure what we would learn. If usage is too high to make Attr a non-Node, then it starts to get a bit silly to have both nodeValue and cloneNode() on Attr instead of leaving it as a node.

[ArkadiuszMichalski]

If making Attr to not inherit from Node is still considered (even in the long term), then having usage of cloneNode() may be useful in future, because someone reported the potential problem with its absence for Attr. At now I see some data for:
Attr.nodeValue https://www.chromestatus.com/metrics/feature/timeline/popularity/348
Attr.textContent https://www.chromestatus.com/metrics/feature/timeline/popularity/349
Attr.specified https://www.chromestatus.com/metrics/feature/timeline/popularity/162
Attr.ownerElement https://www.chromestatus.com/metrics/feature/timeline/popularity/160
With more data you don't have to wait another few weeks/months to take next decisions around Attr, like placing equivalent of cloneNode() directly in the Attr (if we really need them). But the basic question is: if anyone really considering making Attr a non-Node, at least as an experiment in most recent version of engine? After few years this is still just a idea (excluding Servo), but somehow received a recommendation (http://www.w3.org/TR/dom/)... perhaps too early and only creates confusion...

[foolip]

Very well: https://codereview.chromium.org/1474083002/

[foolip]

The use counter for cloneNode has now reached Chrome's stable channel:
https://www.chromestatus.com/metrics/feature/timeline/popularity/1034

Unfortunately, chromestatus.com is not feeling well, so @RByers has helped me check the usage internally, and says that it is 0.0007% of page views.

For something that's worked in all browsers for decades and that would start throwing exceptions, I don't think that just dropping it would be OK. So either Attr has to add cloneNode, or it needs to inherit from Node again.

[domenic]

For something that's worked in all browsers for decades and that would start throwing exceptions, I don't think that just dropping it would be OK.

Hmm. What would be your cutoff?

[foolip]

The truth is I didn't have a cutoff in mind before looking at the data, and I don't know how to honestly pick one after the fact. This is a case where we'd be moving away from interop and taking compat risk at the same time. The problem is that I don't see much on the positive side to weigh against that, at least when I tried making Attr a non-Node in Blink in an experimental CL, not much interesting simplification came out of it.

[DigiTec]

Edge/IE both don't support, properly, the usage of EventTarget on Attr and we've never seen a compatibility issue as such. Meaning that at least some portion of the hierarchy is unnecessary. That would leave only properties on Node that could be a potential compat issue.

[DigiTec]

Also, for Edge this would simplify quite a bit of code. Our tree stores all other node types the same way, but Attr is special and is not stored in the tree itself. So this means we have virtual overrides and double checks for all cases where an Attr is being used since most tree algorithms is cannot participate in.

[smaug----]

What kind of object is Attr then in IE/Edge? It is a Node but not really? (Node is supposed to inherit EventTarget)
Attr.prototype.__proto__.__proto__ should be EventTargetPrototype

[DigiTec]

Attr : Node : EventTarget

However, our implementations of addEventListener don't store listeners for the type and so its "not really" an EventTarget. Its only an EventTarget via instanceof

[smaug----]

Apparently not in IE. It is Object there. Don't have access to Edge right now.

[DigiTec]

Yes, my apologies. The EventTarget reparenting was work I personally checked into the Edge fork for "IE 12". In IE we supported EventTarget via implements, not via hierarchy. I can provide a bunch of details if that would help, but I think there is a more specific question/concern you might have instead?

[foolip]

@DigiTec, that is fantastic feedback! It should give us some confidence that the bits inherited from EventTarget are not needed for compat at the very least.

Is Attr simplification something you'd like to try out in Edge?

[foolip]

Oh wait, "via implements, not via hierarchy." I guess this means all the methods and attributes of Node and EventTarget were always available on Attr?

In IE 11.0.9600.18059 an Attr object is instanceof both Node and EventTarget, so did this really change only on the Edge fork? Don't have older IE images here to test with...

[DigiTec]

This is IE 11:
Attr : Node
Node implements EventTarget
but Attr is not instanceof EventTarget. In fact, IE 11 doesn't even have EventTarget as a concrete interface you can use with instanceof

This is Edge:
Attr : Node : EventTarget

However, as noted, our implementations of addEventListener/removeEventListener/dispatchEvent were broken on Attr types. So we can assume that even though they existed, usage of them would not have worked and thus there must not be a lot of usage.

Attr : Node is still a concern, and I see your points above about things like cloneNode not being on the new Attr. At this point in our cycle, super large changes wouldn't be possible so we would not be able to make this into our next release. However, I'm quite interested in moving this forward in as compatible a way as possible. Even if that means adding some methods to Attr temporarily that can be better use tracked. Right now all of my Node information can't be disambiguated (we don't annotate which derivation/typeid was invoked we only track the entrypoints which are on Node)

[foolip]

So it sounds like we can reasonably assume that none of the 3 EventTarget methods are needed for compat. Having usage data would still be nice, of course.

The remaining uncertainty is all the remaining constants, attributes and methods of Node. For example ownerDocument, that does return the creating document. Someone just needs to go through all of these and think about which make any potential sense and which are simply useless and unlikely to be needed. insertBefore() is one method which has never done anything sensible and is unlikely to be needed.

In other news, chromestatus.com is back up, and is showing just 0.0002% for cloneNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/1034

I have been hesitant to just add everything from Node to Attr for measurement purposes, because that in itself carries some small risk, but I don't think there's any other way around it. @DigiTec, are you able to instrument this in Edge without making web-facing changes, or would you be willing to make the web-facing changes in order to measure it?

[DigiTec]

I think Attr is quite unused. I'll investigate what we can do and get back to you. We might be able to come up with a solution that doesn't require web facing changes that would be "mostly" accurate.

[DigiTec]

I'm going to be working on seeing if we already have the data this week. I believe we do, however, our data pipeline may not be able to expose it as currently written. Which could prove problematic, but if it can provide the data, even if it takes a few weeks to build, would be ideal since it would require no web facing changes!! ;-)

[foolip]

Looking forward to whatever data you can dig up!

[tml]

👍 createTreeWalker() can no longer reach Attr because they're not Node

[cdumez]

What's the status on this? The current specification does not match browser implementations and has not in a while. It does not seem like any major browser was able to align with the specification due to compatibility. I would personally like it if the change was reverted from the specification.

[foolip]

I think that @DigiTec is no longer at Microsoft (right?) so I guess we can't expect any more data to inform the situation.

Because no engine except Servo has attempted this yet nobody has a good idea about the likely compat fallout from this, but some pain seems very likely.

Based on https://www.chromestatus.com/metrics/feature/timeline/popularity/1034 we can say that cloneNode() is needed, and we don't know if the list ends there. As I mentioned in #102 (comment), it doesn't seem like a big simplification in Blink, and I assume the same is true in WebKit.

I think we should set a deadline for implementer interest to materialize, and if things remain unchanged abandon this simplification, keeping Attr in the gallery of silly things on the web platform.

[cdumez]

I do not intend to simplify Attr in WebKit. I personally don't think the change is worth the compatibility risk and major browsers already agree with each other.

[annevk]

My plan is to give up on most hopeful changes (except perhaps mutation events), including this one, this August. Make Attr a Node (but disallow children), add CDATASection back, etc. If folks want to help, PRs would be greatly appreciated.

[annevk]

I created a PR. It's not entirely clear to me if it's complete (in particular around methods that take a node as argument), but it does seem like a major improvement over the status quo.