Closed
Bug 677372
Opened 13 years ago
Closed 8 years ago
Send Tab to Device
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
Tracking
()
VERIFIED
FIXED
Firefox 50
Tracking | Status | |
---|---|---|
firefox50 | --- | verified |
People
(Reporter: gps, Assigned: eoger)
References
Details
(Whiteboard: [sync:tabs] [send-tab])
Attachments
(7 files, 6 obsolete files)
This bug tracks the UX design for the "Send Tab/URI to Device" feature of Sync. Current list of questions are at https://wiki.mozilla.org/Services/Sync/Push_to_device
Comment 1•13 years ago
|
||
I have a proposal, see : http://www.hotimg.com/image/4fXEcbd
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 2•12 years ago
|
||
Is this the bug for the Firefox desktop feature? This also needs a new assignee - Alex Faaborg will not work on this anymore, I guess.
Comment 3•12 years ago
|
||
Thomas: yes, this is for the UI for sending on desktop.
Assignee: faaborg → nobody
Updated•12 years ago
|
Whiteboard: [sync:tabs]
Comment 4•12 years ago
|
||
Might be worth adding this to the URL: https://github.com/mozilla-services/firefox-send-tab-to-device Also the whiteboard should probably read: parity-mobile
Comment 5•12 years ago
|
||
I like how the "send to device / playlist" works in the new iTunes 11, Firefox could easily adopt this style: When dragging a tab, a sidebar automatically appears with the drop targets (devices), it then automatically disappears when the tab was dropped. I think it's a rather good, discoverable and straighforward UX, which does not require more than one action (drag-n-drop). The list of devices can be in fixed order or rank the devices concerning on how often and recently they were used as drop targets (with automatically collapsing devices to a compact styling (e. g. half height of an entry of a "popular" device) when they are rarely or never used).
Updated•10 years ago
|
Component: Firefox Sync: UI → Sync
Product: Mozilla Services → Firefox
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•10 years ago
|
Flags: qe-verify-
Whiteboard: [sync:tabs] → [sync:tabs] [ux]
Updated•9 years ago
|
Priority: -- → P4
Comment 8•9 years ago
|
||
To me this bug is blocked by development on Maslaney's great Share plane work. I can't find the bug though!
Comment 9•9 years ago
|
||
Ryan, is the shareplane work still ongoing? If not, what do you think about adding this capability to the "synced tabs" panel?
Flags: needinfo?(rfeeley)
Comment 10•9 years ago
|
||
I really hope the shareplane work is ongoing, it would make the mobile and desktop experiences more homogeneous.
Comment 11•8 years ago
|
||
Could it be as simple as this? I can't think of any edge cases that need UI.
Flags: needinfo?(rfeeley) → needinfo?(markh)
Comment 12•8 years ago
|
||
I think that's fine as a first cut and we can look into the shareplane etc later. Edwin, I guess we need a bug on file for the implementation of this and it added to our current work, but we can call this bug done, thanks! (I think the ordering of the devices might be an open question, but one we can address in the implementation bug - off the top of my head, I guess we sort by most recently synced)
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(markh) → needinfo?(edwong)
Resolution: --- → FIXED
Comment 13•8 years ago
|
||
The same contextual sub-menu could also be included at the page level like this. Thoughts Mark?
Flags: needinfo?(markh)
Comment 14•8 years ago
|
||
I assume we could also do this with a selected link?
Comment 15•8 years ago
|
||
Attachment #8748831 -
Attachment is obsolete: true
Comment 16•8 years ago
|
||
Attachment #8748830 -
Attachment is obsolete: true
Comment 17•8 years ago
|
||
(added "Tab" to "Send Tab to Device")
Attachment #8748291 -
Attachment is obsolete: true
Comment 18•8 years ago
|
||
The page and link context menus look great, Ryan. Without some kind of indicator, it's hard to know that I can do things by right-clicking the tab, though I suspect it wouldn't hurt to have it in all three places. Right-clicking the page seems "easier" (I don't know if that makes sense) than remembering that the context menu is on the tab.
Comment 19•8 years ago
|
||
I don't imagine we have telemetry on these menus, but I'm guessing that the page context menu is an order of magnitude more popular than the tab context menu.
Comment 20•8 years ago
|
||
Yeah, my gut feel is that we don't really need this on the tab itself and the "page" version is where people would be expecting to find it, but I don't see any reason we can't have it on all 3 if that tickles the fluffy belly of the UX team...
Flags: needinfo?(markh)
Comment 21•8 years ago
|
||
Attachment #8748848 -
Attachment is obsolete: true
Comment 22•8 years ago
|
||
Add "All devices" below with horizontal rule.
Attachment #8748854 -
Attachment is obsolete: true
Comment 23•8 years ago
|
||
Add "All devices" at bottom with horizontal rule.
Attachment #8748849 -
Attachment is obsolete: true
Comment 24•8 years ago
|
||
Add "tab" word.
Comment 25•8 years ago
|
||
Moved item higher up in the tab context menu
Assignee | ||
Comment 26•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60044/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60044/
Attachment #8763941 -
Flags: review?(markh)
Assignee | ||
Updated•8 years ago
|
Attachment #8763941 -
Flags: review?(markh) → feedback?(markh)
Assignee | ||
Comment 27•8 years ago
|
||
rfeeley: here's a build with this patch http://archive.mozilla.org/pub/firefox/try-builds/eoger@mozilla.com-569a6c1c2c02e50e22a57195fb53f34cda9ef50f/try-macosx64/firefox-50.0a1.en-US.mac.dmg
Comment 28•8 years ago
|
||
Comment on attachment 8763941 [details] Bug 677372 - Send Tab/Page/Link to device. https://reviewboard.mozilla.org/r/60044/#review57558 ::: browser/base/content/browser-fxaccounts.js:397 (Diff revision 1) > + > + // "All devices" menu item > + const separator = document.createElement("menuseparator"); > + fragment.appendChild(separator); > + const allDevicesLabel = this.strings.GetStringFromName("sendTabToAllDevices.menuitem"); > + const clientsList = clients.map(client => client.id).join(this.CLIENTS_IDS_SEPARATOR); IIUC this will break if a user adds a comma to their device name? It looks easiest to just use an empty string for "all" and have the handler code re-fetch the list of IDs (and maybe not using the "id" attribute is more appropriate too?) ::: browser/base/content/browser.js:7571 (Diff revision 1) > this._updateToggleMuteMenuItem(this.contextTab); > > this.contextTab.addEventListener("TabAttrModified", this, false); > aPopupMenu.addEventListener("popuphiding", this, false); > + > + const sendTabToDevice = document.getElementById("context_sendTabToDevice"); Aren't these elements going to return null in non-nightly builds? Also, I think it makes some sense to have this in browser-fxaccounts.js - ie, here we'd just say |gFxAccounts.updateTabContextMenu(aPopupMenu)| ::: browser/base/content/browser.js:7581 (Diff revision 1) > + sendTabToDevice.hidden = !show; > + separator.hidden = !show; > + } > + > + showSendTabToDevice(); > + fxAccounts.accountStatus().then(exists => { I think we can probably skip the accountStatus check here and rely on the clientsEngine.stats check (although I think you should also change use of clientsEngine.stats to the new getter you added, so this code and the handler code uses the same abstraction) I was thinking that maybe we should make the check also include the "verified" and the "needs reauth" state, but I don't think that's strictly necessary as IIUC, while in that state the command will still work, just not immediately (ie, it will end up working correctly once the user corrects the account state. ::: browser/base/content/nsContextMenu.js:580 (Diff revision 1) > let popup = document.getElementById("fill-login-popup"); > let insertBeforeElement = document.getElementById("fill-login-no-logins"); > popup.insertBefore(fragment, insertBeforeElement); > }, > > + initSyncItems: function() { Similarly to above, I think it might be better to have the body of this in gFxAccounts - something like |gFxAccounts.initPageContextMenu()| - and similarly use the new getter (and maybe even have a gFxAccounts helper |_shouldShowSendToDevice()| or similar - althouhg this one looks a little tricker - you'd probably need to pass |this| and I'm not sure if that is an improvement or not. What do you think? ::: browser/base/content/nsContextMenu.js:599 (Diff revision 1) > + > + showMenuItems(); > + fxAccounts.accountStatus().then(exists => { > + showSendLink = exists && showSendLink; > + showSendPage = exists && showSendPage; > + showMenuItems(); It appears that this will possibly allow us to have "Send Page" and "Send Link" at the same time - is that intended? ::: browser/locales/en-US/chrome/browser/browser.dtd:47 (Diff revision 1) > <!ENTITY unpinTab.accesskey "b"> > +<!ENTITY sendTabToDevice.label "Send Tab to Device"> > +<!ENTITY sendTabToDevice.accesskey "D"> > +<!ENTITY sendPageToDevice.label "Send Page to Device"> > +<!ENTITY sendPageToDevice.accesskey "D"> > +<!ENTITY sendLinkToDevice.label "Send Link to Device"> As above, if it is the intent that "Send Link" and "Send Page" could both appear, the same access key will be a problem. ::: services/sync/modules/engines/clients.js:78 (Diff revision 1) > }, > set lastRecordUpload(value) { > Svc.Prefs.set(this.name + ".lastRecordUpload", Math.floor(value)); > }, > > + get clients() { I'd be inclined to use |remoteClients| - it's a little more verbose, but given things like .stats() includes the current device I think it makes it clearer to have the name clearly indicate the current device isn't included. Then your other checks would be able to check the number of remoteClient is > 0
Attachment #8763941 -
Flags: review?(markh)
Comment 29•8 years ago
|
||
Comment on attachment 8763941 [details] Bug 677372 - Send Tab/Page/Link to device. Stooopid mozreview...
Attachment #8763941 -
Flags: review?(markh)
Attachment #8763941 -
Flags: feedback?(markh)
Attachment #8763941 -
Flags: feedback+
Comment 30•8 years ago
|
||
I also meant to mention: "needs tests" :) We should also check with Ryan how much he cares about the device order - I think it will be arbitrary in this patch? (In reply to Mark Hammond [:markh] from comment #12) > Edwin, I guess we need a bug on file for the implementation of this and it > added to our current work, but we can call this bug done, thanks! No action from Edwin, so let's just re-open it and do it here.
Assignee: nobody → edouard.oger
Status: RESOLVED → REOPENED
Flags: needinfo?(edwong)
Resolution: FIXED → ---
Comment 31•8 years ago
|
||
*sigh* - sorry for the noise - another thing I forgot. I wonder if it might be better to add a new pref for this, with NIGHTLY_BUILD controlling the default value? Given all the elements are hidden by default, it might mean we could remove almost all NIGHTLY_BUILD blocks and it would still allow people to toggle it to enabled in non-nightly builds. Not too bothered by this though.
Updated•8 years ago
|
Whiteboard: [sync:tabs] [ux] → [sync:tabs] [ux][send-tab]
Assignee | ||
Comment 32•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61110/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61110/
Attachment #8766066 -
Flags: review?(markh)
Attachment #8763941 -
Flags: feedback+ → review?(markh)
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8763941 [details] Bug 677372 - Send Tab/Page/Link to device. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60044/diff/1-2/
Assignee | ||
Comment 34•8 years ago
|
||
Added tests and addressed comments.
Assignee | ||
Updated•8 years ago
|
Summary: Send Tab to Device - UX Design → Send Tab to Device
Whiteboard: [sync:tabs] [ux][send-tab] → [sync:tabs] [send-tab]
Assignee | ||
Updated•8 years ago
|
Priority: P4 → P1
Comment 35•8 years ago
|
||
https://reviewboard.mozilla.org/r/60044/#review58012 This is looking great, but the question of device ordering needs to be addressed (and I fear the patch will end up a little more complex WRT the new remoteClients getter if Ryan wants them sorted by "most recent" as we will also need to ensure that's exposed. ::: browser/base/content/browser-fxaccounts.js:83 (Diff revision 2) > return Weave.Status.login == Weave.LOGIN_FAILED_LOGIN_REJECTED; > }, > > + get sendTabToDeviceEnabled() { > + try { > + return Services.prefs.getBoolPref("services.sync.sendTabToDevice.enabled"); there shouldn't be a need to .catch here as we are ensuring the pref exists in all cases. ::: browser/base/content/browser-fxaccounts.js:395 (Diff revision 2) > + const clientId = event.target.getAttribute("clientId"); > + const clients = clientId > + ? [clientId] > + : this.remoteClients.map(client => client.id); > + > + clients.forEach(clientId => this.sendTabToDevice(url, clientId, title)); I'm not convinced and arbitrary order for devices is reasonable. ISTM it might change over a single run, which would be confusing. The 2 options I see are by device name, or by most recently synced. Please check with rfeeley - if he surprises me and says arbitrary is OK, then take this as r+ ::: browser/base/content/test/general/browser_contextmenu.js:858 (Diff revision 2) > }); > > +const remoteClientsFixture = [ { id: 1, name: "Foo"}, { id: 2, name: "Bar"} ]; > + > +add_task(function* test_plaintext_sendpagetodevice() { > + const oldGetter = setupRemoteClientsFixture(remoteClientsFixture); I think you need to skip these tests if the pref isn't set.
Assignee | ||
Comment 36•8 years ago
|
||
Comment on attachment 8766066 [details] Bug 677372 - Add onContextMenuShown option to test_contextmenu. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61110/diff/1-2/
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8763941 [details] Bug 677372 - Send Tab/Page/Link to device. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60044/diff/2-3/
Assignee | ||
Comment 38•8 years ago
|
||
Comments addressed, I added a lastModified field to the remote clients records in order to sort them by last activity.
Comment 39•8 years ago
|
||
https://reviewboard.mozilla.org/r/60044/#review58340 ::: services/sync/modules/engines/clients.js:557 (Diff revision 3) > this.engine.localCommands = incomingCommands; > }, > > _updateRemoteRecord(record) { > - let currentRecord = this._remoteClients[record.id]; > + const clearText = Object.assign({}, record.cleartext, { > + lastModified: record.modified Sadly I don't think this will work correctly - clients typically only update their own client record every 7 days, which looks like it will give quite misleading results (eg, a device that we know has been turned off for the last 3 days would well be considered more recent than devices we know synced recently). Thinking about it more, I don't think there is *any* way to get the last sync time for another client - sorry for implying there was. So sadly I think we should just sort by device name :( I think the sorting should probably be in browser-fxaccounts though rather than in the client engine.
Assignee | ||
Comment 40•8 years ago
|
||
Comment on attachment 8766066 [details] Bug 677372 - Add onContextMenuShown option to test_contextmenu. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61110/diff/2-3/
Assignee | ||
Comment 41•8 years ago
|
||
Comment on attachment 8763941 [details] Bug 677372 - Send Tab/Page/Link to device. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60044/diff/3-4/
Assignee | ||
Comment 42•8 years ago
|
||
Good point, I made the correction.
Comment 43•8 years ago
|
||
Comment on attachment 8763941 [details] Bug 677372 - Send Tab/Page/Link to device. https://reviewboard.mozilla.org/r/60044/#review59674 Looks great! Sorry for the delay - I thought I already r+'d this
Attachment #8763941 -
Flags: review?(markh) → review+
Updated•8 years ago
|
Attachment #8766066 -
Flags: review?(markh) → review+
Comment 44•8 years ago
|
||
Comment on attachment 8766066 [details] Bug 677372 - Add onContextMenuShown option to test_contextmenu. https://reviewboard.mozilla.org/r/61110/#review59676
Assignee | ||
Comment 45•8 years ago
|
||
Comment on attachment 8766066 [details] Bug 677372 - Add onContextMenuShown option to test_contextmenu. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61110/diff/3-4/
Assignee | ||
Comment 46•8 years ago
|
||
Comment on attachment 8763941 [details] Bug 677372 - Send Tab/Page/Link to device. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60044/diff/4-5/
Assignee | ||
Comment 47•8 years ago
|
||
Comment on attachment 8766066 [details] Bug 677372 - Add onContextMenuShown option to test_contextmenu. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61110/diff/4-5/
Assignee | ||
Comment 48•8 years ago
|
||
Comment on attachment 8763941 [details] Bug 677372 - Send Tab/Page/Link to device. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60044/diff/5-6/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 49•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/a6b4d5fa8ef6 Add onContextMenuShown option to test_contextmenu. r=markh https://hg.mozilla.org/integration/fx-team/rev/46231d51ba66 Send Tab/Page/Link to device. r=markh
Keywords: checkin-needed
Comment 50•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10537975&repo=fx-team, sorry Edouard
Flags: needinfo?(edouard.oger)
Comment 51•8 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/f7e309f64670 Backed out changeset 46231d51ba66 https://hg.mozilla.org/integration/fx-team/rev/22010b91698c Backed out changeset a6b4d5fa8ef6 for causing test failures like in browser_bug409481.js
Assignee | ||
Comment 52•8 years ago
|
||
Comment on attachment 8766066 [details] Bug 677372 - Add onContextMenuShown option to test_contextmenu. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61110/diff/5-6/
Assignee | ||
Comment 53•8 years ago
|
||
Comment on attachment 8763941 [details] Bug 677372 - Send Tab/Page/Link to device. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60044/diff/6-7/
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed
Comment 54•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/3db372da6fe1 Send Tab/Page/Link to device. r=markh, a=KWierso
Keywords: checkin-needed
Comment 55•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3db372da6fe1
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment 56•8 years ago
|
||
I have reproduced this bug with Nightly 46.0a1 (2016-01-22) on Windows 8.1 , 64 bit! The bug's fix is verified on Latest Nightly 50.0a1 . Build ID : 20160722030235 User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0 [bugday-20160720]
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•