1458010 - Add ability to select multiple tabs using Ctrl/Cmd

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

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

This bug should implement the following features:
1. Add a preference to about:config for the feature. The preference can be called `browser.tabs.multiselect` and it should be a boolean that defaults to `false`.
2. When the preference is set to `true`, clicking on a tab while holding Ctrl/Cmd should add the tab to a list of multiselected tabs but should not change which tab is selected (focused).
3. When a tab is in the mulitselect collection, we can bold the text of the tab (this is intermediate styling).
4. Clicking on a tab without pressing Ctrl/Cmd should clear the multiselection.

Comment 1

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

5. If a tab is already in the multiselect collection, clicking on the tab while holding Ctrl/Cmd should remove the tab from the multiselect collection.

Comment 2

[Abdoulaye O. LY]

Attachment: Add ability to select multiple tabs using Ctrl/Cmd

Comment 3

[Abdoulaye O. LY]

Attachment: Add ability to select multiple tabs using Ctrl/Cmd

Comment 4

[Abdoulaye O. LY]

Attachment: Add ability to select multiple tabs using Ctrl/Cmd

Comment 5

[Abdoulaye O. LY]

Attachment: Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

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

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8973924 [details]
Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

https://reviewboard.mozilla.org/r/242266/#review248108


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/tabbrowser.xml:1994
(Diff revision 1)
> +            if (this.multiselected) {
> +              gBrowser.removeFromMultiSelectedTabs(this);
> +            } else {
> +              gBrowser.addToMultiSelectedTabs(this);
> +            }
> +          } else if (gBrowser._multiSelectedTabs.length > 0){

Error: Missing space before opening brace. [eslint: space-before-blocks]

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:14
(Diff revision 1)
> +  if (AppConstants.platform == "macosx") {
> +    options = { metaKey: options.ctrlKey };
> +  }
> +  EventUtils.synthesizeMouseAtCenter(target, options);
> +  // EventUtils.synthesizeMouseAtCenter(target, options);
> +  return await promise;

Error: Redundant use of `await` on a return value. [eslint: no-return-await]

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:47
(Diff revision 1)
> +  const tab = await addTab();
> +
> +  await triggerClickOn(tab, { ctrlKey: true });
> +  ok(tab.multiselected && mSelectedTabs.includes(tab),
> +    "Tab should be (multi) selected after click");
> +  ok(gBrowser.selectedTab != tab, "Focused tab doesn't change")

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:84
(Diff revision 1)
> +  ok(gBrowser._multiSelectedTabs.length == 0, "Selection is cleared");
> +
> +  BrowserTestUtils.removeTab(tab1);
> +  BrowserTestUtils.removeTab(tab2);
> +  BrowserTestUtils.removeTab(tab3);
> +});

Error: Newline required at end of file but not found. [eslint: eol-last]

Comment 7

[Abdoulaye O. LY]

Comment on attachment 8973924 [details]
Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

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

Comment 8

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

Comment on attachment 8973924 [details]
Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

https://reviewboard.mozilla.org/r/242266/#review248296

Looks good! Just a couple things that we can do that should help to clean it up a bit more before we land this.

::: browser/base/content/tabbrowser.js:137
(Diff revision 2)
>      "didStartLoadSinceLastUserTyping", "audioMuted"
>    ],
>  
>    _removingTabs: [],
>  
> +  _multiSelectedTabs: [],

We should use a WeakMap instead of an array here. The WeakMap will allow quicker deletion, remove the needto check for duplicates, and won't require us to clean up references to the tab.

The key for the WeakMap can be the tab.permanentKey.

::: browser/base/content/tabbrowser.xml:1969
(Diff revision 2)
> +          const multiSelectKeyDown = Services.prefs.getBoolPref("browser.tabs.multiselect")
> +            && (AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey);

nit, please put the && at the end of the line instead of the start. This matches how it is used in surrounding code.

::: browser/base/content/tabbrowser.xml:1970
(Diff revision 2)
>            this.style.MozUserFocus = "ignore";
> -        } else if (this.mOverCloseButton ||
> -                   this._overPlayingIcon) {
> +        } else {
> +          // When browser.tabs.multiselect config is set to false,
> +          // then we ignore the state of multi-selection keys (Ctrl/Cmd).
> +          const multiSelectKeyDown = Services.prefs.getBoolPref("browser.tabs.multiselect")
> +            && (AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey);

Use event.getModifierState("Accel"), which will do the right thing based on the OS. See https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/getModifierState#Accel_virtual_modifier for more information.

::: browser/base/content/tabbrowser.xml:1973
(Diff revision 2)
> +                   this._overPlayingIcon ||
> +                   multiSelectKeyDown) {

The indentation here is broken.

::: browser/base/content/tabbrowser.xml:1987
(Diff revision 2)
>          this.style.MozUserFocus = "";
>        </handler>
>  
>        <handler event="click" button="0"><![CDATA[
> +        if (Services.prefs.getBoolPref("browser.tabs.multiselect")) {
> +          const oneItemSelectKeyDown = AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey;

getModifierState here too.

I think the name `tabSelectionToggled` is better. What do you think?

::: browser/base/content/tabbrowser.xml:1993
(Diff revision 2)
> +          if (oneItemSelectKeyDown) {
> +            if (this.multiselected) {
> +              gBrowser.removeFromMultiSelectedTabs(this);
> +            } else {
> +              gBrowser.addToMultiSelectedTabs(this);
> +            }

We should return early after the `else` here. Otherwise ctrl-clicking on a tab's aduio button would select the tab as well as toggle the audio state.

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Test code is normally licensed under Public Domain. You can remove the license header here, and by default the test will fall under Public Domain based on the Licensing of Mozilla Code section of https://www.mozilla.org/en-US/MPL/license-policy/

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:24
(Diff revision 2)
> +  await BrowserTestUtils.browserLoaded(browser);
> +  return tab;
> +}
> +
> +// Let's make sure multiselection doesn't work when pref is not set
> +add_task(async function click() {

Let's use clickWithoutPrefSet() as the function name, since it's printed in the test log and will be more helpful when debugging.

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:25
(Diff revision 2)
> +  return tab;
> +}
> +
> +// Let's make sure multiselection doesn't work when pref is not set
> +add_task(async function click() {
> +  let tab = gBrowser.selectedTab;

This tab is not removed at the end of this subtest.

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:31
(Diff revision 2)
> +  let mSelectedTabs = gBrowser._multiSelectedTabs;
> +
> +  await triggerClickOn(tab, { ctrlKey: true });
> +  ok(!tab.multiselected && !mSelectedTabs.includes(tab),
> +    "Multi-select tab doesn't work when multi-select pref is not set");
> +});

Can you add a check that the selectedTab changed?

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:50
(Diff revision 2)
> +    "Tab should be (multi) selected after click");
> +  ok(gBrowser.selectedTab != tab, "Focused tab doesn't change");
> +
> +  await triggerClickOn(tab, { ctrlKey: true });
> +  ok(!tab.multiselected && !mSelectedTabs.includes(tab), "Tab is unselected");
> +

Can you add another check here that the selectedTab was still unchanged?

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:70
(Diff revision 2)
> +  // We select tab1 and tab2 with ctrl key down
> +  await triggerClickOn(tab1, { ctrlKey: true });
> +  await triggerClickOn(tab2, { ctrlKey: true });
> +
> +  ok(tab1.multiselected, "Tab is (multi) selected");
> +  ok(tab1.multiselected, "Tab is (multi) selected");

This should be checking `tab2`, not `tab1`.

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:74
(Diff revision 2)
> +  ok(tab1.multiselected, "Tab is (multi) selected");
> +  ok(tab1.multiselected, "Tab is (multi) selected");
> +  ok(gBrowser._multiSelectedTabs.length == 2, "Two tabs selected");
> +
> +  // We select tab3 with Ctrl key up
> +  await triggerClickOn(tab3, { ctrlKey: false });

Can you add a check that the tab was not selected before the click, and that it was selected after the click?

Comment 9

[Abdoulaye O. LY]

Comment on attachment 8973924 [details]
Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

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

Comment 10

[Abdoulaye O. LY]

Comment on attachment 8973924 [details]
Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

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

Comment 11

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

Comment on attachment 8973924 [details]
Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

https://reviewboard.mozilla.org/r/242266/#review248598

Looks good! Can you make the following changes and then mark each issue on MozReview as "fixed"? I've pushed your patch to the tryserver to make sure that the tests pass on all platforms. Once you make the following changes and mark the issues as fixed then we can get this landed.

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:27
(Diff revision 4)
> +
> +  ok(gBrowser.selectedTab != tab, "Tab doesn't have focus");
> +
> +  await triggerClickOn(tab, { ctrlKey: true });
> +
> +  ok(!tab.multiselected && !mSelectedTabs.has(tab),

is(!tab.multiselected, !mSelectedTabs.has(tab), "message");

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:29
(Diff revision 4)
> +
> +  await triggerClickOn(tab, { ctrlKey: true });
> +
> +  ok(!tab.multiselected && !mSelectedTabs.has(tab),
> +    "Multi-select tab doesn't work when multi-select pref is not set");
> +  ok(gBrowser.selectedTab == tab,

is(gBrowser.selectedTab, tab, "message");

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:48
(Diff revision 4)
> +  const initialFocusedTab = gBrowser.selectedTab;
> +  const tab = await addTab();
> +
> +  await triggerClickOn(tab, { ctrlKey: true });
> +  ok(tab.multiselected && mSelectedTabs.has(tab), "Tab should be (multi) selected after click");
> +  ok(gBrowser.selectedTab != tab, "Multi-selected tab is not focused");

isnot(gBrowser.selectedTab, tab, "message");

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:49
(Diff revision 4)
> +  const tab = await addTab();
> +
> +  await triggerClickOn(tab, { ctrlKey: true });
> +  ok(tab.multiselected && mSelectedTabs.has(tab), "Tab should be (multi) selected after click");
> +  ok(gBrowser.selectedTab != tab, "Multi-selected tab is not focused");
> +  ok(gBrowser.selectedTab == initialFocusedTab, "Focused tab doesn't change");

is(gBrowser.selectedTab, initialFocusedTab, "message");

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:53
(Diff revision 4)
> +  ok(gBrowser.selectedTab != tab, "Multi-selected tab is not focused");
> +  ok(gBrowser.selectedTab == initialFocusedTab, "Focused tab doesn't change");
> +
> +  await triggerClickOn(tab, { ctrlKey: true });
> +  ok(!tab.multiselected && !mSelectedTabs.has(tab), "Tab is not selected anymore");
> +  ok(gBrowser.selectedTab == initialFocusedTab, "Focused tab still doesn't change");

is()

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:75
(Diff revision 4)
> +  await triggerClickOn(tab1, { ctrlKey: true });
> +  await triggerClickOn(tab2, { ctrlKey: true });
> +
> +  ok(tab1.multiselected && gBrowser._multiSelectedTabsMap.has(tab1), "Tab1 is (multi) selected");
> +  ok(tab2.multiselected && gBrowser._multiSelectedTabsMap.has(tab2), "Tab2 is (multi) selected");
> +  ok(gBrowser.multiSelectedTabsCount() == 2, "Two tabs selected");

is()

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:76
(Diff revision 4)
> +  await triggerClickOn(tab2, { ctrlKey: true });
> +
> +  ok(tab1.multiselected && gBrowser._multiSelectedTabsMap.has(tab1), "Tab1 is (multi) selected");
> +  ok(tab2.multiselected && gBrowser._multiSelectedTabsMap.has(tab2), "Tab2 is (multi) selected");
> +  ok(gBrowser.multiSelectedTabsCount() == 2, "Two tabs selected");
> +  ok(tab3 != gBrowser.selectedTab, "Tab3 doesn't have focus");

isnot()

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:83
(Diff revision 4)
> +  info("We select tab3 with Ctrl key up");
> +  await triggerClickOn(tab3, { ctrlKey: false });
> +
> +  ok(!tab1.multiselected, "Tab1 is unselected");
> +  ok(!tab2.multiselected, "Tab2 is unselected");
> +  ok(gBrowser.multiSelectedTabsCount() == 0, "Selection is cleared");

is()

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_Ctrl.js:84
(Diff revision 4)
> +  await triggerClickOn(tab3, { ctrlKey: false });
> +
> +  ok(!tab1.multiselected, "Tab1 is unselected");
> +  ok(!tab2.multiselected, "Tab2 is unselected");
> +  ok(gBrowser.multiSelectedTabsCount() == 0, "Selection is cleared");
> +  ok(tab3 == gBrowser.selectedTab, "Tab3 has focus");

is()

Comment 12

[Abdoulaye O. LY]

Comment on attachment 8973924 [details]
Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

https://reviewboard.mozilla.org/r/242266/#review248598

> is(!tab.multiselected, !mSelectedTabs.has(tab), "message");

I don't think here we should use "is()" because when both "!tab.multiselected" and "!mSelectedTabs.has(tab)" return false then the test with "is()" will pass when it should not.

Comment 13

[Abdoulaye O. LY]

Comment on attachment 8973924 [details]
Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

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

Comment 14

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

Comment on attachment 8973924 [details]
Bug 1458010 - Add ability to select multiple tabs using Ctrl/Cmd.

https://reviewboard.mozilla.org/r/242266/#review248598

> I don't think here we should use "is()" because when both "!tab.multiselected" and "!mSelectedTabs.has(tab)" return false then the test with "is()" will pass when it should not.

Yes, you're right. Thanks for catching this.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4edd0b644217
Add ability to select multiple tabs using Ctrl/Cmd. r=jaws

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4edd0b644217

Comment 17

[Oriol Brufau [:Oriol]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> We should use a WeakMap instead of an array here. The WeakMap will allow
> quicker deletion, remove the needto check for duplicates, and won't require
> us to clean up references to the tab.

Why not a WeakSet?

Comment 18

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

Indeed, a WeakSet here should work as we're not storing any data as the value in the WeakMap entries. When switching to a WeakSet we would need to also change from using nondeterministicGetWeakMapKeys to nondeterministicGetWeakSetKeys.

Oriol, is this something you would like to do? Can you file a bug for this?

Comment 19

[Oriol Brufau [:Oriol]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Indeed, a WeakSet here should work as we're not storing any data as the
> value in the WeakMap entries. When switching to a WeakSet we would need to
> also change from using nondeterministicGetWeakMapKeys to
> nondeterministicGetWeakSetKeys.
> 
> Oriol, is this something you would like to do? Can you file a bug for this?

OK, filed bug 1466678, the patch should be trivial.

Comment 20

[5silentrain]

Do not forget about the Shift key to select a group of tabs, like in Chrome. This is very important!

Comment 21

[Abdoulaye O. LY]

(In reply to 5silentrain from comment #20)
> Do not forget about the Shift key to select a group of tabs, like in Chrome.
> This is very important!

Yes, it's already implemented, see bug 1458013.

Comment 22

[5silentrain]

Thank you, guys! 

Comment 23

[Vlad Lucaci, Desktop QA (:vlucaci)]

Verified fixed that this implementation is present on the latest Nightly Nightly 65.0a1(2018-11-22)and the latest Beta 64.0b11 as well .