Bug 181468

Summary: Web Inspector: TabBar redesign: improvements to tab layout and resize behavior
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: agomez, commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 181448    
Bug Blocks: 181611    
Attachments:
Description Flags
Patch
none
[Video] Resize and tab picker behavior
none
Patch for landing none

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
DetailsFormatted DiffDiff
[Video] Resize and tab picker behavior (2.11 MB, video/mp4)
2018-01-26 13:20 PST, Matt Baker 		no flags
Details
Patch for landing (34.25 KB, patch)
2018-01-26 16:13 PST, Matt Baker 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-01-09 18:59:27 PST
<rdar://problem/36395439>
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
<rdar://problem/36395475>
Matt Baker
Comment 4 2018-01-26 13:07:07 PST
Created attachment 332408 [details] Patch
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.