Bug 189164

Summary: Document.p.contains returns true for nodes in shadow tree
Product: WebKit Reporter: Justin Ridgewell <jridgewell>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, jridgewell, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=216656
Bug Depends on:    
Bug Blocks: 148695    
Attachments:
Description Flags
Document contains test case
none
Adds a test darin: review+

Justin Ridgewell
Reported 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.
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+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2020-09-26 15:22:49 PDT
This works for me now.
Justin Ridgewell
Comment 2 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.
Ryosuke Niwa
Comment 3 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.
Ryosuke Niwa
Comment 4 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.
Radar WebKit Bug Importer
Comment 5 2020-09-28 15:32:36 PDT
<rdar://problem/69720558>
Ryosuke Niwa
Comment 6 2020-09-28 15:33:15 PDT
Actually, let's add a test for this.
Ryosuke Niwa
Comment 7 2020-09-28 15:33:29 PDT
Adding a test.
Darin Adler
Comment 8 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.
Darin Adler
Comment 9 2020-09-28 16:03:34 PDT
And really surprised WPT doesn’t already have one!
Ryosuke Niwa
Comment 10 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!
Ryosuke Niwa
Comment 11 2020-09-28 16:43:47 PDT
Created attachment 409929 [details] Adds a test
Ryosuke Niwa
Comment 12 2020-09-28 16:48:49 PDT
Committed r267719: <https://trac.webkit.org/changeset/267719>
Note You need to log in before you can comment on or make changes to this bug.