677372 - Send Tab to Device

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Gregory Szorc [:gps]]

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

[guillaume.goessel]

I have a proposal, see : http://www.hotimg.com/image/4fXEcbd

Comment 2

[Thomas Stache]

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

[Richard Newman [:rnewman]]

Thomas: yes, this is for the UI for sending on desktop.

Comment 4

[Paul [pwd]]

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

[Florian Bender]

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).

Comment 8

[Ryan Feeley [:rfeeley]]

To me this bug is blocked by development on Maslaney's great Share plane work. I can't find the bug though!

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Ryan, is the shareplane work still ongoing? If not, what do you think about adding this capability to the "synced tabs" panel?

Comment 10

[Paul [pwd]]

I really hope the shareplane work is ongoing, it would make the mobile and desktop experiences more homogeneous.

Comment 11

[Ryan Feeley [:rfeeley]]

Attachment: send-to-device.png

Could it be as simple as this? I can't think of any edge cases that need UI.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

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)

Comment 13

[Ryan Feeley [:rfeeley]]

Attachment: page-contextual-menu.png

The same contextual sub-menu could also be included at the page level like this. Thoughts Mark?

Comment 14

[Ryan Feeley [:rfeeley]]

Attachment: link-contextual-menu.png

I assume we could also do this with a selected link?

Comment 15

[Ryan Feeley [:rfeeley]]

Attachment: link-contextual-menu.png

Comment 16

[Ryan Feeley [:rfeeley]]

Attachment: page-contextual-menu.png

Comment 17

[Ryan Feeley [:rfeeley]]

Attachment: send-to-device.png

(added "Tab" to "Send Tab to Device")

Comment 18

[Lina Butler [:lina]]

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

[Ryan Feeley [:rfeeley]]

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

[Mark Hammond [:markh] [:mhammond]]

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...

Comment 21

[Ryan Feeley [:rfeeley]]

Attachment: send-link-to-device.png

Comment 22

[Ryan Feeley [:rfeeley]]

Attachment: send-tab-to-device.png

Add "All devices" below with horizontal rule.

Comment 23

[Ryan Feeley [:rfeeley]]

Attachment: send-page-to-device.png

Add "All devices" at bottom with horizontal rule.

Comment 24

[Ryan Feeley [:rfeeley]]

Attachment: send-tab-to-device.png

Add "tab" word.

Comment 25

[Ryan Feeley [:rfeeley]]

Attachment: send-tab-to-device.png

Moved item higher up in the tab context menu

Comment 26

[Edouard Oger [:eoger]]

Attachment: Bug 677372 - Send Tab/Page/Link to device.

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

Comment 27

[Edouard Oger [:eoger]]

rfeeley: here's a build with this patch http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-569a6c1c2c02e50e22a57195fb53f34cda9ef50f/try-macosx64/firefox-50.0a1.en-US.mac.dmg

Comment 28

[Mark Hammond [:markh] [:mhammond]]

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

Comment 29

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8763941 [details]
Bug 677372 - Send Tab/Page/Link to device.

Stooopid mozreview...

Comment 30

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 31

[Mark Hammond [:markh] [:mhammond]]

*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.

Comment 32

[Edouard Oger [:eoger]]

Attachment: Bug 677372 - Add onContextMenuShown option to test_contextmenu.

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

Comment 33

[Edouard Oger [:eoger]]

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/

Comment 34

[Edouard Oger [:eoger]]

Added tests and addressed comments.

Comment 35

[Mark Hammond [:markh] [:mhammond]]

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 36

[Edouard Oger [:eoger]]

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 37

[Edouard Oger [:eoger]]

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/

Comment 38

[Edouard Oger [:eoger]]

Comments addressed, I added a lastModified field to the remote clients records in order to sort them by last activity.

Comment 39

[Mark Hammond [:markh] [:mhammond]]

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 40

[Edouard Oger [:eoger]]

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 41

[Edouard Oger [:eoger]]

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/

Comment 42

[Edouard Oger [:eoger]]

Good point, I made the correction.

Comment 43

[Mark Hammond [:markh] [:mhammond]]

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

Comment 44

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8766066 [details]
Bug 677372 - Add onContextMenuShown option to test_contextmenu.

https://reviewboard.mozilla.org/r/61110/#review59676

Comment 45

[Edouard Oger [:eoger]]

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 46

[Edouard Oger [:eoger]]

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 47

[Edouard Oger [:eoger]]

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 48

[Edouard Oger [:eoger]]

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/

Comment 49

[Pulsebot]

Pushed by [e-mail]:
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

Comment 50

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10537975&repo=fx-team, sorry Edouard

Comment 51

[Pulsebot]

Backout by [e-mail]:
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 52

[Edouard Oger [:eoger]]

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 53

[Edouard Oger [:eoger]]

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/

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/3db372da6fe1
Send Tab/Page/Link to device. r=markh, a=KWierso

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3db372da6fe1

Comment 56

[Azmina]

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]