Bug 60160

Created attachment 92222 [details] screenshot

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.

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.

Created attachment 92227 [details] patch - style fixed

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 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 on attachment 92227 [details] patch This is great! Thanks folks!

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!

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

Cool!

Note: this was removed in bug 62188.

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

(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

Created attachment 120263 [details] Updated patch, no tests

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

Discussion should be moved to bug 78202.