Bug 60160 - Web Inspector: expose shadow DOM in the Elements panel
Summary: Web Inspector: expose shadow DOM in the Elements panel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-04 05:45 PDT by Andrey Kosyakov
Modified: 2012-02-08 21:49 PST (History)
18 users (show)

See Also:


Attachments
screenshot (16.57 KB, image/png)
2011-05-04 05:46 PDT, Andrey Kosyakov
no flags Details
patch (29.75 KB, patch)
2011-05-04 05:57 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (29.86 KB, patch)
2011-05-04 06:08 PDT, Andrey Kosyakov
yurys: review+
Details | Formatted Diff | Diff
Updated patch, no tests (19.02 KB, patch)
2011-12-21 20:42 PST, Silvia Pfeiffer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2011-05-04 05:45:44 PDT
This adds a pseudo-child called "(shadow)" thats hosts the shadow subtree of a DOM element (if present).
Comment 1 Andrey Kosyakov 2011-05-04 05:46:25 PDT
Created attachment 92222 [details]
screenshot
Comment 2 Andrey Kosyakov 2011-05-04 05:57:58 PDT
Created attachment 92223 [details]
patch

Comments on the ways to visually distinguish shadow DOM elements are welcome. So far, these are "opacity: 0.7". Pavel suggests we use a different background for the entire block, but I don't find this attractive.
Comment 3 Eric Seidel (no email) 2011-05-04 05:59:21 PDT
Attachment 92223 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/front-end/ElementsPanel.js:664:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/InspectorDOMAgent.cpp:1268:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/inspector/InspectorDOMAgent.cpp:1420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/inspector/InspectorDOMAgent.cpp:1421:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/inspector/InspectorDOMAgent.cpp:1421:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/inspector/InspectorDOMAgent.cpp:1422:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/inspector/InspectorDOMAgent.cpp:1423:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:570:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:571:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:572:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:573:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:574:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:575:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:576:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:577:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:578:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1386:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 17 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Andrey Kosyakov 2011-05-04 06:08:21 PDT
Created attachment 92227 [details]
patch

- style fixed
Comment 5 Yury Semikhatsky 2011-05-04 06:28:53 PDT
Comment on attachment 92223 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92223&action=review

> LayoutTests/inspector/elements/shadow-dom-expected.txt:20
> +        - <div>

Would be nice if the test used our own element with simpler shadow tree and would depend on the internal structure of <video> element.

> Source/WebCore/dom/Element.cpp:1188
> +    InspectorInstrumentation::willInsertDOMNode(document(), newRoot.get(), this);

Should it be willInsertShadowRoot?

> Source/WebCore/inspector/front-end/DOMAgent.js:321
> +            child._inShadowTree = this._inShadowTree;

This way all child nodes including non-shadow ones will be considered as shadow elements. We should support the case when an element with shadow tree contains non-shadow elements.

>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:570
>> +	    // If we ever had shadow root child, it's going to be second to last (before closing tag)
> 
> Line contains tab character.  [whitespace/tab] [5]

Why is this? Can we check for the shadow root in a more clear way?

>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:571
>> +	    var shadowRootChild = treeElement.children[treeElement.children.length - 2];
> 
> Line contains tab character.  [whitespace/tab] [5]

This will throw if children.length < 2.
Comment 6 Andrey Kosyakov 2011-05-04 06:45:34 PDT
Comment on attachment 92223 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92223&action=review

>> LayoutTests/inspector/elements/shadow-dom-expected.txt:20
>> +        - <div>
> 
> Would be nice if the test used our own element with simpler shadow tree and would depend on the internal structure of <video> element.

We can't do that unless we do it in DumpRenderTree -- shadow DOM is not accessible from JavaScript. I don't think this is worth the hassle, given this would have to be implemented for multiple platforms.

>> Source/WebCore/dom/Element.cpp:1188
>> +    InspectorInstrumentation::willInsertDOMNode(document(), newRoot.get(), this);
> 
> Should it be willInsertShadowRoot?

It _may_ -- I was just trying to keep instrumentation interface narrow. That is, I don't think we have to distinguish between adding a new node as a child or as a shadow root here. I can add a separate method, though, if you think it's worth it.

>> Source/WebCore/inspector/front-end/DOMAgent.js:321
>> +            child._inShadowTree = this._inShadowTree;
> 
> This way all child nodes including non-shadow ones will be considered as shadow elements. We should support the case when an element with shadow tree contains non-shadow elements.

That's the intent, everything under shadow root belongs to shadow tree, there's no elements exposed via normal DOM interfaces under shadow root. This property in particular mirrors Node::inShadowTree() (we need this to visually distinguish entire contents of the shadow tree in the UI)

>>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:570
>>> +	    // If we ever had shadow root child, it's going to be second to last (before closing tag)
>> 
>> Line contains tab character.  [whitespace/tab] [5]
> 
> Why is this? Can we check for the shadow root in a more clear way?

The intent here is to find a shadow root node within the existing children. We can probably scan the children (see how it happens above within the loop in updateChildrenOfNode()), but I don't think it's going to be any more clear (but definitely less efficient). Alternatively, we can store a separate reference to a particular child, but this way we should be careful to drop it when children are removed. I agree this code stinks, but any alternatives I see don't seem to be much better :)

>>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:571
>>> +	    var shadowRootChild = treeElement.children[treeElement.children.length - 2];
>> 
>> Line contains tab character.  [whitespace/tab] [5]
> 
> This will throw if children.length < 2.

Nope, it's JavaScript. ([])[-1] === undefined
Comment 7 Dimitri Glazkov (Google) 2011-05-04 06:47:19 PDT
Comment on attachment 92227 [details]
patch

This is great! Thanks folks!
Comment 8 Andrey Kosyakov 2011-05-04 06:50:29 PDT
Comment on attachment 92223 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92223&action=review

>>> LayoutTests/inspector/elements/shadow-dom-expected.txt:20
>>> +        - <div>
>> 
>> Would be nice if the test used our own element with simpler shadow tree and would depend on the internal structure of <video> element.
> 
> We can't do that unless we do it in DumpRenderTree -- shadow DOM is not accessible from JavaScript. I don't think this is worth the hassle, given this would have to be implemented for multiple platforms.

Actually, I just realized that we already expose LayoutTestController.ensureShadowRoot() -- so I'll get rid of <video> here and will switch to a manually build shadow tree. Thanks for the idea!
Comment 9 Andrey Kosyakov 2011-05-04 08:08:47 PDT
Manually committed r85751: http://trac.webkit.org/changeset/85751
Comment 10 Joseph Pecoraro 2011-05-04 10:58:15 PDT
Cool!
Comment 11 Silvia Pfeiffer 2011-12-21 19:47:02 PST
Note: this was removed in bug 62188.
Comment 12 Silvia Pfeiffer 2011-12-21 19:48:37 PST
I needed to debug shadow dom in chrome, so I have recreated this patch for current TOT chrome.
Comment 13 Michael[tm] Smith 2011-12-21 20:06:08 PST
(In reply to comment #12)
> I needed to debug shadow dom in chrome, so I have recreated this patch for current TOT chrome.

Seems like the discussion on this should move over to bug 62220
Comment 14 Silvia Pfeiffer 2011-12-21 20:42:23 PST
Created attachment 120263 [details]
Updated patch, no tests
Comment 15 Silvia Pfeiffer 2011-12-21 20:44:36 PST
(In reply to comment #13)
> Seems like the discussion on this should move over to bug 62220

I didn't include the tests from the previous patches, so would like to keep it here, so somebody can take this code, add tests ...
Comment 16 Dominic Cooney 2012-02-08 21:49:58 PST
Discussion should be moved to bug 78202.