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.
Created attachment 333998 [details]
Created attachment 334000 [details]
(In reply to Matt Baker from comment #3)
> Created attachment 334000 [details]
- 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 on attachment 334000 [details]
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. :(
> + 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`.
Created attachment 334073 [details]
(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 on attachment 334073 [details]
Attachment 334073 [details] did not pass mac-ews (mac):
New failing tests:
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
Created attachment 334089 [details]
Comment on attachment 334089 [details]
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.
> + 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.
Created attachment 334182 [details]
Comment on attachment 334182 [details]
r=me. Thanks for doing these iterations :)
Comment on attachment 334182 [details]
Clearing flags on attachment: 334182
Committed r228722: <https://trac.webkit.org/changeset/228722>
All reviewed patches have been landed. Closing bug.