Bug 190388 - Web Inspector: indent all network entries when "Group by Node" is checked
Summary: Web Inspector: indent all network entries when "Group by Node" is checked
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: 189606
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-08 19:14 PDT by Devin Rousso
Modified: 2018-10-10 11:02 PDT (History)
6 users (show)

See Also:


Attachments
[Image] Screenshot of Issue (1.26 MB, image/png)
2018-10-08 19:14 PDT, Devin Rousso
no flags Details
Patch (5.65 KB, patch)
2018-10-08 20:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (565.05 KB, image/png)
2018-10-08 20:35 PDT, Devin Rousso
no flags Details
Archive of layout-test-results from ews202 for win-future (12.72 MB, application/zip)
2018-10-09 04:41 PDT, EWS Watchlist
no flags Details
[Image] Screenshot of Resources (65.85 KB, image/png)
2018-10-09 16:12 PDT, Devin Rousso
no flags Details
Patch (4.41 KB, patch)
2018-10-09 22:54 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 2018-10-08 19:14:05 PDT
It looks weird to have only some of the network entry rows be indented.  It should be more uniform.
Comment 1 Devin Rousso 2018-10-08 19:14:56 PDT
Created attachment 351846 [details]
[Image] Screenshot of Issue
Comment 2 Devin Rousso 2018-10-08 20:35:27 PDT
Created attachment 351850 [details]
Patch
Comment 3 Devin Rousso 2018-10-08 20:35:44 PDT
Created attachment 351851 [details]
[Image] After Patch is applied
Comment 4 Joseph Pecoraro 2018-10-09 00:43:32 PDT
Comment on attachment 351850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351850&action=review

> Source/WebInspectorUI/ChangeLog:13
> +        Hide the "Group by Node" checkbox until there is a node that can be grouped.

Hiding a setting some of the time seems like a bad idea. This is a persistent setting, right?

> Source/WebInspectorUI/ChangeLog:22
> +        Apply a padding to all nodes when the `WI.Table` is grouped.

I do like the idea of only adding padding when grouped. I think the screenshot makes the grouping look much clearer. I wish there was something we could do about the wasted space where most of the disclosure triangles would be. What if we just don't have a disclosure triangle (and don't allow collapsing). Does that look or feel weird?
Comment 5 EWS Watchlist 2018-10-09 04:41:13 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-10-09 04:41:24 PDT Comment hidden (obsolete)
Comment 7 Devin Rousso 2018-10-09 09:51:08 PDT
Comment on attachment 351850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351850&action=review

>> Source/WebInspectorUI/ChangeLog:13
>> +        Hide the "Group by Node" checkbox until there is a node that can be grouped.
> 
> Hiding a setting some of the time seems like a bad idea. This is a persistent setting, right?

It isn't visible until we are able to group by node for the first time.  From that point on (even if you navigate the page), it'll stay visible.  My thinking on this is that it would be weird to show a checkbox that doesn't appear to have any effect (e.g. for a page with no media elements), but I suppose we do this elsewhere too (still showing the pretty print icon for things that are already nicely formatted).  I can remove.

>> Source/WebInspectorUI/ChangeLog:22
>> +        Apply a padding to all nodes when the `WI.Table` is grouped.
> 
> I do like the idea of only adding padding when grouped. I think the screenshot makes the grouping look much clearer. I wish there was something we could do about the wasted space where most of the disclosure triangles would be. What if we just don't have a disclosure triangle (and don't allow collapsing). Does that look or feel weird?

I feel like the disclosure triangle is the "indicator" of the fact that this item is a parent.  It is aggravating to "waste" all this space, especially since most network entries won't be grouped, but we do this all the time (e.g. timelines).  I will, however, do a mockup and see how it looks without the triangle.
Comment 8 Timothy Hatcher 2018-10-09 14:04:44 PDT
This looks better! I do think the indentation of the children needs to be more — the icon should align with the text. Right now it seems like a half indent.

See other places in the Inspector to see what I mean.
Comment 9 Devin Rousso 2018-10-09 16:12:29 PDT
Created attachment 351922 [details]
[Image] Screenshot of Resources

(In reply to Timothy Hatcher from comment #8)
> This looks better! I do think the indentation of the children needs to be
> more — the icon should align with the text. Right now it seems like a half
> indent.
> 
> See other places in the Inspector to see what I mean.
Per our conversation in person, I tried to match this as close as I could with our other `WI.TreeOutline`s.
Comment 10 Devin Rousso 2018-10-09 22:54:38 PDT
Created attachment 351947 [details]
Patch
Comment 11 WebKit Commit Bot 2018-10-10 11:01:34 PDT
Comment on attachment 351947 [details]
Patch

Clearing flags on attachment: 351947

Committed r237006: <https://trac.webkit.org/changeset/237006>
Comment 12 WebKit Commit Bot 2018-10-10 11:01:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-10-10 11:02:47 PDT
<rdar://problem/45165926>