RESOLVED FIXED 181468
Web Inspector: TabBar redesign: improvements to tab layout and resize behavior
https://bugs.webkit.org/show_bug.cgi?id=181468
Summary Web Inspector: TabBar redesign: improvements to tab layout and resize behavior
Matt Baker
Reported 2018-01-09 18:59:14 PST
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.
Attachments
Patch (34.26 KB, patch)
2018-01-26 13:07 PST, Matt Baker
no flags
[Video] Resize and tab picker behavior (2.11 MB, video/mp4)
2018-01-26 13:20 PST, Matt Baker
no flags
Patch for landing (34.25 KB, patch)
2018-01-26 16:13 PST, Matt Baker
no flags
Radar WebKit Bug Importer
Comment 1 2018-01-09 18:59:27 PST
Matt Baker
Comment 2 2018-01-09 19:01:13 PST
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.
Radar WebKit Bug Importer
Comment 3 2018-01-09 19:01:26 PST
Matt Baker
Comment 4 2018-01-26 13:07:07 PST
Matt Baker
Comment 5 2018-01-26 13:20:24 PST
Created attachment 332410 [details] [Video] Resize and tab picker behavior
Devin Rousso
Comment 6 2018-01-26 14:05:05 PST
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`?
Matt Baker
Comment 7 2018-01-26 16:10:27 PST
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.
Matt Baker
Comment 8 2018-01-26 16:13:16 PST
Created attachment 332423 [details] Patch for landing
WebKit Commit Bot
Comment 9 2018-01-26 16:49:16 PST
Comment on attachment 332423 [details] Patch for landing Clearing flags on attachment: 332423 Committed r227703: <https://trac.webkit.org/changeset/227703>
WebKit Commit Bot
Comment 10 2018-01-26 16:49:17 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.