160874 – Web Inspector: Add visual indicator for shadow content in DOM tree

RESOLVED FIXED 160874

Web Inspector: Add visual indicator for shadow content in DOM tree

https://bugs.webkit.org/show_bug.cgi?id=160874

Summary Web Inspector: Add visual indicator for shadow content in DOM tree

Devin Rousso

Reported 2016-08-15 16:15:52 PDT

It would be nice to be able to quickly glance at the DOM tree and determine what nodes are shadow content.

Attachments
Patch (4.63 KB, patch) 2016-08-15 16:19 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
[Image] After Patch is applied (121.17 KB, image/png) 2016-08-15 16:30 PDT, Devin Rousso	no flags	Details
[Image] Highlighted node, text color (159.50 KB, image/png) 2016-08-15 19:41 PDT, Matt Baker	no flags	Details
Patch (4.81 KB, patch) 2016-08-15 22:52 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (4.83 KB, patch) 2016-08-16 16:32 PDT, Devin Rousso	bburg: review+	Details Formatted Diff Diff
Patch (4.44 KB, patch) 2016-09-01 14:41 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2016-08-15 16:17:34 PDT

<rdar://problem/27856368>

Devin Rousso

Comment 2 2016-08-15 16:19:34 PDT

Created attachment 286112 [details] Patch

Devin Rousso

Comment 3 2016-08-15 16:30:22 PDT

Created attachment 286113 [details] [Image] After Patch is applied

Joseph Pecoraro

Comment 4 2016-08-15 19:30:01 PDT

Comment on attachment 286112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286112&action=review Neat! r=me, but what does it look like if you also include the Shadow Root in the colored block? Seems that might be better. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1216 > - var fragmentElement = info.titleDOM.createChild("span", "html-fragment"); > + let fragmentElement = info.titleDOM.createChild("span", "html-fragment"); We should not use `let` inside of switch cases. It doesn't do what you expect and is likely to produce errors. Stick with `var`. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:195 > +.tree-outline.dom li.parent.shadow + ol.children.expanded { > + background-color: hsla(0, 0%, 90%, 0.5); > +} Does this meant that multiple nested Shadow DOMs could get progressively darker and darker, or no? If it gets progressively darker, that may be a problem of deeply nested shadow DOMs.

Matt Baker

Comment 5 2016-08-15 19:41:48 PDT

Created attachment 286138 [details] [Image] Highlighted node, text color Cool! It would be nice to fix the color of the Shadow Content text when the node is highlighted (see screenshot). Just need the following in DOMTreeOutline.css: .tree-outline.dom li.parent.shadow.selected { color: white; }

Devin Rousso

Comment 6 2016-08-15 22:09:19 PDT

(In reply to comment #4) > Comment on attachment 286112 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286112&action=review > > Neat! r=me, but what does it look like if you also include the Shadow Root > in the colored block? Seems that might be better. I actually meant to do that. Whoops ¯\_(ツ)_/¯ > We should not use `let` inside of switch cases. It doesn't do what you > expect and is likely to produce errors. Stick with `var`. I learned something new today. That seems like it wasn't really thought through... > Does this meant that multiple nested Shadow DOMs could get progressively > darker and darker, or no? If it gets progressively darker, that may be a > problem of deeply nested shadow DOMs. Yeah that was the idea. I figured that it would be explicitly visible as to where each #shadow began and ended. It would take a lot of shadow DOM to get to that point though...

Devin Rousso

Comment 7 2016-08-15 22:52:43 PDT

Created attachment 286151 [details] Patch

Joseph Pecoraro

Comment 8 2016-08-16 12:15:04 PDT

Comment on attachment 286151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286151&action=review I don't see this fixing the selected text styling issue you point out. Wouldn't that be good to fix with this change? > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:31 > - padding: 0 6px; > - margin: 0; > min-width: 100%; > + margin: 0; > + padding: 0; Hmm, does moving this padding to the <li> affect DOM nodes in the Console? js> document.body <body ...> > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:108 > - padding: 0 0 0 17px; > + padding: 0 6px 0 17px; This as well. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:197 > + width: calc(100% - 15px); What is the 15px for? Is this for some padding on the left edge? Could this be solved by `left: 15px` instead? Maybe this deserves a comment since it is not obvious.

Devin Rousso

Comment 9 2016-08-16 16:29:52 PDT

(In reply to comment #8) > Comment on attachment 286151 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286151&action=review > > I don't see this fixing the selected text styling issue you point out. > Wouldn't that be good to fix with this change? This does actually fix the color change. `.tree-outline.dom:focus li.selected` has a higher specificity than `.tree-outline.dom .shadow`. (☞ﾟヮﾟ)☞ > Hmm, does moving this padding to the <li> affect DOM nodes in the Console? AFAICT, it doesn't. > > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:197 > > + width: calc(100% - 15px); > > What is the 15px for? Is this for some padding on the left edge? Could this > be solved by `left: 15px` instead? Maybe this deserves a comment since it is > not obvious. This is to add some padding on the right edge. Not really sure why it's necessary, but I couldn't get it to work without it. ¯\_(ツ)_/¯

Devin Rousso

Comment 10 2016-08-16 16:32:33 PDT

Created attachment 286232 [details] Patch

Blaze Burg

Comment 11 2016-09-01 10:57:59 PDT

Comment on attachment 286232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286232&action=review r=me > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:85 > - list-style-type: none; > - padding-left: 14px; > margin: 0; > + padding-left: 14px; This chunk seems to be a no-op. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:193 > +/* Cannot apply styling directly to the parent element since it has a disclosure triangle */ Please terminate comments with a period. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:196 > + /* Adds padding to the right edge */ Ditto.

Devin Rousso

Comment 12 2016-09-01 14:41:56 PDT

Created attachment 287684 [details] Patch

WebKit Commit Bot

Comment 13 2016-09-01 15:12:56 PDT

Comment on attachment 287684 [details] Patch Clearing flags on attachment: 287684 Committed r205322: <http://trac.webkit.org/changeset/205322>

WebKit Commit Bot

Comment 14 2016-09-01 15:13:44 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Devin Rousso

Reported

2016-08-15 16:15 PDT

Modified

2016-09-01 15:13 PDT History

CC List

7 users Show

URL

Keywords InRadar

Depends on

Blocks