* SUMMARY Closing the final inspector tab should be allowed. In this situation we should open the "New Tab" tab view. * STEPS TO REPRODUCE 1. Open inspector 2. Close all non-NewTab tabs => expected to be able to, but was not able to close last tab
<rdar://problem/22677259>
Created attachment 261131 [details] [PATCH] Proposed Fix 1. When opening inspector with no tabs => Show New Tab tab (no animation) 2. When closing all (non-New Tab) tabs => Show New Tab tab (no animation) 3. When clicking the New Tab button => Show New Tab tab (animation)
Created attachment 261132 [details] [VIDEO] Default Tab behavior
Comment on attachment 261131 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review > Source/WebInspectorUI/UserInterface/Base/Main.js:476 > + const animate = true; NIT: Are you creating this new const for the sake of clarity? > Source/WebInspectorUI/UserInterface/Base/Main.js:485 > +WebInspector.showNewTabTab = function(animate) NIT: It might make more sense to call this "shouldAnimate" > Source/WebInspectorUI/UserInterface/Views/TabBar.js:224 > + var shouldOpenDefaultTab = !tabBarItem.isDefaultTab && !this.hasNormalTab(); I think you could use const here instead of var
Comment on attachment 261131 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review The approach is good but there's one case where it doesn't work. > Source/WebInspectorUI/ChangeLog:36 > + Update styles to only disable the close button on the This is not sufficient, now that middle-click can close the tab. I think you will need to set pointer-events:none or something like that. >> Source/WebInspectorUI/UserInterface/Base/Main.js:485 >> +WebInspector.showNewTabTab = function(animate) > > NIT: It might make more sense to call this "shouldAnimate" +1, i think we should codify this in the style guide. > Source/WebInspectorUI/UserInterface/Views/TabBar.css:168 > + opacity: 0; See note in the changelog. r- since this patch will allow showing no tabs at all on TOT. > Source/WebInspectorUI/UserInterface/Views/TabBar.js:445 > + for (let tabBarItem of this._tabBarItems) { return this._tabBarItems.some((tab) => tab.pinned); > Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:-124 > - return this._element.classList.contains("hide-close-button"); It feels weird to me that we need to query CSS to tell if it's a default tab. Maybe store a flag, or ask the tab bar which is its default?
Comment on attachment 261131 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review > Source/WebInspectorUI/UserInterface/Views/TabBar.css:173 > +.tab-bar.single-tab > .item.default-tab > .close { > pointer-events: none; > } This is the code that disallows pointer-events. So I think it should work with the middle click case.
Comment on attachment 261131 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review >> Source/WebInspectorUI/UserInterface/Views/TabBar.css:173 >> } > > This is the code that disallows pointer-events. So I think it should work with the middle click case. Oh, this is only on the .close, and middle-click works entirely on the tab. I should be able to test after updating.
Comment on attachment 261131 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review >> Source/WebInspectorUI/UserInterface/Base/Main.js:476 >> + const animate = true; > > NIT: Are you creating this new const for the sake of clarity? Yes. We've been doing this in a few places. Here it made sense. >>> Source/WebInspectorUI/UserInterface/Base/Main.js:485 >>> +WebInspector.showNewTabTab = function(animate) >> >> NIT: It might make more sense to call this "shouldAnimate" > > +1, i think we should codify this in the style guide. Sounds good. >> Source/WebInspectorUI/UserInterface/Views/TabBar.js:445 >> + for (let tabBarItem of this._tabBarItems) { > > return this._tabBarItems.some((tab) => tab.pinned); !tab.pinned. Very nice indeed! >> Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:-124 >> - return this._element.classList.contains("hide-close-button"); > > It feels weird to me that we need to query CSS to tell if it's a default tab. Maybe store a flag, or ask the tab bar which is its default? This is a pretty common pattern for us to store boolean View states in classList.
Created attachment 261215 [details] [PATCH] Proposed Fix
Comment on attachment 261215 [details] [PATCH] Proposed Fix r=me
Comment on attachment 261215 [details] [PATCH] Proposed Fix Clearing flags on attachment: 261215 Committed r189815: <http://trac.webkit.org/changeset/189815>
All reviewed patches have been landed. Closing bug.