Bug 189164 – Document.p.contains returns true for nodes in shadow tree

• Component: DOM
• Status: RESOLVED FIXED

[Justin Ridgewell]

Created attachment 348515 [details]
Document contains test case

`Document.p.contains` currently returns true for nodes inside a shadow tree. This incorrect according to the [spec](https://dom.spec.whatwg.org/#dom-node-contains), which uses "inclusive descendant" not "shadow-including inclusive descendant".

Note that `Element.p.contains` always does the right thing for nodes in a shadow tree.

Steps to reproduce:
- Go to https://output.jsbin.com/gasefow/quiet
- Observe "Doc contains node in shadow tree" is written to the page


This report invalidates the bug at https://bugs.webkit.org/show_bug.cgi?id=119371 (which asks for for `Node.p.contains` to  return true).
Further reading at https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141, which argued for the broken `Document.p.contains` behavior. Instead of breaking contains, `Node.p.isConnected` was added in https://github.com/w3c/webcomponents/issues/81.

[Ryosuke Niwa]

This works for me now.

[Justin Ridgewell]

I tested with Safari TP 113, and it still fails. Is there a recent commit that fixes the behavior?

To be clear, we do not want to see any output on the test page, it should be blank. Safari still shows "Doc contains node in shadow tree" for me.

[Ryosuke Niwa]

(In reply to Justin Ridgewell from comment #2)
> I tested with Safari TP 113, and it still fails. Is there a recent commit
> that fixes the behavior?
> 
> To be clear, we do not want to see any output on the test page, it should be
> blank. Safari still shows "Doc contains node in shadow tree" for me.

Oh, weird. I don't know what I was testing but you're right this is definitely broken.

[Ryosuke Niwa]

Ah, okay, I know what happened there. Darin fixed this in https://trac.webkit.org/r267220.

Node::isDescendantOf has a special code for when "other" node is a document node.

Before r267220, we had the following code, which incorrectly assumed that any node which is connected to "other" is a descendent of "other":
    if (other.isDocumentNode())
        return &document() == &other && !isDocumentNode() && isConnected();

After r267220, now correctly checks that the root node of the current tree is same as "other":
    if (other.isDocumentNode())
        return &treeScope().rootNode() == &other && !isDocumentNode() && isConnected();

I expect this bug will be fixed the next STP.

[Radar WebKit Bug Importer]

<rdar://problem/69720558>

[Ryosuke Niwa]

Actually, let's add a test for this.

[Ryosuke Niwa]

Adding a test.

[Darin Adler]

I knew I was fixing a bug. I’m embarrassed that I didn’t add a test case for it.

[Darin Adler]

And really surprised WPT doesn’t already have one!

[Ryosuke Niwa]

(In reply to Darin Adler from comment #9)
> And really surprised WPT doesn’t already have one!

Yeah!

[Ryosuke Niwa]

Created attachment 409929 [details]
Adds a test

[Ryosuke Niwa]

Committed r267719: <https://trac.webkit.org/changeset/267719>