WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
[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
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug