Closed Bug 1472074 Opened 6 years ago Closed 6 years ago

Selection behavior of multiple tabs by Shift-click is incompatible to Google Chrome

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

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

People

(Reporter: yuki, Assigned: ablayelyfondou)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

# Abstract

Shift-click on tabs now selects multiple tabs by range after the bug 1458013.
However, the behavior is different from Google Chrome's one.

# Steps to reproduce

 1. Start Nightly 63.
 2. Go to "about:config" and set "browser.tabs.multiselect" to "true".
 3. Prepare five tabs: A, B, C, D and E.
 4. Activate the tab C. Now you see tabs like:
    - A
    - B
    - C(active)
    - D
    - E
 5. Shift-click on the tab E. Then three tabs C, D, and E are selected.
    Now you see tabs like:
    - A
    - B
    - C(active/highlighted)
    - D(highlighted)
    - E(highlighted)
 6. Shift-click on the tab A.

# Expected result

D and E are dehighlighted and only A, B and C are highlighted, like:

    - A(highlighted)
    - B(highlighted)
    - C(active/highlighted)
    - D
    - E

# Actual result

C and D are not dehighlighted and all of five tabs are highlighted, like:

    - A(highlighted)
    - B(highlighted)
    - C(active/highlighted)
    - D(highlighted)
    - E(highlighted)

# Environment:

I've verified that Google Chrome 67.0.3396.62 produces the expected result I wrote above.

My Firefox is:

 * Version: 63.0a1
 * Build ID: 20180628220051
Please note that Chrome's selection behavior for shift-click is common on multiple GUI applications. For example: Windows Explorer (file selection), Microsoft Excel (cell selection), and Firefox itself (Places Organizer's row selection).
Assignee: nobody → ablayelyfondou
Comment on attachment 8988966 [details]
Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer.

https://reviewboard.mozilla.org/r/254076/#review260906

I don't quite follow the implementation without looking into it in more depth, but the test verifies something behaves differently from the other apps we're aiming to emulate, so I don't think this is quite right yet. I'll be on IRC later today if you want to talk about it there, or you can needinfo/email here or use slack. Sorry for the delay in reviews!

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

This isn't what happens in Chrome... if I switch to tab 1, accel-select tab 3, then switch-select tab 5, tabs 1 *and* 3-5 are selected, not just 3-5.
Attachment #8988966 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8988966 [details]
Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer.

https://reviewboard.mozilla.org/r/254076/#review260906

> This isn't what happens in Chrome... if I switch to tab 1, accel-select tab 3, then switch-select tab 5, tabs 1 *and* 3-5 are selected, not just 3-5.

I tested with Chrome 66, Chrome 67, Chrome 69 (canary) and Windows Explorer but it bevahes the way I implemented it here. which Chrome version are you using?
Comment on attachment 8988966 [details]
Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer.

https://reviewboard.mozilla.org/r/254076/#review260906

You don't need to apologize about the delay and it was already the weekend. Thanks for these reviews.
Comment on attachment 8988966 [details]
Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer.

https://reviewboard.mozilla.org/r/254076/#review261144

(In reply to Abdoulaye O. LY from comment #4)
> Comment on attachment 8988966 [details]
> Bug 1472074 - Fix behavior for multiselection using shift key to match other
> popular apps such as Chrome or Windows Explorer.
> 
> https://reviewboard.mozilla.org/r/254076/#review260906
> 
> > This isn't what happens in Chrome... if I switch to tab 1, accel-select tab 3, then switch-select tab 5, tabs 1 *and* 3-5 are selected, not just 3-5.
> 
> I tested with Chrome 66, Chrome 67, Chrome 69 (canary) and Windows Explorer
> but it bevahes the way I implemented it here. which Chrome version are you
> using?

Uh... I swear that this is what I saw this morning, but I can no longer reproduce it. However, I did notice that what I described happens if you keep accel pressed when selecting tab 5 (so selecting it with accel+shift instead of just shift) and AFAICT from the code, we don't support accel+shift at the same time. Is that planned for another bug? :-)

I still see some other cases that seem to behave subtly differently (tested with Chrome 67.0.3396.99 (Official Build) (64-bit) on macOS). E.g.:

1. open tabs [a,b,c,d]
2. select [d]
3. accel-select [a]
4. shift-select [c]
5. Firefox post-patch: multi-selected: [a,b,c]; current: [a] -- Chrome: multi-selected: [a,b,c]; current: [c]

So the viewport result is different (assuming [a] and [c] have different pages loaded). In fact, it also seems that after step 3 Chrome already switches to [a], and Firefox doesn't - Chrome just seems to make the tab you click on the current tab, even when you use modifier keys.

It's a bit hard to tell for me in Windows explorer when selecting files because the difference between the "current" selected file and the "last" selected file is so small, but I *think* it does what Chrome does.

Is that an intentional difference?

(Do we have UI design documents that detail how this is "supposed" to work? :-) )

::: browser/base/content/tabbrowser.js:3697
(Diff revision 1)
> +    if (this.selectedTabs.length == 1) {
> +      this.clearMultiSelectedTabs();
> +    }
> +  },
> +
> +  switchToAMultiSelectedTab() {

Nit: I would name this `switchToNextMultiSelectedTab` to make it clear that we're not switching to an arbitrary multi-selected tab, but that the "point" of the method is switching from one tab that was just deselected to the next one.

::: browser/base/content/tabbrowser.js:3699
(Diff revision 1)
> +    if (lastMultiSelectedTab != gBrowser.selectedTab) {
> +      gBrowser.selectedTab = gBrowser.lastMultiSelectedTab;

Could just assign `lastMultiSelectedTab` immediately instead of running the getter another time, right?

::: browser/base/content/tabbrowser.xml:2022
(Diff revision 1)
>                if (this == gBrowser.selectedTab) {
> -                gBrowser.selectedTab = gBrowser.lastMultiSelectedTab;
> +                gBrowser.switchToAMultiSelectedTab();
>                }
>              } else if (this != gBrowser.selectedTab) {
>                for (let tab of [this, gBrowser.selectedTab]) {
>                  gBrowser.addToMultiSelectedTabs(tab);

Can we pass the second argument here and then manually call `_setPositionalAttributes`, to avoid running it twice?
Comment on attachment 8988966 [details]
Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer.

https://reviewboard.mozilla.org/r/254076/#review261144

Unfortunately we don't have a documentation on this. This has been discussed on https://mozilla.invisionapp.com/share/4NJ8CFDW9DP#/screens/295638953 and during the first meetting with UX Team. But the idea is to have basically the same behavior as Chrome and other apps except that we don't want the active tab to change whenever a tab is clicked with the modifiers keys. I personally find annoying to switch tab every time it's hightlighted. What do you think? 

Yeah! I didn't know about the Shift + Accel keys combinaison. Indeed, I even tested it with Windows Explorer and it actually works like Chrome. A bug is not filed but I think I will need to. Jared seems not to know this because I asked when working on the range select using Shift key about what to do when both modifiers are held at the same time, his response was (ref: https://bugzilla.mozilla.org/show_bug.cgi?id=1458013#c2):

"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.".
(In reply to Abdoulaye O. LY from comment #7)
> Comment on attachment 8988966 [details]
> Bug 1472074 - Fix behavior for multiselection using shift key to match other
> popular apps such as Chrome or Windows Explorer.
> 
> https://reviewboard.mozilla.org/r/254076/#review261144
> 
> Unfortunately we don't have a documentation on this. This has been discussed
> on https://mozilla.invisionapp.com/share/4NJ8CFDW9DP#/screens/295638953 and
> during the first meetting with UX Team. But the idea is to have basically
> the same behavior as Chrome and other apps except that we don't want the
> active tab to change whenever a tab is clicked with the modifiers keys. I
> personally find annoying to switch tab every time it's hightlighted. What do
> you think? 

I think it's definitely fine for now, but just something to keep in mind. Maybe once people use this more regularly we change our opinion about what is the best behavior. Seems hard to reason about it in advance, apart from avoiding the continuous switching of tabs (which I agree we should avoid).

> Yeah! I didn't know about the Shift + Accel keys combinaison. Indeed, I even
> tested it with Windows Explorer and it actually works like Chrome. A bug is
> not filed but I think I will need to. Jared seems not to know this because I
> asked when working on the range select using Shift key about what to do when
> both modifiers are held at the same time, his response was (ref:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1458013#c2):
> 
> "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.".

OK, let's worry about this in a separate bug. :-)
Comment on attachment 8988966 [details]
Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer.

https://reviewboard.mozilla.org/r/254074/#review261456

::: browser/base/content/tabbrowser.js:3650
(Diff revision 2)
> +
> +  switchToNextMultiSelectedTab() {
> +    let lastMultiSelectedTab = gBrowser.lastMultiSelectedTab;
> +    if (lastMultiSelectedTab != gBrowser.selectedTab) {
> +      gBrowser.selectedTab = lastMultiSelectedTab;
> +      return;

An oversight that just made other tests failing.
Blocks: 1473187
(In reply to :Gijs (he/him) from comment #8)
> I think it's definitely fine for now, but just something to keep in mind.
> Maybe once people use this more regularly we change our opinion about what
> is the best behavior. Seems hard to reason about it in advance, apart from
> avoiding the continuous switching of tabs (which I agree we should avoid).

Absolutely!
Comment on attachment 8988966 [details]
Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer.

https://reviewboard.mozilla.org/r/254076/#review261590

Looks good, thanks!
Attachment #8988966 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8988966 [details]
Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer.

https://reviewboard.mozilla.org/r/254076/#review261768
Hey Gijs, would you like to push this into autoland.
Thanks.
Flags: needinfo?(gijskruitbosch+bugs)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1c3bde759b48a8782e564add7142fbdbe083c4f8 -d fda31bd96fa1: rebasing 471417:1c3bde759b48 "Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer. r=Gijs" (tip)
merging browser/base/content/tabbrowser.js
warning: conflicts while merging browser/base/content/tabbrowser.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(In reply to Mozilla Autoland from comment #15)
> We're sorry, Autoland could not rebase your commits for you automatically.
> Please manually rebase your commits and try again.
> 
> hg error in cmd: hg rebase -s 1c3bde759b48a8782e564add7142fbdbe083c4f8 -d
> fda31bd96fa1: rebasing 471417:1c3bde759b48 "Bug 1472074 - Fix behavior for
> multiselection using shift key to match other popular apps such as Chrome or
> Windows Explorer. r=Gijs" (tip)
> merging browser/base/content/tabbrowser.js
> warning: conflicts while merging browser/base/content/tabbrowser.js! (edit,
> then use 'hg resolve --mark')
> unresolved conflicts (see hg resolve, then hg rebase --continue)

I tried, but it didn't work. Can you push a rebased version? Thanks!
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ablayelyfondou)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/80d8f267abd8
Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/80d8f267abd8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Flags: needinfo?(ablayelyfondou)
Blocks: 1481700
No longer blocks: 1481700
I can reproduce this bug following str from comment 0 with Nightly 63.0a1 (2018-06-28) (64-bit) on Linux (x86_64)

I can verify that this issue is fixed in latest Nightly.

Build ID 	20181015100128
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:64.0) Gecko/20100101 Firefox/64.0
QA Whiteboard: [bugday-20181010]
Verified fixed in latest nightly 65.0a1(2018-11-11) and latest Beta 64.0b8 on Windows 10x64, macOS 10.13 and Ubuntu 16.04x64.

Behavior is now consistent with the behavior from Chrome Browser
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.