Dom: Node.prototype.rootNode is not Web compatible

Created on 30 Apr 2016  ·  89Comments  ·  Source: whatwg/dom

We've got at least two P1 bug reports for websites being broken after adding rootNode on Node. We need to rename this attribute.

shadow

Most helpful comment

Okay, so Google is objecting to this being added as an attribute?

I find it extremely frustrating that representatives from a single company keep saying different things in a cross-vendor discussion, often conflicting their own representatives. With that kind of inconsistencies, I can't decide how serious I need to take any of your feedback since another representative from your company can just disagree with you and veto whatever other person has agreed to in public.

I almost don't see a point in engaging in the discussion if each one of you keep coming back with a different answer on every discussion like this.

All 89 comments

@annevk @hayatoito

highestAncestorNode? I previously suggested treeRoot but that name seems to be too popular according to a quick github search.

And then people ask why it's bad to add custom props to prototypes.

I don't think people are adding things to prototype in this case. They're adding it on Node instances themselves. The problem is that they're adding rootNode lazily, and the fact that Node.prototype.rootNode exists (and therefore node.rootNode returns a non-falsey value) is screwing them over :(

That is shitty, rootNode was kinda perfect. Even treeRootNode seems somewhat popular as a name. furthestInclusiveAncestorNode seems unique, but a bit verbose (highestAncestorNode doesn't indicate it could be the node itself).

Also paging @smaug---- since Gecko was implementing this.

How about enclosingRoot / enclosingRootNode or treeTop (surprisingly unpopular in JavaScript).

Another crazy idea: Node#getRootNode() or just Node#getRoot().

FYI.
Blink got one report for TheBay.com.
https://bugs.chromium.org/p/chromium/issues/detail?id=600950
, which is marked as WONTFIX, as of now.

Yeah, we can choose a different name which is unlikely to being used in the world, however, we can not be 100% sure there is no web site which uses a chosen name in a bad manner. Even if we rename rootNode to _something_, we might get a report for _something being used in a web site_ in the future. Then, should we choose another name again?

It would be nice that the site owner can fix this kind of issues promptly, but I am afraid that we can not depends on that. Is there any criteria for this kind of "name conflicts", in terms of Web Standards?

Is there any criteria for this kind of "name conflicts", in terms of Web Standards?

There's no clear methodology that I know of. You try something, hope it works out, and if it doesn't to the extent implementers are no longer comfortable shipping, you need to try something else.

@NekR it's rather inconsistent with the existing interface, e.g., parentNode.

Thanks.

As of now, Blink does not have a plan to unship _rootNode_.

We got a report a few days ago. Unless we get more and more reports, it is unlikely to unship rootNode, I think. Thus, I would like to wait for a couple of days. If we get a lot of breakage reports, and the site owners can not fix this promptly, we might need to take an action.

We're definitely unshipping this feature in https://bugs.webkit.org/show_bug.cgi?id=157233, we will NOT consider adding it back in the future.

There is no point in picking a name has a known compatibility issue.

Just curious, how much sites it breaks? 3/10/999+?

We'll probably disable rootNode in Gecko beta/release until we know whether it is good enough from web compatibility point of view or whether the name will need to be changed.
Do we have any data on how commonly web sites use the property on Node? And which sites? @rniwa, could you perhaps tell which sites have caused issues for Apple?

We've identified that two bug reports for Mytricare.com and mailchimp.com are caused by this feature but there could be many more that haven't been investigated yet. Mytricare.com become completely dysfunctional (blank), and key UI elements stop to function on mailchimp.com (drop down menu no longer works).

We removed rootNode from WebKit in http://trac.webkit.org/changeset/200297.

I could live with treeTopNode. It's rather unfortunate we can't pick the names that are best, but so be it.

I'm not sure this will be a web-compatible name either, but what about just root? Another possible alternative would be topNode instead of treeTopNode.

Regarding whether the breakage is acceptable: so far the fallout is three broken websites, in some cases severely. This is with fairly limited testing so far. I worry that this means website use of rootNode in this hack way might me more widespread, possibly due to some sort of library or shared JS code doing it.

Unfortunately, both root and topNode seem quite popular in Github code search so I doubt we can use either one of those names :(

Well, in theory even names that existing code uses should be web-compatible, as long as the existing code doesn't do something particularly weird like mixing together DOM nodes and other objects, and assuming the presence of obj.root means it's a non-DOM node. Which, apparently, several sites are doing for rootNode.... But I'd still think we could try those other names.

Can someone confirm that the two sites @rniwa mentions have the same problems in Chrome Canary, just in case it's some weird browser-specific interaction? If it's something weird then maybe we have more alternatives for working around it.

Can someone confirm that the two sites @rniwa mentions have the same problems in Chrome Canary, just in case it's some weird browser-specific interaction? If it's something weird then maybe we have more alternatives for working around it.

I've already confirmed that.

Thanks. I guess the remaining substance of my post is that we have two options:

  • Keep trying for a nice short name. There's no real way to do this besides trying it in nightlies and seeing what happens. This might involve a few tries, and might ultimately fail.
  • Just go for something long and very unlikely to conflict, in the hopes that it will stick the first time.

I think the first strategy is plausible and nice, if implementers are willing.

I think treeTop and highestNode are two names that don't match many results on Github for JavaScript (0 for treeTop and 16 for highestNode).

I imagine that no one uses treeTop because a naming scheme where the top and root of the tree are the same point is pretty screwy. For that reason, if this comes down to a straight choice between various naming options, I prefer the longer names like furthestAncestor.

Is anyone up for trying just "root"?

Is it really root node? I mean, can it refer to document or only to just
element? If it's not important to have document there, then rootElement
probably may work too.

The actual root node, yes. Supposedly it will refer to documents and shadow roots, primarily.

I previously suggested treeRoot but that name seems to be too popular according to a quick github search.

Would nodeTreeRoot work (node tree being a concept)? It's not popular. Of course, if root works that's even better.

treeRootNode then (node last), but seems overkill anyway.

Chrome also revered the change.

🚲 🏡

nodeRoot
radix(Node):
radicle(Node): https://en.wikipedia.org/wiki/Radicle (lol?)
alphaNode

I think radicle is a sufficiently unusual word that people outside the English speaking world won't be able to find it. And the term alpha has way too many overloaded meanings in our industry that it's rather confusing.

I think any name that ends with ancestor is confusing. And names like root and topNode seem to be already being used in the wild. Even names like treeRootNode are taken.

The current contenders are treeTop and highestNode. I think we should pick one or the other.

nodeTreeRoot and treeTopNode would work but it's a bit long. So does nodeAtRoot but it's strange to have node at the beginning given we have parentNode, etc...

Yet another alternative is to add a method like findRootNode instead of an attribute. @annevk pointed out on #whatwg that we could add more options like {piercing: true} in the future if we did that. I briefly looked through Github, and there doesn't seem to be many instances of findRootNode (58 of them).

I guess yet another alternative is to add two attributes one for the root of a tree, and another one for the shadow-including root of a tree.

Perhaps immediateRootNode / closestRootNode and piercingRootNode / furthestRootNode?

FWIW, I don't like 'piercing', especially when going from shadow->light DOM way.
Shadow is supposed to be able to access light DOM, so there isn't really 'piercing' happening.
Piercing selector was other way round.

The current contenders are treeTop and highestNode. I think we should pick one or the other.

nodeTreeRoot and treeTopNode would work but it's a bit long.

Note, the length of highestNode is the same as length of treeTopNode.

To my non-native-English-speaker ears 'highest' gives a bit wrong associations. Sounds like somehow qualifying thing.
treeTop or nodeTreeRoot or nodeTreeTop feel better.

But I start to like findRootNode(). It deals with the current node tree, and then once we figure out what kind of parameters would be good for it, it could deal with also ancestor node trees.
And not having it as a property, at least to me, hints that the operation isn't necessarily O(1).
(I assume it will be O(1) within same node tree, but not necessarily when dealing with multiple nested shadow trees)

Hmm, I thought we could use closest() but that only matches elements. findRootNode() seems fine.

It's O(n) when a node is neither in a shadow tree or a document tree for us (WebKit).

Let's settle on findRootNode() then, pending approval by @hayatoito next week.

Hmm.. So getRootNode() isn't consistent with parentNode, but findRootNode() is fine?

@NekR I think getRootNode() / getRoot() might also be fine if we are to go with a method. Not entirely sure why the "find" prefix was proposed. Sorry for not going back over the earlier suggestions.

I'm fine with getRootNode.

Not that I'm against getRootNode(), but was ownerRoot discussed before?
It's a bit awkward, but consistent with ownerDocument.
On May 5, 2016 19:10, "Ryosuke Niwa" [email protected] wrote:

I'm fine with getRootNode.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/whatwg/dom/issues/241#issuecomment-217197553

The semantics is very different. A node always belongs to a document regardless of whether it's in a document or not, and this is where the term "owner" comes from.

On the other hand, the root of a tree is the node or its highest ancestor, which could change multiple times over the lifetime of the node. It doesn't own any of its nodes.

Just to check: nobody wants to try root? Even if it's used in existing code, it doesn't necessarily cause compat problems. (Unless someone already found problematic patterns in common libraries.)

@rniwa I see, thanks.
@domenic root is fine, but I do not know.. it's, too simple?

Btw, just a bit off-topic question, won't root introduce some confusion? i.e. in XML (and XHTML?) the _document's root_ is first enclosing element, which in case of HTML is .

I know HTML is not XML, but still.

If you were asking me, I did be call it rootElement and make it always refer to an _element_. In case of XML it would be _document's root_, in case of HTML it would be <html> (document.documentElement) and in case of ShadowDOM to shadow root.

The document's root is not <html>. That is the document element; it has nothing to do with the root.

Well, someone should fix MDN. (And nobody should pay attention to that W3C document.)

CSS calls it root and refers to HTML: https://www.w3.org/TR/css3-selectors/#root-pseudo

I guess your confusion is between root element and root node.

@domenic well, you are wrong.

I do not have confusion, I know what means _root node_ here and what is root element in HTML and XML. Thing is -- it will produce much confusion, at least you will call property just root. Which root? element or node?

I'm definitely NOT willing to try root in WebKit given rootNode didn't work. I think we should go with getRootNode().

rootAncestor or rootAncestorNode? Later not used in the wild: https://github.com/search?utf8=%E2%9C%93&q=rootAncestorNode&type=Code&ref=searchresults

Is there any problem making rootAncestor = null on root itself?

rootOriginNode?

I just came back from a vacation. Sorry for the late reply. I do not have any strong opinion about a new name.

It looks we have a dilemma: :(

  • It we choose a good name, the name is likely to be used in the wild.
  • If we choose a bad name, the name is unlikely to be used in the wild.

BTW, "property vs method" is the first thing we have to choose. If we are to use a method, I'm +1 for getRootNode().

I think most UAs can return getRootNode() in O(1) if the node is in a document tree or a shadow tree, but it might take O(N) if the node is in neither of them. Thus, a method might make sense, IMO.

(@NekR, I think the confusion "root" might cause is acceptable. But I also expect most folks to know that the document is the typical root of the node tree. That fact is constantly used with event delegation and such.)

If we do getRootNode() we can also do getRootNode({composed:true}) so you can figure out if the "ultimate" root is a document or not. That seems somewhat nice.

Method is very weird, can we do treeRoot instead?

@esprehn I recommend searching through the issue first. That came up in the third comment.

But I also expect most folks to know that the document is the typical root of the node tree. That fact is constantly used with event delegation and such.

Yeah, I agree. Problem is only with naming, e.g. root and topRoot are confusing, while rootNode and getRootNode() is much-much less confusing since it explicitly points to Node, not Element.
I know you probably have reasons to have document as a root in this property, this is why I "just think" that rootElement could be better here. But nevermind, it was just a bit of off-topic.

Are we going with getRootNode() then?

How would rootElement be better? Shadow trees don't have a root element. Normal node trees don't necessarily have a root element either. Only document trees have a root element.

I thought we'd go with getRootNode(), but implementers flip-flopping isn't helping. If it's just @esprehn then I'm inclined to go with it since it's taking rather long, but we can wait a little longer I suppose.

@annevk okay, I probably wrong about parent element. Let's ignore those my comments.

Those broken sites should just change their implementations :)

I have created a pull request for renaming rootNode. I plan on landing it tomorrow if there are no further arguments.

getRootNode() sounds ok to me.

I object to a method, it makes this API strange compared to the rest of the web and sets a bad precedent. Please come up with an property name.

@esprehn that's not very constructive. We went through a lot of names, none of them great and lots of them unlikely to be compatible. Given that this method takes an argument I don't think it's that inconsistent. I guess the alternative is that we simply don't provide it and let libraries fill this gap.

Internally we call the top thing a treeScope. You could call this treeScope, scopeRootNode, topTreeNode, treeScopeRoot, ...

Okay, so Google is objecting to this being added as an attribute?

I find it extremely frustrating that representatives from a single company keep saying different things in a cross-vendor discussion, often conflicting their own representatives. With that kind of inconsistencies, I can't decide how serious I need to take any of your feedback since another representative from your company can just disagree with you and veto whatever other person has agreed to in public.

I almost don't see a point in engaging in the discussion if each one of you keep coming back with a different answer on every discussion like this.

So what is the latest here? Copying the ownerSVGElement pattern as ownerRootNode would be another option maybe. I like that better than the various names @esprehn came up with it.

Anyway, if we don't have something by next Friday my plan is to drop the feature from the standard and leave it to JavaScript libraries. It's a little inconvenient, but not too hard to implement yourself either.

@annevk see this and next comment about owner* https://github.com/whatwg/dom/issues/241#issuecomment-217215821

I don't think should use the prefix "owner" since the owner document of a node is not necessary its root node or its shadow-inclusive ancestor node. You can have a detached node and its owner document is well defined. That semantics is quite distinct from the root node of a node tree to which the context object belongs, which is what this property / method is returning.

Right, that is why one would be called ownerDocument and the other ownerRootNode. Note that ownerSVGElement already exists and means something similar to what is proposed here. Anyway, it sounds like we're better of removing this.

So, removing and closing this then?

Yeah, removing it from the spec sounds good to me. After all, this was just nice-to-have. It's not going to (nor should it) block the shadow DOM spec or the rest of DOM spec.

It is unfortunate that we can not have it only because we can not find a good name which does not conflict existing Web sites. :(

Yeah, this is nice-to-have. This is a completely independent feature. However, this feature could make Web faster. We can try to think a better name anytime later...

Sorry for the delay, had forgotten I was going to be out and about at the end of last week.

This is indeed unfortunate, I proposed a long list of names, and instead of choosing one of them we've given developers a O(n) solution instead of the O(1) answer the browser could have given them.

Why can't we use any of those names I listed? This doesn't seem like a good outcome.

Huh! Or we could have picked the one name everyone else (including other representatives from Google) involved in the discussion agreed on earlier: findRootNode().

Now, that I've stated the elephant in the room, let me elaborate why we don't want any of names you listed:

  • treeScope: _scope_ is a bad name for this because what we're finding is really the root of the node tree, not some scope. Also this is confusing with respect to the similarly named concept in CSS.
  • scopeRootNode: See above.
  • topTreeNode: What we're looking for the node that's the root of the tree. Also, someone complained that top usually means the leaf of a tree when I suggested treeTop so the same complaint applies here.
  • treeScopeRoot: Again, having scope is just confusing and treeRoot is too popular.

Yeah, I think it's very hypocritical to come in here, break consensus, and then be upset when nobody chooses the consensus-breaking names that you propose.

I discussed this with Domenic offline. Let's just go back to the method name that as decided upon. That's findRootNode?

That was getRootNode().

@rniwa @hayatoito @nekr et al, still happy with that one?

Yeah, getRootNode() is better than nothing. I am happy with it, and it is worth trying.

Sure, getRootNode() sounds great to me.

Seems like GIthub was done for some time. Yes, I am also okay with getRootNode().

Was this page helpful?
0 / 5 - 0 ratings