Closed Bug 1477780 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P3)

Product:
Firefox
Component:
Tabbed Browser
Version:
63 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed
firefox64 --- verified
firefox65 --- verified

People

(Reporter: dakhmedova, Assigned: ablayelyfondou)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
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.
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?
Flags: needinfo?(amlee)
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.
Flags: needinfo?(amlee)
Assignee: nobody → ablayelyfondou
Status: NEW → ASSIGNED
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"
Attachment #8996928 - Flags: review?(jaws) → review-
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 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 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
Attachment #8996928 - Flags: review?(jaws) → review+
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.
Pushed by jwein@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/078d558d85e9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
status-firefox64: --- → verified
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.
Status: RESOLVED → VERIFIED
status-firefox65: --- → verified
You need to log in before you can comment on or make changes to this bug.