compareDocumentPosition with Attr nodes still not quite right

[foolip]

Context:
https://github.com/whatwg/dom/pull/299/files/82df412250bd1321562cbe50574219860b976645#r75397386
#299 (comment)

There was a partial fix in 6ae89ed, but when two Attr nodes attached to elements in the same tree and when you're comparing an Attr node to an element in the same tree it shouldn't do the pointer comparison, but instead behave as if the Attr nodes are children of its element.

@cdumez

[annevk]

In that case node1 and node2 share a root though so there will not be a pointer comparison, right?

[foolip]

The problem is attributes don't participate in a tree, they have no parent and thus no root.

[annevk]

node1/node2 is not an Attr node though. Set to Attr node's element earlier.

[foolip]

But that's still not quite right in the case of comparing an element's attribute to one of its proper children, is it?

[annevk]

It would end up being the same as if the element was compared to its child. So the problem is the addition of DOCUMENT_POSITION_CONTAIN[S|EN_BY].

So would a fix here by that step 7 and 8 also check that attr1 and attr2 are null?

[foolip]

https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4411 illustrates the case I was thinking of, not sure if there are other variations.

As currently written, things first go wrong at step 6, where node1 and node2 may both be attributes in the same document (although as defined they don't have a root), but still the pointer compare happens. Test: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4412

With step 6 fixed somehow, yes, some tweaks to step 7 and 8 ought to do it. I'm not sure exactly what you had in mind, but it'd have to be such that an attribute is still considered contained by its element:
https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4413

[annevk]

Hmm yeah, this is all kinds of wrong. 😟

[foolip]

They can both start out as attributes in step 2, but yeah, if they're attached to elements then node1 and node2 will be reassigned to those elements. So if node1 or node2 are attributes at step 6, then they're not attached. Saying "node1 or node2 are attributes" would handle detached cases, then.

Step 7 or 8 would still need some tweaking, I presume. Since you reverse engineered Gecko, I think that it all follows from "If there is an owner element add the attribute to the chain and walk up to the element" (my emphasis).

[foolip]

(FWIW, Blink sets the equivalent of node1 to the attribute's element, which means it can then be null. It works out to the same thing, it just means that "node1 is null" means the same thing as "node1 is an attribute" as now spec'd.)

[annevk]

Oh wow, Chrome also made all kinds of ShadowRoot special cases. Firefox does not have those.

[annevk]

I think Chrome should remove the ShadowRoot stuff from compareDocumentPosition(). If @hayatoito (or someone else) wants to keep that please file a new issue. I don't think we ever had that discussion.

[hayatoito]

Hmm. I need a help to understand the issue. Could someone explain what is the spec non-compliant behavior in compareDocumentPosition, in terms of ShadowRoot, in Blink? I could not find it in this discussion.

The code is here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.cpp?q=compareDocument&sq=package:chromium&l=1344

I have not touched this code much, but I think authors have updated this function to consider ShadowRoot, without knowing that it would break external API behavior.

[annevk]

If it does not change behavior of the API it's fine. Otherwise it needs to be reworked or we need to agree to change the standard.

[hayatoito]

I see. I think Blink's implementation does not have any intention to change behavior of the Web-facing API.
If it would change, it should be considered as a bug.

[foolip]

The TreatShadowTreesAsComposed case is only used for an internal call site, from scripts one will get TreatShadowTreesAsDisconnected which (presumably) maintains the old behavior.

Still might be worthwhile adding a note about this or at the very least making sure there's something in web-platform-tests ensuring that everyone agrees on how to handle ShadowRoots.