866880 - Implement "Close Tabs to the Right" as a built-in feature

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Closing tabs "to the right" is a useful feature that many users could benefit from. It is currently implemented as an add-on (see URL field).

Some background:
When we open new tabs they appear on the end, and so naturally tabs that have a longer lifetime end up being promoted to the start-side of the bar. This leads us towards the situation where closing tabs "to the right" is a simple way of closing the ephemeral tabs.

This feature would work as another context-menu item when right-clicking on a tab, alongside "Close other tabs".

Ekanan, would you be able to work on this?

Comment 1

[Michael Hoffmann]

Hi, I'd be happy to work on this one if it's free. I've recently submitted a patch to another bug, waiting for review, so this wouldn't count as my first bug. But if that's still ok I think this one seems to be an interesting one to work with.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Michael Brennan from comment #1)
> Hi, I'd be happy to work on this one if it's free. I've recently submitted a
> patch to another bug, waiting for review, so this wouldn't count as my first
> bug. But if that's still ok I think this one seems to be an interesting one
> to work with.

That's perfectly fine! Let me know if you have any questions.

Comment 3

[Frank Yan (:fryn)]

I don't think tabs are that often actively promoted towards the left via drag & drop, but it probably happens through selective tab closure.

To provide some historical context, the directionality of this menu item is what made Faaborg reluctant to add this a few years ago, but I'm not opposed to it.

---

This will need to be "Close Tabs to the Left" in RTL mode.

Comment 4

[Michael Hoffmann]

Thanks, I have started going through the code and I some questions that have popped up.

I assume that the user should get a warning if more than one tab are about to be closed? In tabbrowser.xml there's the warnAboutClosingTabs function that warns if all tabs, or all but one tab, are to be closed. This is controlled by the boolean parameter aAll and the settings "browser.tabs.warnOnClose" and "browser.tabs.warnOnCloseOtherTabs".

How should it be handled here? Should a new setting be added and support be added for it to warnAboutClosingTabs? Because of RTL mode, maybe an appropriate name for it could be CloseFollowingTabs to avoid confusion?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Michael Brennan from comment #4)
> Thanks, I have started going through the code and I some questions that have
> popped up.
> 
> I assume that the user should get a warning if more than one tab are about
> to be closed? In tabbrowser.xml there's the warnAboutClosingTabs function
> that warns if all tabs, or all but one tab, are to be closed. This is
> controlled by the boolean parameter aAll and the settings
> "browser.tabs.warnOnClose" and "browser.tabs.warnOnCloseOtherTabs".
> 
> How should it be handled here? Should a new setting be added and support be
> added for it to warnAboutClosingTabs?

We should use the 'warnOnCloseOtherTabs' pref to determine if we should show the dialog. The 'warnOnClose' prompt is used more for closing the full window.

> Because of RTL mode, maybe an
> appropriate name for it could be CloseFollowingTabs to avoid confusion?

How about 'closeTabsToTheEnd'?

Comment 6

[Michael Hoffmann]

Attachment: Partial patch of a first implementation

Thanks for your input! I've made an implementation that seems to work, but I am not finished yet as I haven't added any tests and I have an idea to improve it described below, but I thought I'd show my solution this far.

I modified warnAboutClosingTabs to be able to handle a third case of counting the tabs from the context tab. I'm not particulary fond of the solution of using a string to specify the action, though I'm not used to javascript so I think it's good to get some comments on this.

I was thinking about making two methods, for example warnAboutClosingAllTabs() and warnAboutClosingOtherTabs(aToTheEnd), where the parameter is a boolean, or possibly a number specifying the number of tabs to warn about, leaving it up to the caller to count the tabs. What do you think about that?

I've tried it with forced RTL mode where it also behaves properly. Is RTL mode fixed with the locale, so that it's only a matter of localization whether the menu item should say "Close Tabs to the Right" or "Close Tabs to the Left"?

Oh, and should the removal of the tabs be animated? It's currently not.

Comment 7

[Michael Hoffmann]

Two things I realized afterwards: I probably shouldn't use for...in for the tabs array in removeTabsToTheEndFrom, and it would be better to use unshift rather than push in getUnpinnedTabsToTheEndFrom, so that the order of the tabs aren't reversed.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 747385 [details] [diff] [review]
Partial patch of a first implementation

Review of attachment 747385 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Michael Brennan from comment #6)
> Thanks for your input! I've made an implementation that seems to work, but I
> am not finished yet as I haven't added any tests and I have an idea to
> improve it described below, but I thought I'd show my solution this far.

Cool! It will be great to have some tests for this :)
 
> I modified warnAboutClosingTabs to be able to handle a third case of
> counting the tabs from the context tab. I'm not particulary fond of the
> solution of using a string to specify the action, though I'm not used to
> javascript so I think it's good to get some comments on this.
> 
> I was thinking about making two methods, for example
> warnAboutClosingAllTabs() and warnAboutClosingOtherTabs(aToTheEnd), where
> the parameter is a boolean, or possibly a number specifying the number of
> tabs to warn about, leaving it up to the caller to count the tabs. What do
> you think about that?

I think the way you've got it now is fine.

> I've tried it with forced RTL mode where it also behaves properly. Is RTL
> mode fixed with the locale, so that it's only a matter of localization
> whether the menu item should say "Close Tabs to the Right" or "Close Tabs to
> the Left"?

RTL mode is part of locale, so the locales that use a RTL language will have "Close Tabs to the Left" as their strings. No need for extra work on your end here :)

(In reply to Michael Brennan from comment #7)
> Two things I realized afterwards: I probably shouldn't use for...in for the
> tabs array in removeTabsToTheEndFrom, and it would be better to use unshift
> rather than push in getUnpinnedTabsToTheEndFrom, so that the order of the
> tabs aren't reversed.

Be careful about calling unshift in a loop, as it will cause n^2 memory operations. Also be careful about removing items from a collection while in a loop, as the indices will change and it is easy to skip items or go out of bounds.

::: browser/base/content/tabbrowser.xml
@@ +1489,5 @@
>        <body>
>          <![CDATA[
> +          var tabsToClose;
> +          switch (aCloseTabs) {
> +            case "all":

I think an enumerated type here may be better (basically define these values as constant strings), then you can also include a default case that throws an exception so we can make sure all callers are using the correct arguments.

Here's an example of a section of our code that defines constant strings that will be used as arguments, http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/applications.js#9

@@ +1570,5 @@
> +
> +            if (this.warnAboutClosingTabs("end")) {
> +              let tabs = this.getUnpinnedTabsToTheEndFrom(aTab);
> +              for (var index in tabs) {
> +                this.removeTab(tabs[index]);

The tabs should animate when closing. You can pass in {animate: true} as the second argument to removeTab.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1572

Comment 9

[Michael Hoffmann]

Attachment: Patch for the bug with tests

Thanks for great feedback! I've taken your comments into consideration and attached a fix together with some tests.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 751851 [details] [diff] [review]
Patch for the bug with tests

Review of attachment 751851 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really great!

::: browser/base/content/tabbrowser.xml
@@ +1495,5 @@
> +          var tabsToClose;
> +          switch (aCloseTabs) {
> +            case this.warnTabs.ALL:
> +              tabsToClose = this.tabs.length - this._removingTabs.length
> +                - gBrowser._numPinnedTabs;

I know you are just modifying how this previous code worked, but we should move the subtract operator to the previous line and line up the gBrowser._numPinnedTabs with this.tabs.length, like so:

> tabsToClose = this.tabs.length - this._removingTabs.length -
>               gBrowser._numPinnedTabs;

@@ +1501,5 @@
> +            case this.warnTabs.OTHER:
> +              tabsToClose = this.visibleTabs.length - 1 - gBrowser._numPinnedTabs;
> +              break;
> +            case this.warnTabs.END:
> +              if (aTab === undefined)

if (!aTab)

::: browser/components/nsBrowserGlue.js
@@ +689,5 @@
>          // tabs checks browser.tabs.warnOnClose and returns if it's ok to close
>          // the window. It doesn't actually close the window.
>          mostRecentBrowserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +        aCancelQuit.data = !mostRecentBrowserWindow.gBrowser.warnAboutClosingTabs(
> +          mostRecentBrowserWindow.gBrowser.warnTabs.ALL);

Add an alias to mostRecentBrowserWindow.gBrowser.warnTabs.ALL before this line so it doesn't have to wrap. For example:

> let allTabs = mostRecentBrowserWindow.gBrowser.warnTabs.ALL;
> aCancelQuit.data = !mostRecentBrowserWindow.gBrowser.warnAboutClosingTabs(allTabs);

Comment 11

[Michael Hoffmann]

Attachment: Patch for the bug with tests, with changes from review

Thanks! I've attached a new patch which includes the changes from your review.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 753757 [details] [diff] [review]
Patch for the bug with tests, with changes from review

>+      <field name="warnTabs" readonly="true">({ ALL: "all", OTHER: "other", END: "end" });</field>

How about:

<field name="closingTabsEnum" readonly="true">({ ALL: 0, OTHER: 1, TO_END: 2 });</field>

>+      <method name="getUnpinnedTabsToTheEndFrom">
>+        <parameter name="aTab"/>
>+        <body>
>+          <![CDATA[
>+              var tabsToEnd = [];
>+              let tabs = this.visibleTabs;
>+              for (let i = tabs.length - 1; tabs[i] != aTab && i >= 0; --i) {
>+                if (!tabs[i].pinned)
>+                  tabsToEnd.push(tabs[i]);
>+              }
>+              return tabsToEnd.reverse();
>+          ]]>
>+        </body>
>+      </method>

Indentation is off.

I don't understand why this method cares about pinned tabs. No caller seems to depend on this.

>+<!ENTITY  closeTabsToTheEnd.label            "Close Tabs to the Right">
>+<!ENTITY  closeTabsToTheEnd.accesskey        "i">

This needs a localization note about UI directions.

Comment 13

[Michael Hoffmann]

(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 753757 [details] [diff] [review]
> Patch for the bug with tests, with changes from review
> 
> >+      <field name="warnTabs" readonly="true">({ ALL: "all", OTHER: "other", END: "end" });</field>
> 
> How about:
> 
> <field name="closingTabsEnum" readonly="true">({ ALL: 0, OTHER: 1, TO_END: 2
> });</field>

Looks good to me.
 
> >+      <method name="getUnpinnedTabsToTheEndFrom">
> >+        <parameter name="aTab"/>
> >+        <body>
> >+          <![CDATA[
> >+              var tabsToEnd = [];
> >+              let tabs = this.visibleTabs;
> >+              for (let i = tabs.length - 1; tabs[i] != aTab && i >= 0; --i) {
> >+                if (!tabs[i].pinned)
> >+                  tabsToEnd.push(tabs[i]);
> >+              }
> >+              return tabsToEnd.reverse();
> >+          ]]>
> >+        </body>
> >+      </method>
> 
> Indentation is off.

Good catch!

> I don't understand why this method cares about pinned tabs. No caller seems
> to depend on this.

Like the close-other-tabs functionality and the implementation of the referenced add-on, I figured that pinned tabs should not be closed and that's why it explicitly ignores them. Of course, if we can rely on the pinned tabs always being before a normal tab (can we?), then that check for pinned tabs is unnecessary.

I removed it and it seems to work fine.
 
> >+<!ENTITY  closeTabsToTheEnd.label            "Close Tabs to the Right">
> >+<!ENTITY  closeTabsToTheEnd.accesskey        "i">
> 
> This needs a localization note about UI directions.

Yes, I was going to ask about this one. How is this handled? Is there anything I should do here?

Comment 14

[Michael Hoffmann]

Attachment: Patch with suggested changes from :dao

Comment 15

[Dão Gottwald [:dao]]

(In reply to Michael Brennan from comment #13)
> > >+<!ENTITY  closeTabsToTheEnd.label            "Close Tabs to the Right">
> > >+<!ENTITY  closeTabsToTheEnd.accesskey        "i">
> > 
> > This needs a localization note about UI directions.
> 
> Yes, I was going to ask about this one. How is this handled? Is there
> anything I should do here?

Yes. You need to explain that "right" shouldn't be translated verbally, depending on the UI direction. A few lines below your entities you can find an existing localization note that you can take as a model.

Comment 16

[Michael Hoffmann]

Attachment: Patch with suggested changes from :dao

Updated patch with localization note

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 754283 [details] [diff] [review]
Patch with suggested changes from :dao

Review of attachment 754283 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Please make the following changes, and then you can reupload the patch with r=jaws at the end of your patch summary.

::: browser/base/content/tabbrowser.xml
@@ +1576,5 @@
> +        <parameter name="aTab"/>
> +        <body>
> +          <![CDATA[
> +            if (aTab.pinned)
> +              return;

This check seems unnecessary since the function isn't called from pinned tabs.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +17,5 @@
>  
>  <!-- Tab context menu -->
> +<!-- LOCALIZATION NOTE (closeTabsToTheEnd.label): This should indicate the
> +direction in which tabs are closed, i.e. locales that use RTL mode should say
> +left instead of right. -->

Please move the localization note to be on the line directly above the entity definition for closeTabsToTheEnd.label.

Comment 18

[Michael Hoffmann]

Attachment: Patch for the bug with tests

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 755365 [details] [diff] [review]
Patch for the bug with tests

Review of attachment 755365 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, let's wait for a ui-review from someone on the UX team before landing.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0faea43bb3d

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e0faea43bb3d

Comment 23

[MarianG]

being that "browser.tabs.closeButtons" (about:config) is an "integer" shouldn't it put the close button to the "user set" number of tabs?

Comment 24

[MarianG]

Sorry about that, I found my answer.

Comment 25

[Alex Keybl [:akeybl]]

Adding the feature keyword to be included in the new Release Tracking page.

Comment 26

[Haziq]

Kindly as the keyboard shortcut for closing the tabs of right