Bug 181805 – Web Inspector: Layers tab should do away with popovers (if possible)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Ross Kirsling]

Background
----------
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.

Proposal
--------
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.

[Ross Kirsling]

Created attachment 331682 [details]
Screenshot

[Ross Kirsling]

Created attachment 331695 [details]
Patch

[Ross Kirsling]

Comment on attachment 331695 [details]
Patch

No doubt the styling could be improved, but I think this feels a lot better when using it.

[Matt Baker]

Comment on attachment 331695 [details]
Patch

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;

Ditto.

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

Ditto.

> 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");

this._layerInfoElement.classList.remove("hidden");

(see previous comments)

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

Use a member variable.

[Matt Baker]

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.

[Ross Kirsling]

Created attachment 331790 [details]
Patch

[Matt Baker]

Comment on attachment 331790 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 331790 [details]
Patch

Clearing flags on attachment: 331790

Committed r227244: <https://trac.webkit.org/changeset/227244>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36673083>