Closed Bug 1288247 Opened 8 years ago Closed 8 years ago

Synced Tabs sidebar should open new tabs on middle click

Categories

(Firefox :: Sync, defect, P1)

Product:
Firefox
Component:
Sync
Version:
47 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- verified

People

(Reporter: robert.pollak, Assigned: eoger)

References

Details

(Whiteboard: [synced-tabs-ui])

Attachments

(1 file)

Bug 1288247 - Open synced tabs in a new tab on middle click.
58 bytes, text/x-review-board-request
markh
: review+
Details 
Currently, I can only left-click in the synced tabs sidebar. This loads the URL in the currently active tab. Middle-clicking a synced tab does nothing. I suggest opening the URL in a new tab on middle click.
Agreed - the other sidebars all do this. I'm actually surprised we didn't already support that.
OS: Linux → Unspecified
Hardware: x86_64 → Unspecified
Priority: -- → P2
Whiteboard: [synced-tabs-ui]
Priority: P2 → P1
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Comment on attachment 8783129 [details]
Bug 1288247 - Open synced tabs in a new tab on middle click.

https://reviewboard.mozilla.org/r/73072/#review70884

This looks fine to me, although for parity with the bookmarks sidebar, it appears we should also support a middle click on the device offering to open all the tabs for this device (with a dialog checking you really do want to open that many tabs). I'm not too bothered if that is done in this bug or another bug opened for that purpose.
Attachment #8783129 - Flags: review?(markh) → review+
Added shift-click/middle-button click/ctrl-click on the client item to open all the tabs from this client (didn't implement the dialog since we don't have it for bookmarks).
Attachment #8783129 - Flags: review+ → review?(markh)
Added a confirmation dialog when opening a lot of tabs at once.
Comment on attachment 8783129 [details]
Bug 1288247 - Open synced tabs in a new tab on middle click.

https://reviewboard.mozilla.org/r/73072/#review71534

Great, thanks!

::: browser/components/syncedtabs/TabListComponent.js:121
(Diff revision 4)
>      BrowserUITelemetry.countSyncedTabEvent("open", "sidebar");
>    },
>  
> +  onOpenTabs(urls, where, params) {
> +    if(!PlacesUIUtils.confirmOpenInTabs(urls.length, this._window)) {
> +      return;

We should still count these opens in telemetry. Sadly countSyncedTabEvent doesn't take a count, so you probably need to loop.
Attachment #8783129 - Flags: review?(markh) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bbe3a00528ed
Open synced tabs in a new tab on middle click. r=markh
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/f976df7f1eba
Open synced tabs in a new tab on middle click: Add whitespace after 'if' to make eslint happy. r=eslint-fix
Backed out for failing test_TabListComponent.js:

https://hg.mozilla.org/integration/autoland/rev/a862065d5651313075f6f20adde47446abd259b5
https://hg.mozilla.org/integration/autoland/rev/8cfff5c353c7e48dd766b30a46f55ecdd88f29ac

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bbe3a00528ed2b0731854fafc25963f08be9218c
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=2462006&repo=autoland

16:10:36     INFO -  TEST-PASS | browser/components/syncedtabs/test/xpcshell/test_TabListComponent.js | testActions - [testActions : 130] true == true
16:10:36     INFO -  PROCESS | 8950 | *************************
16:10:36     INFO -  PROCESS | 8950 | A coding exception was thrown and uncaught in a Task.
16:10:36     INFO -  PROCESS | 8950 | Full message: TypeError: this._window.getBrowserURL is not a function
16:10:36     INFO -  PROCESS | 8950 | Full stack: onOpenTabs@resource:///modules/syncedtabs/TabListComponent.js:124:31
16:10:36     INFO -  PROCESS | 8950 | testActions@/home/worker/workspace/build/tests/xpcshell/tests/browser/components/syncedtabs/test/xpcshell/test_TabListComponent.js:131:3
16:10:36     INFO -  PROCESS | 8950 | _run_next_test@/home/worker/workspace/build/tests/xpcshell/head.js:1566:9
16:10:36     INFO -  PROCESS | 8950 | do_execute_soon/<.run@/home/worker/workspace/build/tests/xpcshell/head.js:713:9
16:10:36     INFO -  PROCESS | 8950 | _do_main@/home/worker/workspace/build/tests/xpcshell/head.js:210:5
16:10:36     INFO -  PROCESS | 8950 | _execute_test@/home/worker/workspace/build/tests/xpcshell/head.js:545:5
16:10:36     INFO -  PROCESS | 8950 | @-e:1:1
16:10:36     INFO -  PROCESS | 8950 | *************************
Flags: needinfo?(eoger)
Thanks!
Flags: needinfo?(eoger)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9c50e31bc8b6
Open synced tabs in a new tab on middle click. r=markh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9c50e31bc8b6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
No one notice middle click on a client select the last synced tab

TabListComponent.js > onOpenTabs call 'this._window.openUILinkIn(url, where, params);' for each url
when where is a tab we end up selecting each new tab until the last one
Depends on: 1322408
Good catch! I think bug 1322408 should cover it.
I have reproduced this bug with Nightly 50.0a1 (2016-07-20) on Windows 7 , 4 Bit 

This bug's fix is verified with latest Beta 

Build       ID    20161201172143
User     Agent    Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
[bugday-20161207]
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Verified fixed on Windows 7 x64, Ubuntu 14.04 x64 and Mac OSX 10.11 using Firefox 51 Beta 9 (buildID: 20161219140923).
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: