1472910 - In a multiselect context, close other tabs should close all except the multi-selected tabs.

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Abdoulaye O. LY]

Close other tabs menu should not affect multiselected tabs when the context tab is multiselected.

Comment 1

[Abdoulaye O. LY]

Attachment: Bug 1472910 - Close all unselected tabs except those pinned with gBrowser.removeAllTabsBut(aTab) when aTab is multi-selected.

Review commit: https://reviewboard.mozilla.org/r/256428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256428/

Comment 2

[Abdoulaye O. LY]

Comment on attachment 8991552 [details]
Bug 1472910 - Close all unselected tabs except those pinned with gBrowser.removeAllTabsBut(aTab) when aTab is multi-selected.

https://reviewboard.mozilla.org/r/256426/#review263346

::: browser/base/content/tabbrowser.js:2505
(Diff revision 1)
> +          tabsToClose = this.visibleTabs.length - unpinnedSelectedTab.length -
> +            gBrowser._numPinnedTabs;
> +        } else {
> +          // If aTab is pinned, it will already be considered
> +          // with gBrowser._numPinnedTabs.
> +          tabsToClose = this.visibleTabs.length - gBrowser._numPinnedTabs -

I don't know if it's okay to fix this "tiny" bug (bad count value for removing tabs with pinned tabs) here but I did.

To reproduce:
1. open 4 tabs A,B,C,D
2. pin A
3. righ click on A and select "close other tabs"
4. Warning on close message will say 2 tabs to close while it should be 3 instead.

Comment 3

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

Comment on attachment 8991552 [details]
Bug 1472910 - Close all unselected tabs except those pinned with gBrowser.removeAllTabsBut(aTab) when aTab is multi-selected.

https://reviewboard.mozilla.org/r/256428/#review263440

This looks good. Can you update the commit message and do the unpinnedSelectedTab -> unpinnedSelectedTabs rename? And then in a separate commit do the refactoring of warnAboutClosingTabs?

::: commit-message-3aca1:1
(Diff revision 1)
> +Bug 1472910 - Close all none selected tabs except those unpinned with gBrowser.removeAllTabsBut(aTab) when aTab is multi-selected. r?jaws

Can you use this as the commit message?

Bug 1472910 - Close all unselected tabs except those pinned with gBrowser.removeAllTabsBut(aTab) when aTab is multi-selected. r?jaws

::: browser/base/content/tabbrowser.js:2499
(Diff revision 1)
> +          throw new Error("Required argument missing: aTab");
> +        }
> +        if (aTab.multiselected) {
> +          // Avoid counting pinned selected tabs twice as they will already be
> +          // taken into account with gBrowser._numPinnedTabs.
> +          let unpinnedSelectedTab = this.selectedTabs.filter(tab => !tab.pinned);

unpinnedSelectedTabs

::: browser/base/content/tabbrowser.js:2499
(Diff revision 1)
> +          let unpinnedSelectedTab = this.selectedTabs.filter(tab => !tab.pinned);
> +          tabsToClose = this.visibleTabs.length - unpinnedSelectedTab.length -
> +            gBrowser._numPinnedTabs;

Can we use the same code from removeAllTabsBut here? So instead of doing the math the way you have it here, we could just do:

tabsToClose = this.visibleTabs.filter(tab => !tab.multiselected && !tab.pinned).length;

We should probably refactor this code so that warnAboutClosingTabs just takes the number of tabs instead of `aTab`, and then we don't have to duplicate logic between warnAboutClosingTabs and removeTabsToTheEndFrom/removeAllTabsBut.

Comment 4

[Abdoulaye O. LY]

Comment on attachment 8991552 [details]
Bug 1472910 - Close all unselected tabs except those pinned with gBrowser.removeAllTabsBut(aTab) when aTab is multi-selected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256428/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3eeac2986f6c
Close all unselected tabs except those pinned with gBrowser.removeAllTabsBut(aTab) when aTab is multi-selected. r=jaws

Comment 6

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3eeac2986f6c

Comment 7

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 63.0a1 (2018-07-05) on Windows 10, 64 Bit!
This bug's fix is verified with latest Nightly!

Build ID 	20180811220142
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Comment 8

[Vlad Lucaci, Desktop QA (:vlucaci)]

Verified fixed in latest nightly 65.0a1(2018-11-07) and latest Beta 64.0b7 on Windows 10x64, macOS 10.13 and Ubuntu 16.04x64.