577121 - [App Tab] using Tab Context Menu Item "Close Other Tabs" should exclude App Tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[(mostly gone) XtC4UaLL [:xtc4uall]]

Let's say there's given [AppTab1][AppTab2][Tab1][Tab2], using "Close Other Tabs" on the [Tab2] Menu closes all the other Tabs.

I'd rather expect - depending on the Purpose and Definition of App Tabs (Stickyness/Persistence) - that all normal Tabs minus the App Tabs are being closed. Currently there's no distinction of both Tab Kinds being made in this Regard.

As Comparison:
On Google Chrome 6.0.453.1 with above Steps, the App Tabs remain opened.

Comment 1

[Michael Kohler [:mkohler]]

Attachment: Patch v1 (with test)

* checks if the tab-to-close is an app tab before closing it.
* the patch contains a test, which tests if removeAllTabsBut(tab) behaves correctly

Comment 2

[Michael Kohler [:mkohler]]

Attachment: Patch v2 (with test)

* don't close app tabs if "Close other Tabs" is executed
* don't show "Close other Tabs" if there is only one unpinned tab
* corrected the close warning (right number of tabs to close)
* includes a test for testing that only unpinned tabs are closed by "Close other Tabs"

Comment 3

[Dão Gottwald [:dao]]

There's gBrowser._numPinnedTabs which you can use in a couple of places.

Comment 4

[Michael Kohler [:mkohler]]

Attachment: Patch v3 (with test)

gBrowser._numPinnedTabs used.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 456858 [details] [diff] [review]
Patch v3 (with test)

>+    // Disable "Close other Tabs" if there is just one unpinned tab and other
>+    // pinned tabs.
>+    var unpinnedTabs = gBrowser.tabs.length - gBrowser._numPinnedTabs;
>+    document.getElementById("context_closeOtherTabs").disabled = unpinnedTabs < 2;

Remove tbattr="tabbrowser-multiple" from this element since you're handling it manually.

>+          if (!aAll) {
>+            // don't close the current tab and don't close pinned tabs.
>+            tabsToClose = (tabsToClose - 1) - gBrowser._numPinnedTabs;

tabsToClose -= 1 + gBrowser._numPinnedTabs;

However, this doesn't handle the case of the current tab being pinned.

>+  // Pin the second tab.
>+  testTab2.setAttribute("pinned", "true");

gBrowser.pinTab(testTab2);

Comment 6

[Dão Gottwald [:dao]]

Also, the test shouldn't be using waitForExplicitFinish.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #5)
> >+          if (!aAll) {
> >+            // don't close the current tab and don't close pinned tabs.
> >+            tabsToClose = (tabsToClose - 1) - gBrowser._numPinnedTabs;
> 
> tabsToClose -= 1 + gBrowser._numPinnedTabs;
> 
> However, this doesn't handle the case of the current tab being pinned.

Maybe "Close Other Tabs" shouldn't be available in the first place if the context tab is pinned?

Comment 8

[Michael Kohler [:mkohler]]

(In reply to comment #7)
> (In reply to comment #5)
> > >+          if (!aAll) {
> > >+            // don't close the current tab and don't close pinned tabs.
> > >+            tabsToClose = (tabsToClose - 1) - gBrowser._numPinnedTabs;
> > 
> > tabsToClose -= 1 + gBrowser._numPinnedTabs;
> > 
> > However, this doesn't handle the case of the current tab being pinned.
> 
> Maybe "Close Other Tabs" shouldn't be available in the first place if the
> context tab is pinned?

And what if the user wants to close all "normal" tabs?

Comment 9

[Dão Gottwald [:dao]]

He'd have to do it differently. It wouldn't make much sense for "other tabs" to cover only normal tabs when right-clicking an app tab.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

We can hide the option "close other tabs" on the context menu for app tabs to avoid the ambiguity.  We will likely want to make the context menus for app tabs specific to the particular tab either way (including things like showing or hiding chrome).

Comment 11

[eyal gruss]

how about "bookmarks all tabs" - should this also exclude app tabs?

Comment 13

[Michael Kohler [:mkohler]]

Attachment: Patch v4 (with test)

* don't close AppTabs on "Close other Tabs"
* disabled "Close other Tabs" if there is just one unpinned tab (and 0 or several AppTabs)
* hide "Close other Tabs" if the user rightclicked on an AppTab

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 458753 [details] [diff] [review]
Patch v4 (with test)

># HG changeset patch
># User Michael Kohler <[e-mail]>
>+    document.getElementById("context_closeOtherTabs").disabled = unpinnedTabs == 1;

nit: unpinnedTabs <= 1

(not that it matters...)

>+function cleanup() {
>+  gBrowser.removeTab(testTab);
>+  finish();
>+}

This function doesn't seem to be called.

>+function test() {
>+  // Open 3 tabs, it doesn't work with two, because it wouldn't close any other
>+  // tab like that. We need one tab to pin, one to remain open and one which gets
>+  // closed.
>+  testTab1 = gBrowser.addTab();
>+  testTab2 = gBrowser.addTab();
>+  testTab3 = gBrowser.addTab();
>+
>+  // Pin the second tab.
>+  gBrowser.pinTab(testTab2);
>+
>+  // Now execute "Close other Tabs" on the first opened tab.
>+  // -> Tab2 ist pinned, Tab1 should remain open and only Tab3 gets closed.

Your descriptions are inaccurate. The test starts with one tab already, so you're dealing with 4 tabs.

>+  gBrowser.removeAllTabsBut(testTab1);
>+
>+  is(gBrowser.tabs.length, 2, "There are two remaining tabs open.");
>+  is(gBrowser.tabs[2] == null, true, "Tab was closed correctly.");

This is testing the same thing twice. If gBrowser.tabs.length is 2, gBrowser.tabs[2] cannot exist.
You should probably test that gBrowser.tabs.length is 2, gBrowser.tabs[0] is testTab2 and gBrowser.tabs[1] is testTab1.

Comment 15

[Michael Kohler [:mkohler]]

Why is the initial tab not counted on gBrowser.tabs.length?

Comment 16

[Michael Kohler [:mkohler]]

(In reply to comment #15)
> Why is the initial tab not counted on gBrowser.tabs.length?

aah, forget that..

Comment 17

[Michael Kohler [:mkohler]]

Attachment: Patch v5 (with test)

(In reply to comment #14)
> Comment on attachment 458753 [details] [diff] [review]
> Patch v4 (with test)
> [...]
> >+function test() {
> >+  // Open 3 tabs, it doesn't work with two, because it wouldn't close any other
> >+  // tab like that. We need one tab to pin, one to remain open and one which gets
> >+  // closed.
> >+  testTab1 = gBrowser.addTab();
> >+  testTab2 = gBrowser.addTab();
> >+  testTab3 = gBrowser.addTab();
> >+
> >+  // Pin the second tab.
> >+  gBrowser.pinTab(testTab2);
> >+
> >+  // Now execute "Close other Tabs" on the first opened tab.
> >+  // -> Tab2 ist pinned, Tab1 should remain open and only Tab3 gets closed.
> 
> Your descriptions are inaccurate. The test starts with one tab already, so
> you're dealing with 4 tabs.

This means we just need to open 2 other tabs to suppress the warnAboutClosingTabs dialog. And the third one we be useless anyway. We get an error, if the initial tab isn't closed and that should be enough, or is this assumption wrong?

Also, I'm sorry it needs that many reviews..

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 458762 [details] [diff] [review]
Patch v5 (with test)

>       <menuitem id="context_reloadAllTabs" label="&reloadAllTabs.label;" accesskey="&reloadAllTabs.accesskey;"
>-                tbattr="tabbrowser-multiple"
>                 oncommand="gBrowser.reloadAllTabs();"/>
>       <menuitem id="context_closeOtherTabs" label="&closeOtherTabs.label;" accesskey="&closeOtherTabs.accesskey;"
>                 tbattr="tabbrowser-multiple"
>                 oncommand="gBrowser.removeAllTabsBut(TabContextMenu.contextTab);"/>

You hit the wrong element.

>       <method name="removeAllTabsBut">
>         <parameter name="aTab"/>
>         <body>
>           <![CDATA[
>             if (this.warnAboutClosingTabs(false)) {
>               this.selectedTab = aTab;
> 
>               for (let i = this.tabs.length - 1; i >= 0; --i) {
>-                if (this.tabs[i] != aTab)
>+                if (this.tabs[i] != aTab && !this.tabs[i].pinned)
>                   this.removeTab(this.tabs[i]);
>               }
>             }
>           ]]>
>         </body>
>       </method>

Can you make this do nothing, i.e. return early, if aTab is pinned? Seems like we should protect against this case.

>+let testTab1;
>+let testTab2;

nit: these don't need to be global, just declare them in test().

>+  // Cleanup
>+  gBrowser.removeTab(testTab1);
>+  gBrowser.removeTab(testTab2);

You're removing too many tabs, i.e. all, which is going to close the window. You need to leave one tab open.

Comment 19

[Michael Kohler [:mkohler]]

Attachment: Patch v6 (with test)

hopefully the last round of review..

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 458977 [details] [diff] [review]
Patch v6 (with test)

>+  // Cleanup. Close only one tab because we need an opened tab at the end of
>+  // the test.
>+  gBrowser.removeTab(testTab1);
>+}

You need to leave a normal tab open, so you want to remove testTab2 instead.

Comment 21

[Michael Kohler [:mkohler]]

Attachment: Patch v7 (still with test)

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 459253 [details] [diff] [review]
Patch v7 (still with test)

thanks :)

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f4fd78850340