1477780 - "Close Tabs to the Right" is closing all tabs to the right from where pointed cursor tab.

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[Damira Akh]

Steps to Reproduce:

1. Launch Firefox and open multiple tabs
2. Click and hold Ctrl/Cmd keys and select multiple tabs
4. Select "Close Tabs to the Right" from the context menu

Expected Result:
All tabs which are on the Right side of the "Multi-selected tabs" are closed.

Actual Result:
Right now "Close Tabs to the Right" is closing all tabs from the right, where pointed cursor tab, even they are part of Multi-select tabs.

Comment 1

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

I believe this is acting as desired. Consider the case of a single tab being selected, and right-clicking on a tab to the left of the selected tab and choosing "Close Tabs to the Right". This act will close the selected tab.

Amy, can you confirm?

Comment 2

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

Abdoulaye and I met and discussed this bug and found that we are not matching Chrome in all cases. If the tab that is right-clicked on is part of the multiselection, then all tabs to the right should be closed except the multiselected tabs. If the tab that is right-clicked on is not part of the multiselection, then all tabs to the right should be closed.

Comment 3

[Abdoulaye O. LY]

Attachment: Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.

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

Comment 4

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

Comment on attachment 8996928 [details]
Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.

https://reviewboard.mozilla.org/r/260270/#review268062

r- because I'd like to get your opinion on the AvoidSingleSelectedTab and putting more logic in to getTabsToTheEndFrom.

::: browser/base/content/tabbrowser.js:2552
(Diff revision 1)
> +   * In a multi-select context, the tabs (except pinned tabs) that are located to the
> +   * right of the rightmost selected tab will be removed.

I think it makes more sense to put this logic inside of getTabsToTheEndFrom, and then you won't have to duplicate some of the logic when setting the contextmenu disabled state.

::: browser/base/content/tabbrowser.js:2556
(Diff revision 1)
> -    let tabs = this.getTabsToTheEndFrom(aTab);
> +    let newRightmostTab = aTab;
> +    if (aTab.multiselected) {
> +      let selectedTabs = this.selectedTabs;
> +      newRightmostTab = selectedTabs[selectedTabs.length - 1];
> +    }
> +
> +    let tabs = this.getTabsToTheEndFrom(newRightmostTab);

`newRightmostTab` sounds awkward in the case of non-multiselected tabs.

What do you think about the following?

```js
let tabs;
if (aTab.multiselected) {
  let selectedTabs = this.selectedTabs;
  let rightmostSelectedTab = selectedTabs[selectedTabs.length - 1];
  tabs = this.getTabsToTheEndFrom(rightmostSelectedTab);
} else {
  tabs = this.getTabsToTheEndFrom(aTab);
}
```

::: browser/base/content/tabbrowser.js:2601
(Diff revision 1)
>    },
>  
>    removeTabs(tabs) {
> +    this._clearMultiSelectionLocked = true;
> +
> +    try {

Please add a comment like so,
// Guarantee that _clearMultiSelectionLocked lock gets released.

::: browser/base/content/tabbrowser.js:3735
(Diff revision 1)
> -  updateActiveTabMultiSelectState() {
> -    if (this.selectedTabs.length == 1) {
> +  AvoidSingleSelectedTab() {
> +    if (this.multiSelectedTabsCount == 1 ) {

How do we get in to this situation?

Is this a symptom of a bug in the two places where this function is called?

Also, this function would be named `avoidSingleSelectedTab` to follow naming conventions in this file.

::: browser/base/content/test/tabs/browser_multiselect_tabs_close_other_tabs.js:92
(Diff revision 1)
>    ok(!tab3.multiselected, "Tab3 is not multiselected");
>    ok(!tab4.multiselected, "Tab4 is not multiselected");
>    ok(tab4.pinned, "Tab4 is pinned");
> -  is(gBrowser.multiSelectedTabsCount, 2, "Two multiselected tabs");
> +  ok(tab5.multiselected, "Tab5 is multiselected");
> +  ok(tab5.pinned, "Tab5 is pinned");
> +  is(gBrowser.multiSelectedTabsCount, 3, "Two multiselected tabs");

Three multiselected tabs

::: browser/base/content/test/tabs/browser_multiselect_tabs_close_tabs_to_the_right.js:73
(Diff revision 1)
> +  ok(tab1.multiselected, "Tab1 is multiselected");
> +  ok(!tab2.multiselected, "Tab2 is not multiselected");
> +  ok(tab3.multiselected, "Tab3 is multiselected");
> +  ok(!tab4.multiselected, "Tab4 is not multiselected");
> +  ok(tab5.multiselected, "Tab5 is multiselected");
> +  is(gBrowser.multiSelectedTabsCount, 3, "Two multiselected tabs");

"Three multiselected tabs"

Comment 5

[Abdoulaye O. LY]

Comment on attachment 8996928 [details]
Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.

https://reviewboard.mozilla.org/r/260270/#review268062

> How do we get in to this situation?
> 
> Is this a symptom of a bug in the two places where this function is called?
> 
> Also, this function would be named `avoidSingleSelectedTab` to follow naming conventions in this file.

Some scenario may lead to only one single tab multi-selected, this is something to avoid (chrome does the same)
Consider 4 tabs A,B,C,D with A having the focus
1. select C with Ctrl
2. Right-click on B and "Close Tabs to The Right"

Expected result
C and D closing
A being the only multi-selected tab, selection should be cleared


Before I though It would happen only with active tabs but I was wrong. I found that it could happen with even a none-focused tab.
For exemple for the menu "Close other tabs", it could happen with a multi-selected pinned tab.
For illustration, consider 4 tabs A,B,C,D with B active
1. pin A and Ctrl-select it
2. Ctrl-select C
3. right-click on D and click "Close Other Tabs"

Expected result
B and C closing
A[pinned] being the only multi-selected tab, selection should be cleared.

Please let me know, if you have more questions.

Comment 6

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

Comment on attachment 8996928 [details]
Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.

https://reviewboard.mozilla.org/r/260270/#review268062

> Some scenario may lead to only one single tab multi-selected, this is something to avoid (chrome does the same)
> Consider 4 tabs A,B,C,D with A having the focus
> 1. select C with Ctrl
> 2. Right-click on B and "Close Tabs to The Right"
> 
> Expected result
> C and D closing
> A being the only multi-selected tab, selection should be cleared
> 
> 
> Before I though It would happen only with active tabs but I was wrong. I found that it could happen with even a none-focused tab.
> For exemple for the menu "Close other tabs", it could happen with a multi-selected pinned tab.
> For illustration, consider 4 tabs A,B,C,D with B active
> 1. pin A and Ctrl-select it
> 2. Ctrl-select C
> 3. right-click on D and click "Close Other Tabs"
> 
> Expected result
> B and C closing
> A[pinned] being the only multi-selected tab, selection should be cleared.
> 
> Please let me know, if you have more questions.

Thanks, that helps. It might be a good idea to include this as a comment for the avoidSingleSelectedTab function.

Comment 7

[Abdoulaye O. LY]

Comment on attachment 8996928 [details]
Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.

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

Comment 8

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

Comment on attachment 8996928 [details]
Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.

https://reviewboard.mozilla.org/r/260270/#review268176

Comment 9

[Abdoulaye O. LY]

Comment on attachment 8996928 [details]
Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.

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

Comment 10

[Abdoulaye O. LY]

Comment on attachment 8996928 [details]
Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.

https://reviewboard.mozilla.org/r/260270/#review268062

> Thanks, that helps. It might be a good idea to include this as a comment for the avoidSingleSelectedTab function.

Yes great idea, I will.

Comment 11

[Abdoulaye O. LY]

Comment on attachment 8996928 [details]
Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260270/diff/3-4/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/078d558d85e9
In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab. r=jaws

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/078d558d85e9

Comment 14

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

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