REGRESSION (r195456): Web Inspector: Changing tabs in Styles sidebar shouldn't change sidebar's width

[Nikita Vasilyev]

Created attachment 283347 [details]
[Animated GIF] Bug

Steps:
1. Open Elements tab
2. Select Styles tab in the right sidebar
3. Make the sidebar wider
4. Select the Node tab

Expected:
The sidebar didn't change its width after selecting the Node tab.

Actual:
The sidebar changed its width to the previous width of the Node tab.

[Radar WebKit Bug Importer]

<rdar://problem/27286338>

[Joseph Pecoraro]

+1 I just ran into this the other day and it was annoying.

[Matt Baker]

+1 This was bugging me over the weekend!

[Devin Rousso]

I think this has to do with the fact each of the `detailsSidebarPanels` inside `WebInspector.ElementsTabContentView` have separate `savedWidth` values (which comes froma `WebInspector.Setting` object) via `WebInspector.SidebarPanel`.  Since Nodes, Styles, and Layers are all considered separated sidebars (and this problem may exist anywhere that multiple sidebar panels are used, like the Resource and Scope Chain), the best solution may be to add some logic to `WebInspector.TabContentView` or `WebInspector.Sidebar` (since that is the "controller" that maintains all of the added sidebars),

[Nikita Vasilyev]

This broke in https://trac.webkit.org/changeset/195456/trunk.

[Nikita Vasilyev]

1. Sidebar panels should NOT remember their width, Sidebar should. Node, Styles, and Layers should all have the same width, which isn't currently the case.

2. However, sidebar width should be uniq for each tab. Changing sidebar width in Elements shouldn't change sidebar width in Resources.

If we agree on these two points, I think it makes sense to make sidebar a subview of TabContentView. Currently, sidebars are global (WebInspector.detailsSidebar and WebInspector.navigationSidebar). If we make them local per tab, we can store their width in, say, WebInspector.Sidebar#_widthSetting.

[Timothy Hatcher]

(In reply to comment #6)
> 1. Sidebar panels should NOT remember their width, Sidebar should. Node,
> Styles, and Layers should all have the same width, which isn't currently the
> case.
> 
> 2. However, sidebar width should be uniq for each tab. Changing sidebar
> width in Elements shouldn't change sidebar width in Resources.
> 
> If we agree on these two points, I think it makes sense to make sidebar a
> subview of TabContentView. Currently, sidebars are global
> (WebInspector.detailsSidebar and WebInspector.navigationSidebar). If we make
> them local per tab, we can store their width in, say,
> WebInspector.Sidebar#_widthSetting.

I agree with those points, but Sidebar needs to be global because of the way it lays out in relation to the quick console.

Managing the width changes per tab can be done by TabBrowser.js. It already does things like this when the tab changes:

  this._navigationSidebar.collapsed = tabContentView.navigationSidebarCollapsedSetting.value;

[Nikita Vasilyev]

Created attachment 284633 [details]
WIP

[Nikita Vasilyev]

Created attachment 284645 [details]
Patch

[BJ Burg]

Comment on attachment 284645 [details]
Patch

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

r=me with comments

> Source/WebInspectorUI/ChangeLog:12
> +

Please explain that TabContentView stores the width but TabBrowser manages saving and restoring it. This is weird so we should point it out.

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:268
> +        this.element.style.width = newWidth + "px";

Nit: prefer = `${newWidth}px`. This change does not seem relevant to the patch.

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:276
> +        this.dispatchEventToListeners(WebInspector.Sidebar.Event.WidthDidChange, {newWidth: newWidth});

Nit: just {newWidth} will work.

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:284
> +        var tabContentView = this.selectedTabContentView;

Nit: let

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:340
> +        if (tabContentView.detailsSidebarWidthSetting.value !== 0)

What's up with this conditional? Do we really want to allow this._detailsSidebar.width to be undefined, false, or null? I think you can just drop the !== 0.

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:181
> +    get detailsSidebarWidthSetting()

This seems awkward IMO, but I don't think we are going to change it now. Carry on.

[Nikita Vasilyev]

Created attachment 284879 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284879 [details]
Patch

Clearing flags on attachment: 284879

Committed r203912: <http://trac.webkit.org/changeset/203912>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.