|Summary:||Web Inspector: Canvas tab: hide navigation sidebar when viewing the overview|
|Product:||WebKit||Reporter:||Matt Baker <mattbaker>|
|Component:||Web Inspector||Assignee:||Matt Baker <mattbaker>|
|Severity:||Normal||CC:||commit-queue, drousso, ews-watchlist, inspector-bugzilla-changes, rniwa, webkit-bug-importer|
|Version:||WebKit Nightly Build|
|Bug Depends on:|
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 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 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 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 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.