Bug 160874 - Web Inspector: Add visual indicator for shadow content in DOM tree
Summary: Web Inspector: Add visual indicator for shadow content in DOM tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-08-15 16:15 PDT by Devin Rousso
Modified: 2016-09-01 15:13 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 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.
Comment 1 Radar WebKit Bug Importer 2016-08-15 16:17:34 PDT
<rdar://problem/27856368>
Comment 2 Devin Rousso 2016-08-15 16:19:34 PDT
Created attachment 286112 [details]
Patch
Comment 3 Devin Rousso 2016-08-15 16:30:22 PDT
Created attachment 286113 [details]
[Image] After Patch is applied
Comment 4 Joseph Pecoraro 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.
Comment 5 Matt Baker 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;
}
Comment 6 Devin Rousso 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...
Comment 7 Devin Rousso 2016-08-15 22:52:43 PDT
Created attachment 286151 [details]
Patch
Comment 8 Joseph Pecoraro 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.
Comment 9 Devin Rousso 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. ¯\_(ツ)_/¯
Comment 10 Devin Rousso 2016-08-16 16:32:33 PDT
Created attachment 286232 [details]
Patch
Comment 11 Brian Burg 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.
Comment 12 Devin Rousso 2016-09-01 14:41:56 PDT
Created attachment 287684 [details]
Patch
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2016-09-01 15:13:44 PDT
All reviewed patches have been landed.  Closing bug.