Bug 182597

Summary: Web Inspector: Canvas tab: hide navigation sidebar when viewing the overview
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, ews-watchlist, inspector-bugzilla-changes, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 175485    
Attachments:
Description Flags
WIP
none
WIP
none
WIP 2
none
Archive of layout-test-results from ews103 for mac-sierra
none
Patch
none
Patch none

Description Matt Baker 2018-02-07 22:28:28 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-02-07 22:31:09 PST
<rdar://problem/37341564>
Comment 2 Matt Baker 2018-02-15 21:40:17 PST
Created attachment 333998 [details]
WIP
Comment 3 Matt Baker 2018-02-15 21:47:19 PST
Created attachment 334000 [details]
WIP
Comment 4 Matt Baker 2018-02-15 21:51:02 PST
(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
Comment 5 Devin Rousso 2018-02-16 11:33:38 PST
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`.
Comment 6 Matt Baker 2018-02-16 14:46:15 PST
Created attachment 334073 [details]
WIP 2
Comment 7 Matt Baker 2018-02-16 14:49:55 PST
(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.
Comment 8 EWS Watchlist 2018-02-16 15:46:46 PST
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
Comment 9 EWS Watchlist 2018-02-16 15:46:47 PST
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
Comment 10 Matt Baker 2018-02-16 16:25:26 PST
Created attachment 334089 [details]
Patch
Comment 11 Devin Rousso 2018-02-16 17:02:58 PST
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.
Comment 12 Matt Baker 2018-02-19 13:38:46 PST
Created attachment 334182 [details]
Patch
Comment 13 Devin Rousso 2018-02-19 18:00:05 PST
Comment on attachment 334182 [details]
Patch

r=me.  Thanks for doing these iterations :)
Comment 14 WebKit Commit Bot 2018-02-19 18:04:36 PST
Comment on attachment 334182 [details]
Patch

Clearing flags on attachment: 334182

Committed r228722: <https://trac.webkit.org/changeset/228722>
Comment 15 WebKit Commit Bot 2018-02-19 18:04:37 PST
All reviewed patches have been landed.  Closing bug.