Bug 182597 – Web Inspector: Canvas tab: hide navigation sidebar when viewing the overview

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

[Matt Baker]

Summary:
Hide navigation sidebar when viewing the overview.

This will require support for conditionally disabling the navigation sidebar based on the current represented object. The idea is similar to what the details sidebar already does, so there may be an opportunity to share common logic.

[Radar WebKit Bug Importer]

<rdar://problem/37341564>

[Matt Baker]

Created attachment 333998 [details]
WIP

[Matt Baker]

Created attachment 334000 [details]
WIP

[Matt Baker]

(In reply to Matt Baker from comment #3)
> Created attachment 334000 [details]
> WIP

Remaining issues:
- Sidebar is still visible when the Inspector opens to the Canvas tab
- Sidebar collapsed setting should be honored when re-enabling the sidebar
- Might need to push logic into TabBrowser for keyboard shortcuts to work

[Devin Rousso]

Comment on attachment 334000 [details]
WIP

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

This looks good!

(In reply to Matt Baker from comment #4)
> Remaining issues:
> - Sidebar is still visible when the Inspector opens to the Canvas tab

This actually appears to be working just fine for me.  When I open WebInspector with the Canvas tab previously selected, I don't see the sidebar at all (it's also disabled).

> - Sidebar collapsed setting should be honored when re-enabling the sidebar

The main issue here is that `set collapsed` fires `WI.Sidebar.Event.CollapsedStateDidChange`, which calls through to 
`WI.TabBrowser.prototype._sidebarCollapsedStateDidChange`, and in there the WI.Setting actually gets changed.  We still want all the other functionality, but we just don't want that value to change.

> - Might need to push logic into TabBrowser for keyboard shortcuts to work

Yeah, the shortcut still works even if the button is disabled.  :(

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:44
> +        this._contentBrowser.addEventListener(WI.ContentBrowser.Event.CurrentRepresentedObjectsDidChange, this._currentRepresentedObjectsDidChange, this);

I think we should follow the naming convention established on the following line and use `_contentBrowserCurrentRepresentedObjectsDidChange`.

[Matt Baker]

Created attachment 334073 [details]
WIP 2

[Matt Baker]

(In reply to Matt Baker from comment #6)
> Created attachment 334073 [details]
> WIP 2

In this patch, the sidebar logic is shared between the TabBrowser and ContentBrowserTabContentView, mirroring the approach taken for DetailsSidebarPanels. This fixes:
- Sidebar being shown when Inspector opens to the Canvas tab
- Disabled sidebar being expandable using the keyboard shortcut

Collapsed state management is improved, but not perfect.

[EWS Watchlist]

Comment on attachment 334073 [details]
WIP 2

Attachment 334073 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6543448

New failing tests:
http/tests/security/http-0.9/xhr-blocked.html

[EWS Watchlist]

Created attachment 334083 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Matt Baker]

Created attachment 334089 [details]
Patch

[Devin Rousso]

Comment on attachment 334089 [details]
Patch

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

Other than the one issue, this looks great.  I'd rather not r+ until it's fixed.

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:324
> +        if (event.target === this._navigationSidebar && !tabContentView.managesNavigationSidebarPanel)

By doing this, we don't save the collapsed state of the NavigationSidebar whenever we show/hide it manually via the button.  This means that the collapsed state will never change from what it was before this change.  I think we only want to apply this logic in the event that the sidebar is collapsed via code, not by the user, possibly by editing WI.toggleNavigationSidebar (and maybe WI.toggleDetailsSidebar too) to set a boolean or to manually set the setting value.

[Matt Baker]

Created attachment 334182 [details]
Patch

[Devin Rousso]

Comment on attachment 334182 [details]
Patch

r=me.  Thanks for doing these iterations :)

[WebKit Commit Bot]

Comment on attachment 334182 [details]
Patch

Clearing flags on attachment: 334182

Committed r228722: <https://trac.webkit.org/changeset/228722>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.