Bug 149130 - Web Inspector: Closing the final inspector tab should be allowed
Summary: Web Inspector: Closing the final inspector tab should be allowed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-14 14:37 PDT by Joseph Pecoraro
Modified: 2015-09-15 11:26 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (10.38 KB, patch)
2015-09-14 14:55 PDT, Joseph Pecoraro
bburg: review-
bburg: commit-queue-
Details | Formatted Diff | Diff
[VIDEO] Default Tab behavior (1.64 MB, video/quicktime)
2015-09-14 14:58 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (11.01 KB, patch)
2015-09-15 10:59 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-09-14 14:37:00 PDT
* 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
Comment 1 Joseph Pecoraro 2015-09-14 14:37:08 PDT
<rdar://problem/22677259>
Comment 2 Joseph Pecoraro 2015-09-14 14:55:14 PDT
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)
Comment 3 Joseph Pecoraro 2015-09-14 14:58:05 PDT
Created attachment 261132 [details]
[VIDEO] Default Tab behavior
Comment 4 Devin Rousso 2015-09-14 16:03:07 PDT
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
Comment 5 BJ Burg 2015-09-15 09:22:42 PDT
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?
Comment 6 Joseph Pecoraro 2015-09-15 10:21:05 PDT
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.
Comment 7 Joseph Pecoraro 2015-09-15 10:22:01 PDT
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.
Comment 8 Joseph Pecoraro 2015-09-15 10:59:05 PDT
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.
Comment 9 Joseph Pecoraro 2015-09-15 10:59:40 PDT
Created attachment 261215 [details]
[PATCH] Proposed Fix
Comment 10 BJ Burg 2015-09-15 11:20:48 PDT
Comment on attachment 261215 [details]
[PATCH] Proposed Fix

r=me
Comment 11 WebKit Commit Bot 2015-09-15 11:26:41 PDT
Comment on attachment 261215 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 261215

Committed r189815: <http://trac.webkit.org/changeset/189815>
Comment 12 WebKit Commit Bot 2015-09-15 11:26:46 PDT
All reviewed patches have been landed.  Closing bug.