Summary: Improvements to tab layout and resize behavior. This task covers the following polish items: - Remove Close button (x) from all tabs (except for the Search and New Tab tabs). - When shrinking the bar would cause an overflow, hide tab icons. - When not all TabBarItems can fit in the TabBar, hide tabs as needed starting from the right. - The selected tab cannot be hidden. Note: Since they don't appear in the TabBar's context menu (see https://webkit.org/b/181448), the Search and New Tab tabs have to retain the close button for now. Eventually the New Tab button will be removed, and Search results will become an ephemeral tab (like Settings). At that point we can drop support for the close button in TabBarItem.
<rdar://problem/36395439>
As well as: - When one or more tabs are hidden, add a ">>" TabBarItem that when clicked shows a popup menu with a list of hidden tabs.
<rdar://problem/36395475>
Created attachment 332408 [details] Patch
Created attachment 332410 [details] [Video] Resize and tab picker behavior
Comment on attachment 332408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332408&action=review r=me, with some Style and NIT > Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:57 > + get title() > + { > + return super.title; > + } Is this necessary? > Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:89 > + contextMenu.appendItem(WI.UIString("Close Tab"), () => this._parentTabBar.removeTabBarItem(this), this.isDefaultTab); I think our style is to usually always have the {} for inline functions on ContextMenu items, as we never want to return any value. contextMenu.appendItem(WI.UIString("Close Tab"), () => { this._parentTabBar.removeTabBarItem(this); }, this.isDefaultTab); > Source/WebInspectorUI/UserInterface/Views/TabBar.js:410 > + item.element.classList.toggle("hidden", hidden); One gotcha to keep an eye out for is that if `undefined` is passed as the second argument to `classList.toggle` then it will just flip-flop the class like a lightswitch. You might want to add a `!!` in front of `hidden` so that this can't happen: item.element.classList.toggle("hidden", !!hidden); > Source/WebInspectorUI/UserInterface/Views/TabBar.js:427 > + } Style: missing semicolon > Source/WebInspectorUI/UserInterface/Views/TabBar.js:444 > Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Views/TabBar.js:609 > + contextMenu.appendItem(item.title, () => this.selectedTabBarItem = item); This reads somewhat poorly. I think it would be clearer to not make this one line: for (let item of this._hiddenTabBarItems) { contextMenu.append(item.title, () => { this.selectedTabBarItem = item; }); } Also, does this not need to call `needsLayout()` as it does in `_handleTabPickerTabContextMenu`?
Comment on attachment 332408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332408&action=review >> Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:57 >> + } > > Is this necessary? Overriding a property's setter (or getter) shadows the whole property: class Foo { get x() { return 1; } } class Bar extends Foo { set x() {} } > let o = new Bar; > o.x < undefined >> Source/WebInspectorUI/UserInterface/Views/TabBar.js:410 >> + item.element.classList.toggle("hidden", hidden); > > One gotcha to keep an eye out for is that if `undefined` is passed as the second argument to `classList.toggle` then it will just flip-flop the class like a lightswitch. You might want to add a `!!` in front of `hidden` so that this can't happen: > > item.element.classList.toggle("hidden", !!hidden); In practice the second argument is always defined, but it's a good habit to be in. Will change. >> Source/WebInspectorUI/UserInterface/Views/TabBar.js:609 >> + contextMenu.appendItem(item.title, () => this.selectedTabBarItem = item); > > This reads somewhat poorly. I think it would be clearer to not make this one line: > > for (let item of this._hiddenTabBarItems) { > contextMenu.append(item.title, () => { > this.selectedTabBarItem = item; > }); > } > > Also, does this not need to call `needsLayout()` as it does in `_handleTabPickerTabContextMenu`? The layout call in _handleTabPickerTabContextMenu shouldn't be necessary. Will remove.
Created attachment 332423 [details] Patch for landing
Comment on attachment 332423 [details] Patch for landing Clearing flags on attachment: 332423 Committed r227703: <https://trac.webkit.org/changeset/227703>
All reviewed patches have been landed. Closing bug.