Closed Bug 1458022 Opened 2 years ago Closed 2 years ago

Implement ability to close a selection of tabs

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
firefox61 --- wontfix
firefox62 - disabled
firefox64 --- verified
firefox65 --- verified

People

(Reporter: jaws, Assigned: ablayelyfondou, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: feature)

Attachments

(1 file)

When a group of tabs are selected:
1. Clicking on the Close button of any tab in the selection will close all of the tabs in the selection.
2. Right-clicking on any tab in the selection should:
2a. Provide a context menu option of "Close selected tabs".
2b. The "Close tab" option will be hidden.

The preference `browser.tabs.warnOnClose` should be respected. While working on this, we should remove duplication between the tabbrowser.js `removeAllTabsBut` and `removeTabsToTheEndFrom` functions.
Now when a user clicks on the Close button of a tab which is not in the selection: 
 1. Should we clear the selection? 
 2. Or should they remain selected?
Flags: needinfo?(jaws)
To match the behavior of Google Chrome we should go with #2 and they should remain selected.
Flags: needinfo?(jaws)
Comment on attachment 8979423 [details]
Bug 1458022 - Implement ability to close a selection of tabs.

https://reviewboard.mozilla.org/r/245600/#review252218

Looks good! Please make the following changes, then I'll push it to tryserver to make sure that it passes on all platforms before landing it on autoland.

::: browser/base/content/tabbrowser.js:2607
(Diff revision 1)
> -
> -      this.removeTab(tab, aParams);
> -    };
> -
>      let tabs = this.getTabsToTheEndFrom(aTab);
> -    let tabsWithBeforeUnload = [];
> +    this.removeCollectionOfTabs(tabs);

This should also use {animate: true}. It's a bug that we didn't animate the closing of these tabs when this feature got implemented.

::: browser/base/content/tabbrowser.js:2615
(Diff revision 1)
>    removeAllTabsBut(aTab) {
>      if (!this.warnAboutClosingTabs(this.closingTabsEnum.OTHER)) {
>        return;
>      }
>  
> -    let tabs = this.visibleTabs.reverse();
> +    let tabs = this.visibleTabs.filter(tab => tab != aTab && !tab.pinned);

`let tabs = this.visibleTabs.reverse();` was added by bug 1356153 comment 9 to make sure that tabs are removed in left-to-right order. The array is reversed because the for loop goes in reverse order.

I don't see why we wouldn't want to close tabs in left-to-right order for removeAllTabsBut and removeMultiSelectedTabs so let's change the loop in removeCollectionOfTabs to move in ascending order.

::: browser/base/content/tabbrowser.js:2625
(Diff revision 1)
> +  removeMultiSelectedTabs(aParams) {
> +    if (!this.warnAboutClosingTabs(this.closingTabsEnum.MULTI_SELECTED)) {
> +      return;
> +    }
> +
> +    const selectedTabs = ChromeUtils.nondeterministicGetWeakMapKeys(this._multiSelectedTabsMap)

Can you use `let` here since we use `let` for the other remove* functions?

::: browser/base/content/tabbrowser.js:2630
(Diff revision 1)
> +    const selectedTabs = ChromeUtils.nondeterministicGetWeakMapKeys(this._multiSelectedTabsMap)
> +                                    .filter(tab => tab.isConnected);
> +    this.removeCollectionOfTabs(selectedTabs, aParams);
> +  },
> +
> +  removeCollectionOfTabs(aTabs, aParams) {

You can drop aParams here since all callers will be using {animate: true}

Also, change aTabs to just tabs and then you can delete the `let tabs = aTabs;` line.

::: browser/base/content/tabbrowser.js:2634
(Diff revision 1)
>      for (let i = tabs.length - 1; i >= 0; --i) {
>        let tab = tabs[i];

Adding on to my previous comment, this can now be `for (let tab of tabs)` and then you can delete the `let tab = tabs[i];` line.

::: browser/base/content/tabbrowser.js:2638
(Diff revision 1)
> +      } else if (this._hasBeforeUnload(tab))
> -          tabsWithBeforeUnload.push(tab);
> +        tabsWithBeforeUnload.push(tab);
> -        else
> +      else

We should be consistent with curly brackets. Right now this if/else if/else has a mix of them. I'm fine with you removing them from the if-condition at the beginning of the loop since that is the only new code.

::: browser/base/content/tabbrowser.js:3639
(Diff revision 1)
>      if (aTab1 == aTab2) {
>        this.addToMultiSelectedTabs(aTab1);
>        return;
>      }
>  
> -    const tabs = [...this.tabs];
> +    const tabs = this._visibleTabs;

Thanks!

::: browser/base/content/tabbrowser.xml:2018
(Diff revision 1)
>          }
>  
>          if (event.originalTarget.getAttribute("anonid") == "close-button") {
> -          gBrowser.removeTab(this, {
> +          const params = {
>              animate: true,
> -            byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE,
> +            byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE

Please revert this change and put the comma back at the end of the line. eslint will normally require that each line of an object definition have a comma at the end.

::: browser/base/content/test/tabs/browser_multiselect_tabs_close.js:32
(Diff revision 1)
> +    ok(!tab4.multiselected, "Tab4 is not multiselected");
> +    is(gBrowser.multiSelectedTabsCount, 2, "Two multiselected tabs");
> +
> +    // Closing a tab which is not multiselected
> +    let tab4CloseBtn = document.getAnonymousElementByAttribute(tab4, "anonid", "close-button");
> +    tab4CloseBtn.click();

Use BrowserTestUtils.waitForTabClosing to pause the test until the tab starts closing.

::: browser/base/content/test/tabs/browser_multiselect_tabs_close.js:71
(Diff revision 1)
> +    // Check the context menu with zero multiselected tabs
> +    updateTabContextMenu(tab4);
> +    is(menuItemCloseTab.hidden, false, "Close Tab is visible");
> +    is(menuItemCloseSelectedTabs.hidden, true, "Close Selected Tabs is hidden");
> +
> +

Please remove one of these blank lines.
Attachment #8979423 - Flags: review?(jaws) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d8d57b1d5f0
Implement ability to close a selection of tabs. r=jaws
https://hg.mozilla.org/mozilla-central/rev/1d8d57b1d5f0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> When a group of tabs are selected:
> 1. Clicking on the Close button of any tab in the selection will close all
> of the tabs in the selection.
> 2. Right-clicking on any tab in the selection should:
> 2a. Provide a context menu option of "Close selected tabs".
> 2b. The "Close tab" option will be hidden.
> 
> The preference `browser.tabs.warnOnClose` should be respected. While working
> on this, we should remove duplication between the tabbrowser.js
> `removeAllTabsBut` and `removeTabsToTheEndFrom` functions.

I just tested this out and closing a selection of tabs with either the close button or the context menu works perfectly.

One question though: shouldn't this also respect the keybinding to close tabs (ie. command+w and ctrl+w)? Because right now, when having a selection, pressing the keybinding will only close the current tab.
Flags: needinfo?(jaws)
Does this feature affect to the WebExtensions API: browser.tabs.remove()? When one of multiselected tab is specified, all multiselected tabs should be closed also?
Abdoulaye, could you make a release notes addition request? Instructions are here:
https://wiki.mozilla.org/Release_Management/Release_Notes#How_to_nominate_a_bug_for_release_notes_addition.3F

Thanks
Flags: needinfo?(ablayelyfondou)
Keywords: feature
(In reply to julie.engel from comment #8)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> > When a group of tabs are selected:
> > 1. Clicking on the Close button of any tab in the selection will close all
> > of the tabs in the selection.
> > 2. Right-clicking on any tab in the selection should:
> > 2a. Provide a context menu option of "Close selected tabs".
> > 2b. The "Close tab" option will be hidden.
> > 
> > The preference `browser.tabs.warnOnClose` should be respected. While working
> > on this, we should remove duplication between the tabbrowser.js
> > `removeAllTabsBut` and `removeTabsToTheEndFrom` functions.
> 
> I just tested this out and closing a selection of tabs with either the close
> button or the context menu works perfectly.
> 
> One question though: shouldn't this also respect the keybinding to close
> tabs (ie. command+w and ctrl+w)? Because right now, when having a selection,
> pressing the keybinding will only close the current tab.

Yes, pressing Command+W/Ctrl+W should close the full selection. Can you please file a bug to track this?

(In reply to YUKI "Piro" Hiroshi from comment #9)
> Does this feature affect to the WebExtensions API: browser.tabs.remove()?
> When one of multiselected tab is specified, all multiselected tabs should be
> closed also?

You can use browser.tabs.query({highlighted: true}) to get the selection of tabs then pass that to browser.tabs.remove().

(In reply to Pascal Chevrel:pascalc from comment #10)
> Abdoulaye, could you make a release notes addition request? Instructions are
> here:
> https://wiki.mozilla.org/Release_Management/
> Release_Notes#How_to_nominate_a_bug_for_release_notes_addition.3F
> 
> Thanks

We should not put this in the release notes yet as the feature is still disabled behind the `browser.tabs.multiselect` pref.
Flags: needinfo?(jaws)
Flags: needinfo?(ablayelyfondou)
Hi Jared,

I understand that the feature will be disabled behind the `browser.tabs.multiselect` pref but we(QA) would need details on what kind of QA involvement is expected for this. We need to understand how to test this and if this will be treated as a Bug work or we need to follow the full process (Test Plan, Sign Off etc.).

Could you please refer this (https://mana.mozilla.org/wiki/display/PI/PI+Request) and submit a PI request? We would need this submitted by June 4th, your earliest response would be appreciated!

Thank you!
Flags: needinfo?(jaws)
Re: PI Request for this ticket. Just wanted to add a point.

Here is the Trello link for reference. We track the Milestones and Test results in Trello
https://trello.com/c/RRnxzi9X/541-implement-ability-to-close-a-selection-of-tabs#
[Tracking Requested - why for this release]: Requesting that tracking-firefox62 be marked as '-' due to the feature being disabled on Firefox 62.
Flags: needinfo?(jaws)
See Also: → 1467340
(In reply to julie.engel from comment #8)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> > When a group of tabs are selected:
> > 1. Clicking on the Close button of any tab in the selection will close all
> > of the tabs in the selection.
> > 2. Right-clicking on any tab in the selection should:
> > 2a. Provide a context menu option of "Close selected tabs".
> > 2b. The "Close tab" option will be hidden.
> > 
> > The preference `browser.tabs.warnOnClose` should be respected. While working
> > on this, we should remove duplication between the tabbrowser.js
> > `removeAllTabsBut` and `removeTabsToTheEndFrom` functions.
> 
> I just tested this out and closing a selection of tabs with either the close
> button or the context menu works perfectly.
> 
> One question though: shouldn't this also respect the keybinding to close
> tabs (ie. command+w and ctrl+w)? Because right now, when having a selection,
> pressing the keybinding will only close the current tab.

Thank you Julie, I filed a bug #1467340 for this and working on a patch.
Blocks: 1468443
Blocks: 1472910
No longer blocks: 1472910
Depends on: 1478652
Depends on: 1479775
Verified fixed in latest nightly 65.0a1(2018-11-07) and latest Beta 64.0b7.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.