Bug 181805 - Web Inspector: Layers tab should do away with popovers (if possible)
Summary: Web Inspector: Layers tab should do away with popovers (if possible)
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Ross Kirsling
Keywords: InRadar
Depends on:
Blocks: 174176
  Show dependency treegraph
Reported: 2018-01-18 10:56 PST by Ross Kirsling
Modified: 2018-01-19 15:20 PST (History)
4 users (show)

See Also:

Screenshot (554.66 KB, image/png)
2018-01-18 18:00 PST, Ross Kirsling
no flags Details
Patch (24.27 KB, patch)
2018-01-18 19:21 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
[Image] Layer overlay at narrow width (215.49 KB, image/png)
2018-01-19 12:58 PST, Matt Baker
no flags Details
Patch (25.12 KB, patch)
2018-01-19 14:44 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2018-01-18 10:56:10 PST
As Matt Baker (among others) has pointed out, the Layers tab's details sidebar has a bit of an identity crisis:
- Part of its usage allows one to navigate the visualization, but the tabular data means we can't make it a nav sidebar.
- It *does* display all the details for the visualization, yet it's not "optional" like other details sidebars, because the data for the selected layer is buried in its popover.

Eliminate the popover and display the selected layer information as part of the main panel.

Specifically, keep the same data arrangement but display it in a div that sits on top of the visualization canvas at the top left corner.
(Note that putting it *in* the visualization would be counterproductive, since we don't want it to move around in 3D space.)

If we do this, I think the awkwardness of the details sidebar show more or less go away.
Comment 1 Ross Kirsling 2018-01-18 18:00:53 PST
Created attachment 331682 [details]
Comment 2 Ross Kirsling 2018-01-18 19:21:30 PST
Created attachment 331695 [details]
Comment 3 Ross Kirsling 2018-01-18 19:23:38 PST
Comment on attachment 331695 [details]

No doubt the styling could be improved, but I think this feels a lot better when using it.
Comment 4 Matt Baker 2018-01-19 12:54:54 PST
Comment on attachment 331695 [details]

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

r- for now. Overall this looks good. Most of my comments are style nits, however I did notice that at narrow widths, the property values are not vertically aligned as I'd expect. See the following screenshot.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:40
> +}

I think it would be cleaner to remove this class, as well as `display: none` from .layer-info above, and instead toggle the global `hidden` style on the div.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:43
> +    font-size: 1.1em;

We usually specify font-size in `px`. It's about 12px, which is a common size used for title/heading type things across the UI.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:52
> +    line-height: 1.3em;

Convert to `px`.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:60
> +    -webkit-padding-start: 1em;


> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:64
> +    line-height: 1.3em;


> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:391
> +        reasonsTitle.textContent = WI.UIString("Reasons for compositing:");

Remove the trailing colon. "Dimensions" doesn't have one. This panel is like a floating details section, which doesn't include a colon after its title.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:403
> +            this._layerInfoElement.classList.remove("showing");

See comment in Layers3DContentView.css.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:409
> +        let visibleValue = this._layerInfoElement.querySelector(".dimensions-visible");

Instead of querying, these elements can be member variables set by _buildLayerInfoElement() at creation time.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:416
> +            this._layerInfoElement.classList.add("showing");


(see previous comments)

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:422
> +        let reasonsList = this._layerInfoElement.querySelector(".reasons");

Use a member variable.
Comment 5 Matt Baker 2018-01-19 12:58:39 PST
Created attachment 331774 [details]
[Image] Layer overlay at narrow width

Adjust vertical alignment of the property names so that they line up with the values when the values wrap.
Comment 6 Ross Kirsling 2018-01-19 14:44:04 PST
Created attachment 331790 [details]
Comment 7 Matt Baker 2018-01-19 14:52:07 PST
Comment on attachment 331790 [details]

Comment 8 WebKit Commit Bot 2018-01-19 15:18:48 PST
Comment on attachment 331790 [details]

Clearing flags on attachment: 331790

Committed r227244: <https://trac.webkit.org/changeset/227244>
Comment 9 WebKit Commit Bot 2018-01-19 15:18:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-01-19 15:20:01 PST