1458013 - Add ability to select multiple tabs using Shift

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, enhancement, P2)

Description

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

Following up to bug 1458010, we should add ability to select a range of tabs using the Shift key.

If no items are in the multiselect collection:
Holding Shift and clicking on a tab should add the current selected (focused) tab, the tab that was just clicked on, and all tabs between them to the multiselect collection. The currently selected (focused) tab should not change.

If there are items in the multiselect collection:
Holding Shift and clicking on a tab should add all tabs between the last tab that was added to the collection and the tab that was clicked on, inclusively. The currently selected (focused) tab should not change.

Shift cannot be used to remove tabs from the collection, though Ctrl/Cmd will offer that ability.

Comment 1

[Abdoulaye O. LY]

When Ctrl/Cmd and shift are pressed at the same time when selecting a tab, which one should we take into account?

Comment 2

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

Shift should get higher precedence than Ctrl/Cmd. If Ctrl/Cmd and Shift are pressed at the same time, we should treat this as though just Shift is pressed. This matches the behavior that I see on Chrome version 68.0.3423.1 (Official Build) canary-dcheck (32-bit) on Windows 10.

Comment 3

[Abdoulaye O. LY]

Attachment: Bug 1458013 - Add ability to select a range of tabs using Shift.

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

Comment 4

[Abdoulaye O. LY]

Comment on attachment 8976047 [details]
Bug 1458013 - Add ability to select a range of tabs using Shift.

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

Comment 5

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

Comment on attachment 8976047 [details]
Bug 1458013 - Add ability to select a range of tabs using Shift.

https://reviewboard.mozilla.org/r/244244/#review250276

Looks good. Just some minor changes to make here and there then I think this will be ready. Nice work!

::: browser/base/content/tabbrowser.js:3638
(Diff revision 2)
>  
> +  /**
> +   * Adds two given tabs and all tabs between them into the (multi) selected tabs collection
> +   */
> +  addRangeToMultiSelectedTabs(aTab1, aTab2) {
> +    // Let's avoid going through all the heavy process bellow when the same

below*

::: browser/base/content/tabbrowser.js:3649
(Diff revision 2)
> +
> +    const tabs = [...this.tabs];
> +    const indexOfTab1 = tabs.indexOf(aTab1);
> +    const indexOfTab2 = tabs.indexOf(aTab2);
> +
> +    const [lowerIndex, greaterIndex] = indexOfTab1 < indexOfTab2 ?

lowerIndex matches better with higherIndex
lesserIndex matches better with greaterIndex

Let's use lowerIndex and higherIndex here :)

::: browser/base/content/tabbrowser.js:3684
(Diff revision 2)
>    },
>  
> +  get lastMultiSeletectedTab() {
> +    const selectedTabs = ChromeUtils.nondeterministicGetWeakMapKeys(this._multiSelectedTabsMap)
> +      .filter(tab => tab.isConnected);
> +    return selectedTabs[selectedTabs.length - 1];

I don't think it's safe to assume that the order returned from nondeterminisiticGetWeakMapKeys will match the insert order.

We can store a weak reference to the last multiselected tab, and then update it each time we add a new tab.

You can use Cu.getWeakReference to get a weak reference to the tab.

See https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/browser/base/content/browser.js#8304 for an example.

If the weak reference returns null or if it refers to a tab that is no longer in the selection (such as if the user Ctrl-clicks on a tab to remove it from the multiselection), then we should fallback to the currently selected tab.

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:3
(Diff revision 2)
> +const PREF_MULTISELECT_TABS = "browser.tabs.multiselect";
> +
> +async function triggerClickOn(target, options) {

Can you move this function to head.js so it doesn't get duplicated for each of your test files? You can also move addTab().

Note, this doesn't need to be async since there is no await inside of it. Callers of the function can still await on the result because it returns a promise.

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:40
(Diff revision 2)
> +    info("Click on tab3 while holding shift key");
> +    await BrowserTestUtils.switchTab(gBrowser, () => {
> +        triggerClickOn(tab3, { shiftKey: true });
> +    });
> +
> +    ok(!tab1.multiselected && !mSelectedTabs.has(tab1), "Ta1 is not multi-selected");

Tab1*

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:42
(Diff revision 2)
> +        triggerClickOn(tab3, { shiftKey: true });
> +    });
> +
> +    ok(!tab1.multiselected && !mSelectedTabs.has(tab1), "Ta1 is not multi-selected");
> +    ok(!tab2.multiselected && !mSelectedTabs.has(tab2), "Tab2 is not multi-selected ");
> +    ok(!tab3.multiselected && !mSelectedTabs.has(tab3), "Tab3 is not selected");

The message should match the ones above, so say "Tab3 is not multi-selected"

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:51
(Diff revision 2)
> +    BrowserTestUtils.removeTab(tab1);
> +    BrowserTestUtils.removeTab(tab2);
> +    BrowserTestUtils.removeTab(tab3);
> +});
> +
> +add_task(async function noItemsInTheCollection() {

noItemsInTheCollectionBeforeShiftClicking

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:52
(Diff revision 2)
> +    await SpecialPowers.pushPrefEnv({
> +        set: [
> +            [PREF_MULTISELECT_TABS, true]

pushPrefEnv should keep the pref set until the test file has finished, so you shouldn't need to set it within each task.

It is common to have a separate task that calls pushPrefEnv to make this clearer. For example, after your prefNotSet task, you could have a new task that is simply:
```
add_task(async function setPref() {
  await SpecialPowers.pushPrefEnv({
    ...
  });
};
```

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:74
(Diff revision 2)
> +    info("Click on tab3 while holding shift key");
> +    await triggerClickOn(tab3, { shiftKey: true });
> +
> +    ok(tab1.multiselected && mSelectedTabs.has(tab1), "Tab1 is multi-selected");
> +    ok(tab2.multiselected && mSelectedTabs.has(tab2), "Tab2 is multi-selected ");
> +    ok(tab3.multiselected && mSelectedTabs.has(tab3), "Tab3 is selected");

same comment here, "Tab3 is multi-selected"

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:85
(Diff revision 2)
> +    BrowserTestUtils.removeTab(tab2);
> +    BrowserTestUtils.removeTab(tab3);
> +    BrowserTestUtils.removeTab(tab4);
> +});
> +
> +add_task(async function itemsInTheCollection() {

itemsInTheCollectionBeforeShift

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:99
(Diff revision 2)
> +    let tab3 = await addTab();
> +    let tab4 = await addTab();
> +
> +    let mSelectedTabs = gBrowser._multiSelectedTabsMap;
> +
> +    await triggerClickOn(tab1, {});

This should use `await BrowserTestUtils.switchTab(gBrowser, () => triggerClickOn(tab1, {}));`

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:104
(Diff revision 2)
> +    await triggerClickOn(tab2, {ctrlKey: true});
> +    is(gBrowser.selectedTab, tab1, "Tab1 still has focus");
> +    is(gBrowser.multiSelectedTabsCount(), 1, "One tab is selected");

Can you add a fifth tab to this test?

Since tab1 has focus, we should do the ctrl-click on tab3, leaving tab2 to not be focused or multi-selected. Then do a shift-click on tab5, so we end up with:

tab1: focused but not multi-selected
tab2: not multi-selected
tab3: multi-selected
tab4: multi-selected
tab5: multi-selected

This will show that the shift-selection starts from the last selected tab and not the currently focused tab.

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:107
(Diff revision 2)
> +    is(gBrowser.multiSelectedTabsCount(), 0, "No tab is selected");
> +
> +    await triggerClickOn(tab2, {ctrlKey: true});
> +    is(gBrowser.selectedTab, tab1, "Tab1 still has focus");
> +    is(gBrowser.multiSelectedTabsCount(), 1, "One tab is selected");
> +    ok(tab2.multiselected && mSelectedTabs.has(tab2), "Tab2 is selected");

"Tab2 is multi-selected"

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js:113
(Diff revision 2)
> +    ok(!tab1.multiselected && !mSelectedTabs.has(tab1), "Tab1 is not selected");
> +    ok(tab2.multiselected && mSelectedTabs.has(tab2), "Tab2 is selected ");
> +    ok(tab3.multiselected && mSelectedTabs.has(tab3), "Tab3 is selected");
> +    ok(tab4.multiselected && mSelectedTabs.has(tab4), "Tab4 is  selected");
> +    is(gBrowser.multiSelectedTabsCount(), 3, "Three tabs are selected");

For all of these please use "multi-selected" instead of "selected", since it can be confusing when we also use gBrowser.selectedTab.

Note, please also fix up the whitespace inside of the strings. The Tab2 string has an extra space at the end of the string, and Tab4 has an extra space before the word "selected".

Comment 6

[Abdoulaye O. LY]

Comment on attachment 8976047 [details]
Bug 1458013 - Add ability to select a range of tabs using Shift.

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

Comment 7

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

Comment on attachment 8976047 [details]
Bug 1458013 - Add ability to select a range of tabs using Shift.

https://reviewboard.mozilla.org/r/244244/#review250588

Looks good. Just a couple minor things I noticed below. I'll push this to tryserver now to make sure that it passes on all platforms.

::: browser/base/content/tabbrowser.js:3640
(Diff revision 3)
> +    for (let i = lowerIndex; i <= higherIndex; i++) {
> +      this.addToMultiSelectedTabs(tabs[i]);

The TabHiding webextension API allows extensions to hide tabs, so we should check that a tab is not hidden before adding it to the collection.

You can check if `tab.hidden` is true before adding the tab.

See https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/browser/base/content/tabbrowser.js#3361 for the implementation of hideTab, and above that is the implementation of showTab.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/hide explains part of the API.

::: browser/base/content/tabbrowser.js:3669
(Diff revision 3)
>      return ChromeUtils.nondeterministicGetWeakMapKeys(this._multiSelectedTabsMap)
>        .filter(tab => tab.isConnected)
>        .length;
>    },
>  
> +  get lastMultiSeletectedTab() {

spelling error, lastMultiSelectedTab

::: browser/base/content/tabbrowser.js:3677
(Diff revision 3)
> +      return tab;
> +    }
> +    return gBrowser.selectedTab;
> +  },
> +
> +  set lastMultiSeletectedTab(aTab) {

lastMultiSelectedTab

Comment 8

[Abdoulaye O. LY]

Comment on attachment 8976047 [details]
Bug 1458013 - Add ability to select a range of tabs using Shift.

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

Comment 9

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

Comment on attachment 8976047 [details]
Bug 1458013 - Add ability to select a range of tabs using Shift.

https://reviewboard.mozilla.org/r/244244/#review250832

Looks great! Thanks for adding the test that covers the hidden tab :)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f58f88669083
Add ability to select a range of tabs using Shift. r=jaws

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f58f88669083

Comment 12

[YUKI "Piro" Hiroshi]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> If there are items in the multiselect collection:
> Holding Shift and clicking on a tab should add all tabs between the last tab
> that was added to the collection and the tab that was clicked on,
> inclusively. The currently selected (focused) tab should not change.

Which is the existing implementation this behavior respects? I've tried Shift-click operation on LibreOffice Calc and Windows Explorer (verified on WIndows 10), but both they look different from Firefox's behavior: they clear old selection in the case. For example, assume that there are 5 cells A, B, C, D, E and C is the active:

 1. A, B, [C(active)], D, E
 2. (Shift-click E)
 3. A, B, [C(active, selected)], [D(selected)], [E(selected)]
 4. (Shift-click A)
 5. [A(selected)], [B(selected)], [C(active, selected)], D, E

If any existing application (especially) works like Firefox, I think there is enough reason to respect the behavior. Otherwise, I believe that Firefox should respect major existing behavior about Shift-click for better user experience.

Comment 13

[YUKI "Piro" Hiroshi]

I've filed a new bug 1472074 based on my previous comment.

Comment 14

[Abdoulaye O. LY]

(In reply to (away 7/1-7/7) Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Shift should get higher precedence than Ctrl/Cmd. If Ctrl/Cmd and Shift are
> pressed at the same time, we should treat this as though just Shift is
> pressed. This matches the behavior that I see on Chrome version 68.0.3423.1
> (Official Build) canary-dcheck (32-bit) on Windows 10.

This not valid now, see bug 1473187 for further details.

(In reply to (away 7/1-7/7) Jared Wein [:jaws] (please needinfo? me) from comment #0)
> If there are items in the multiselect collection:
> Holding Shift and clicking on a tab should add all tabs between the last tab
> that was added to the collection and the tab that was clicked on,
> inclusively. The currently selected (focused) tab should not change.
> 
> Shift cannot be used to remove tabs from the collection, though Ctrl/Cmd
> will offer that ability.

This is also not valid now and is fixed with bug 1472074.

Comment 15

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Verified fixed in latest nightly 65.0a1(2018-11-07) and latest Beta 64.0b7.