Bug 60160 – Web Inspector: expose shadow DOM in the Elements panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

This adds a pseudo-child called "(shadow)" thats hosts the shadow subtree of a DOM element (if present).

[Andrey Kosyakov]

Created attachment 92222 [details]
screenshot

[Andrey Kosyakov]

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.

[Eric Seidel (no email)]

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.

[Andrey Kosyakov]

Created attachment 92227 [details]
patch

- style fixed

[Yury Semikhatsky]

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.

[Andrey Kosyakov]

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

[Dimitri Glazkov (Google)]

Comment on attachment 92227 [details]
patch

This is great! Thanks folks!

[Andrey Kosyakov]

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!

[Andrey Kosyakov]

Manually committed r85751: http://trac.webkit.org/changeset/85751

[Joseph Pecoraro]

Cool!

[Silvia Pfeiffer]

Note: this was removed in bug 62188.

[Silvia Pfeiffer]

I needed to debug shadow dom in chrome, so I have recreated this patch for current TOT chrome.

[sideshowbarker]

(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

[Silvia Pfeiffer]

Created attachment 120263 [details]
Updated patch, no tests

[Silvia Pfeiffer]

(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 ...

[Dominic Cooney]

Discussion should be moved to bug 78202.