Bug 178803 - Web Inspector: Canvas Tab: clicking on a canvas card causes details sidebar to show and mess up card arrangement
Summary: Web Inspector: Canvas Tab: clicking on a canvas card causes details sidebar t...
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: WebInspectorCanvasTab
  Show dependency treegraph
 
Reported: 2017-10-25 10:03 PDT by Brian Burg
Modified: 2017-12-04 17:31 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.09 KB, patch)
2017-10-25 13:45 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (297.58 KB, image/png)
2017-10-25 13:47 PDT, Devin Rousso
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-10-25 10:03:16 PDT
It's pretty easy to get in a situation where you want to select a canvas to refresh it, realize you clicked on the wrong one, and then everything moves around because the details sidebar has popped out. This is really jarring.

I think we should just force the details sidebar to always show on the canvas overview. If nothing is selected, then simply add a placeholder "No Canvas Selected" text like the left side has "No Recording Data".
Comment 1 Radar WebKit Bug Importer 2017-10-25 10:03:56 PDT
<rdar://problem/35176082>
Comment 2 Brian Burg 2017-10-25 10:06:36 PDT
It seems the inter-row spacing is a result of the detached inspector being very tall. I think we should stick to a constant inter-row spacing rather than letting it flex larger if the window has more space than needed. If we don't fix this, it will look really weird when the window is tall and narrow, as in the screenshot.
Comment 3 Devin Rousso 2017-10-25 13:45:45 PDT
Created attachment 324886 [details]
Patch
Comment 4 Devin Rousso 2017-10-25 13:47:14 PDT
Created attachment 324887 [details]
[Image] After Patch is applied
Comment 5 Brian Burg 2017-10-25 14:53:34 PDT
Comment on attachment 324886 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:25
> +        Drive-by: these sidebar panels are now only used by the Canvas tab, so we no longer need to

Nice.

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.css:45
> +    padding: 5px 15px 6px;

This ain't gonna work with RTL layout. Can you split out padding and make it flip correctly?

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:-103
> -        this.contentView.element.appendChild(identitySection.element);

Nice cleanup.

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:150
> +        this.contentView.element.append(...this._sections.map(section => section.element));

Slick.
Comment 6 Devin Rousso 2017-10-25 15:01:25 PDT
Comment on attachment 324886 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.css:45
>> +    padding: 5px 15px 6px;
> 
> This ain't gonna work with RTL layout. Can you split out padding and make it flip correctly?

This should be fine with RTL.  This is shorthand for `top right&left bottom`, so both `padding-right` and `padding-left` will have a value of "15px".  Am I mistaken?
Comment 7 WebKit Commit Bot 2017-10-25 16:13:23 PDT
Comment on attachment 324886 [details]
Patch

Clearing flags on attachment: 324886

Committed r223991: <https://trac.webkit.org/changeset/223991>
Comment 8 WebKit Commit Bot 2017-10-25 16:13:24 PDT
All reviewed patches have been landed.  Closing bug.