| Summary: | Web Inspector: Add visual indicator for shadow content in DOM tree | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||
| Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | bburg, commit-queue, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Devin Rousso
2016-08-15 16:15:52 PDT
Created attachment 286112 [details]
Patch
Created attachment 286113 [details]
[Image] After Patch is applied
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. 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;
}
(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... Created attachment 286151 [details]
Patch
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. (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. ¯\_(ツ)_/¯ Created attachment 286232 [details]
Patch
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. Created attachment 287684 [details]
Patch
Comment on attachment 287684 [details] Patch Clearing flags on attachment: 287684 Committed r205322: <http://trac.webkit.org/changeset/205322> All reviewed patches have been landed. Closing bug. |