Bug 189164 - Document.p.contains returns true for nodes in shadow tree
Summary: Document.p.contains returns true for nodes in shadow tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2018-08-30 11:18 PDT by Justin Ridgewell
Modified: 2020-09-28 16:48 PDT (History)
5 users (show)

See Also:


Attachments
Document contains test case (269 bytes, text/html)
2018-08-30 11:18 PDT, Justin Ridgewell
no flags Details
Adds a test (3.39 KB, patch)
2020-09-28 16:43 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Ridgewell 2018-08-30 11:18:04 PDT
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.
Comment 1 Ryosuke Niwa 2020-09-26 15:22:49 PDT
This works for me now.
Comment 2 Justin Ridgewell 2020-09-28 10:06:24 PDT
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.
Comment 3 Ryosuke Niwa 2020-09-28 15:26:40 PDT
(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.
Comment 4 Ryosuke Niwa 2020-09-28 15:31:55 PDT
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.
Comment 5 Radar WebKit Bug Importer 2020-09-28 15:32:36 PDT
<rdar://problem/69720558>
Comment 6 Ryosuke Niwa 2020-09-28 15:33:15 PDT
Actually, let's add a test for this.
Comment 7 Ryosuke Niwa 2020-09-28 15:33:29 PDT
Adding a test.
Comment 8 Darin Adler 2020-09-28 15:47:12 PDT
I knew I was fixing a bug. I’m embarrassed that I didn’t add a test case for it.
Comment 9 Darin Adler 2020-09-28 16:03:34 PDT
And really surprised WPT doesn’t already have one!
Comment 10 Ryosuke Niwa 2020-09-28 16:09:54 PDT
(In reply to Darin Adler from comment #9)
> And really surprised WPT doesn’t already have one!

Yeah!
Comment 11 Ryosuke Niwa 2020-09-28 16:43:47 PDT
Created attachment 409929 [details]
Adds a test
Comment 12 Ryosuke Niwa 2020-09-28 16:48:49 PDT
Committed r267719: <https://trac.webkit.org/changeset/267719>