Web Inspector: Closing the final inspector tab should be allowed

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Closing the final inspector tab should be allowed.

In this situation we should open the "New Tab" tab view.

* STEPS TO REPRODUCE
1. Open inspector
2. Close all non-NewTab tabs
  => expected to be able to, but was not able to close last tab

[Joseph Pecoraro]

<rdar://problem/22677259>

[Joseph Pecoraro]

Created attachment 261131 [details]
[PATCH] Proposed Fix

1. When opening inspector with no tabs
  => Show New Tab tab (no animation)

2. When closing all (non-New Tab) tabs
  => Show New Tab tab (no animation)

3. When clicking the New Tab button
  => Show New Tab tab (animation)

[Joseph Pecoraro]

Created attachment 261132 [details]
[VIDEO] Default Tab behavior

[Devin Rousso]

Comment on attachment 261131 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review

> Source/WebInspectorUI/UserInterface/Base/Main.js:476
> +    const animate = true;

NIT: Are you creating this new const for the sake of clarity?

> Source/WebInspectorUI/UserInterface/Base/Main.js:485
> +WebInspector.showNewTabTab = function(animate)

NIT: It might make more sense to call this "shouldAnimate"

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:224
> +        var shouldOpenDefaultTab = !tabBarItem.isDefaultTab && !this.hasNormalTab();

I think you could use const here instead of var

[BJ Burg]

Comment on attachment 261131 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review

The approach is good but there's one case where it doesn't work.

> Source/WebInspectorUI/ChangeLog:36
> +        Update styles to only disable the close button on the

This is not sufficient, now that middle-click can close the tab. I think you will need to set pointer-events:none or something like that.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:485
>> +WebInspector.showNewTabTab = function(animate)
> 
> NIT: It might make more sense to call this "shouldAnimate"

+1, i think we should codify this in the style guide.

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:168
> +    opacity: 0;

See note in the changelog. r- since this patch will allow showing no tabs at all on TOT.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:445
> +        for (let tabBarItem of this._tabBarItems) {

return this._tabBarItems.some((tab) => tab.pinned);

> Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:-124
> -        return this._element.classList.contains("hide-close-button");

It feels weird to me that we need to query CSS to tell if it's a default tab. Maybe store a flag, or ask the tab bar which is its default?

[Joseph Pecoraro]

Comment on attachment 261131 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:173
> +.tab-bar.single-tab > .item.default-tab > .close {
>      pointer-events: none;
>  }

This is the code that disallows pointer-events. So I think it should work with the middle click case.

[Joseph Pecoraro]

Comment on attachment 261131 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review

>> Source/WebInspectorUI/UserInterface/Views/TabBar.css:173
>>  }
> 
> This is the code that disallows pointer-events. So I think it should work with the middle click case.

Oh, this is only on the .close, and middle-click works entirely on the tab. I should be able to test after updating.

[Joseph Pecoraro]

Comment on attachment 261131 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=261131&action=review

>> Source/WebInspectorUI/UserInterface/Base/Main.js:476
>> +    const animate = true;
> 
> NIT: Are you creating this new const for the sake of clarity?

Yes. We've been doing this in a few places. Here it made sense.

>>> Source/WebInspectorUI/UserInterface/Base/Main.js:485
>>> +WebInspector.showNewTabTab = function(animate)
>> 
>> NIT: It might make more sense to call this "shouldAnimate"
> 
> +1, i think we should codify this in the style guide.

Sounds good.

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:445
>> +        for (let tabBarItem of this._tabBarItems) {
> 
> return this._tabBarItems.some((tab) => tab.pinned);

!tab.pinned. Very nice indeed!

>> Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:-124
>> -        return this._element.classList.contains("hide-close-button");
> 
> It feels weird to me that we need to query CSS to tell if it's a default tab. Maybe store a flag, or ask the tab bar which is its default?

This is a pretty common pattern for us to store boolean View states in classList.

[Joseph Pecoraro]

Created attachment 261215 [details]
[PATCH] Proposed Fix

[BJ Burg]

Comment on attachment 261215 [details]
[PATCH] Proposed Fix

r=me

[WebKit Commit Bot]

Comment on attachment 261215 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 261215

Committed r189815: <http://trac.webkit.org/changeset/189815>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.