The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jaws, Assigned: Michael Brennan)

Tracking

(Depends on: 1 bug, {feature})

Trunk
Firefox 24
feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 24+)

Details

(Whiteboard: [good first bug][mentor=jaws][lang=js], URL)

Attachments

(1 attachment, 5 obsolete attachments)

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?

Updated

4 years ago
Assignee: ananuti → nobody

Updated

4 years ago
Status: ASSIGNED → NEW
(Assignee)

Comment 1

4 years ago
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.

Updated

4 years ago
Assignee: nobody → brennan.brisad
Status: NEW → ASSIGNED
(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

4 years ago
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.
(Assignee)

Comment 4

4 years ago
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?
(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'?
(Assignee)

Comment 6

4 years ago
Created attachment 747385 [details] [diff] [review]
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.
(Assignee)

Comment 7

4 years ago
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.
Attachment #747385 - Flags: feedback?(jaws)
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
Attachment #747385 - Flags: feedback?(jaws) → feedback+
(Assignee)

Comment 9

4 years ago
Created attachment 751851 [details] [diff] [review]
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.
Attachment #747385 - Attachment is obsolete: true
Attachment #751851 - Flags: review?(jaws)
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);
Attachment #751851 - Flags: review?(jaws) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 753757 [details] [diff] [review]
Patch for the bug with tests, with changes from review

Thanks! I've attached a new patch which includes the changes from your review.
Attachment #753757 - Flags: review?(jaws)
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.
(Assignee)

Comment 13

4 years ago
(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?
(Assignee)

Comment 14

4 years ago
Created attachment 754037 [details] [diff] [review]
Patch with suggested changes from :dao
Attachment #754037 - Flags: review?(jaws)
(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.

Updated

4 years ago
Attachment #753757 - Attachment is obsolete: true
Attachment #753757 - Flags: review?(jaws)

Updated

4 years ago
Attachment #751851 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Created attachment 754283 [details] [diff] [review]
Patch with suggested changes from :dao

Updated patch with localization note
Attachment #754037 - Attachment is obsolete: true
Attachment #754037 - Flags: review?(jaws)
Attachment #754283 - Flags: review?(jaws)
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.
Attachment #754283 - Flags: review?(jaws) → review+
(Assignee)

Comment 18

4 years ago
Created attachment 755365 [details] [diff] [review]
Patch for the bug with tests
Attachment #754283 - Attachment is obsolete: true
Attachment #755365 - Flags: review?(jaws)

Updated

4 years ago
Attachment #755365 - Flags: ui-review?(ux-review)
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.
Attachment #755365 - Flags: review?(jaws) → review+
Attachment #755365 - Flags: ui-review?(ux-review) → ui-review?(madhava)
Attachment #755365 - Flags: ui-review?(madhava) → ui-review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0faea43bb3d
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0faea43bb3d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24

Updated

4 years ago
Depends on: 884740
relnote-firefox: --- → ?

Updated

4 years ago
relnote-firefox: ? → 24+

Updated

4 years ago
Duplicate of this bug: 715170
Depends on: 896291
No longer depends on: 896291

Comment 23

4 years ago
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

4 years ago
Sorry about that, I found my answer.
Adding the feature keyword to be included in the new Release Tracking page.
Keywords: feature

Updated

2 years ago
Depends on: 1135303
You need to log in before you can comment on or make changes to this bug.