Bug 181468 – Web Inspector: TabBar redesign: improvements to tab layout and resize behavior

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

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.

[Radar WebKit Bug Importer]

<rdar://problem/36395439>

[Matt Baker]

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]

<rdar://problem/36395475>

[Matt Baker]

Created attachment 332408 [details]
Patch

[Matt Baker]

Created attachment 332410 [details]
[Video] Resize and tab picker behavior

[Devin Rousso]

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 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]

Created attachment 332423 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 332423 [details]
Patch for landing

Clearing flags on attachment: 332423

Committed r227703: <https://trac.webkit.org/changeset/227703>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.