Bug 103214

Summary: [Shadow DOM] Node::isDescendant() should be able to take care of shadow boundary
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: abarth, arv, ojan, webcomponents-bugzilla, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://jsfiddle.net/pjpR8/
Bug Depends on:    
Bug Blocks: 72352    
Attachments:
Description Flags
Patch
none
Patch none

Hajime Morrita
Reported 2012-11-25 20:31:58 PST
Node::contains() isn't aware Shadow DOM but it should, because shadow tree is also a part of the DOM tree.
Attachments
Patch (12.15 KB, patch)
2012-11-26 00:18 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Patch (11.49 KB, patch)
2012-11-28 20:41 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hajime Morrita
Comment 1 2012-11-26 00:18:16 PST
Created attachment 175936 [details] Patch
Erik Arvidsson
Comment 2 2012-11-26 07:05:29 PST
Comment on attachment 175936 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175936&action=review Node::isDescendantOf (and the DOM exposed contains method) MUST only work on the light DOM. > Source/WebCore/ChangeLog:9 > + several cosisntency checks in DOM rely on this. typo > LayoutTests/fast/dom/shadow/node-contains.html:14 > + shouldBeTrue("host.contains(shadow)"); This should be false. contains is defined by DOM spec as Node.prototype.contains = function(otherNode) { if (otherNode === this) return true; if (otherNode === null) return false; return this.contains(otherNode.parentNode); }; and the shadow dom says: The parentNode and parentElement attributes of the shadow root object must always return null. In that way contains should return false.
Adam Barth
Comment 3 2012-11-26 10:50:50 PST
Does this patch affect performance?
Hajime Morrita
Comment 4 2012-11-28 20:41:04 PST
Created attachment 176633 [details] Patch
Hajime Morrita
Comment 5 2012-11-28 20:47:29 PST
Thanks for comments. I updated the patch but it isn't for review yet. I addressed Arv's comment. That means it also addresses Dimitri's suggestion which says we shouldn't change Node::contains() behavior. It has small perf slowdown. I checked dom-modify and html5-full-renderer and they show sub-1% slowdown. I thought http://trac.webkit.org/changeset/136076 can mask it, but then changed my mind that more generic Shadow DOM speedup could minimize this slow down too. I'll try that first then come back here.
Hajime Morrita
Comment 6 2013-01-23 00:42:25 PST
*** This bug has been marked as a duplicate of bug 105066 ***
Note You need to log in before you can comment on or make changes to this bug.