Closed Bug 677372 Opened 8 years ago Closed 3 years ago

Send Tab to Device

Categories

(Firefox :: Sync, defect, P1)

defect

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
I have a proposal, see : http://www.hotimg.com/image/4fXEcbd
OS: Mac OS X → All
Hardware: x86 → All
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.
Thomas: yes, this is for the UI for sending on desktop.
Assignee: faaborg → nobody
Whiteboard: [sync:tabs]
Depends on: 769744
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
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).
Component: Firefox Sync: UI → Sync
Product: Mozilla Services → Firefox
Flags: firefox-backlog+
Flags: qe-verify-
Whiteboard: [sync:tabs] → [sync:tabs] [ux]
Priority: -- → P4
Duplicate of this bug: 1222601
Duplicate of this bug: 1224788
To me this bug is blocked by development on Maslaney's great Share plane work. I can't find the bug though!
Depends on: 1154302
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)
I really hope the shareplane work is ongoing, it would make the mobile and desktop experiences more homogeneous.
Attached image send-to-device.png (obsolete) —
Could it be as simple as this? I can't think of any edge cases that need UI.
Flags: needinfo?(rfeeley) → needinfo?(markh)
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: 4 years ago
Flags: needinfo?(markh) → needinfo?(edwong)
Resolution: --- → FIXED
Attached image page-contextual-menu.png (obsolete) —
The same contextual sub-menu could also be included at the page level like this. Thoughts Mark?
Flags: needinfo?(markh)
Attached image link-contextual-menu.png (obsolete) —
I assume we could also do this with a selected link?
Attached image link-contextual-menu.png (obsolete) —
Attachment #8748831 - Attachment is obsolete: true
Attached image page-contextual-menu.png (obsolete) —
Attachment #8748830 - Attachment is obsolete: true
Attached image send-to-device.png (obsolete) —
(added "Tab" to "Send Tab to Device")
Attachment #8748291 - Attachment is obsolete: true
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.
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.
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)
Attached image send-link-to-device.png
Attachment #8748848 - Attachment is obsolete: true
Attached image send-tab-to-device.png
Add "All devices" below with horizontal rule.
Attachment #8748854 - Attachment is obsolete: true
Attached image send-page-to-device.png
Add "All devices" at bottom with horizontal rule.
Attachment #8748849 - Attachment is obsolete: true
Attached image send-tab-to-device.png
Add "tab" word.
Attached image send-tab-to-device.png
Moved item higher up in the tab context menu
Attachment #8763941 - Flags: review?(markh) → feedback?(markh)
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 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+
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 → ---
*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.
Whiteboard: [sync:tabs] [ux] → [sync:tabs] [ux][send-tab]
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/
Added tests and addressed comments.
Summary: Send Tab to Device - UX Design → Send Tab to Device
Whiteboard: [sync:tabs] [ux][send-tab] → [sync:tabs] [send-tab]
Priority: P4 → P1
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.
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/
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/
Comments addressed, I added a lastModified field to the remote clients records in order to sort them by last activity.
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.
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/
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/
Good point, I made the correction.
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+
Attachment #8766066 - Flags: review?(markh) → review+
Comment on attachment 8766066 [details]
Bug 677372 - Add onContextMenuShown option to test_contextmenu.

https://reviewboard.mozilla.org/r/61110/#review59676
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/
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/
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/
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/
Keywords: checkin-needed
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
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10537975&repo=fx-team, sorry Edouard
Flags: needinfo?(edouard.oger)
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
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/
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/
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/3db372da6fe1
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.