1244597 - Show notification and decorate incoming tabs.

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P2)

Description

[Ryan Feeley [:rfeeley]]

Can we, and should we, notify users when their tabs sent from mobile arrive on desktop?

USER STORY
So that I can better realize when I've sent myself a tab from a mobile device, as a Sync user on desktop Firefox, I want to receive a desktop notification when a tab appears.

Draft copy suggestions:
Incoming tab {Page title} from {Device Name}

Will we know page title?
Will we know device name?
Can the push notification linked to the pushed tab?

Let's discuss.

Comment 1

[Ryan Kelly [:rfkelly]]

(Pretty much all the mechanics of sending tabs are implemented way down in sync, so I'm adding more sync-related folks to the bug.)

I think it'd be nice to do something like this.  The experience right now is not very obvious - you send the tab from your mobile device, and it kinda pops up as a new background tab sometime in the next five minutes, when the target device syncs it down.

> Will we know page title?
> Will we know device name?

I believe "yes" to both of these; the display-this-uri command appears to include oth the sender device id (from which we can look up its name in clients collection) and the title of the page:

  https://hg.mozilla.org/mozilla-central/file/tip/services/sync/modules/engines/clients.js#l377

> Can the push notification linked to the pushed tab?

I assume by "push notification" here you mean the UI that pops up to tell the user about the new tab, so I'll defer to folks with more platform experience on that one.  (Unfortunately "push notification" seems to be a pretty overloaded term around here at the moment)

Comment 2

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

(In reply to Ryan Kelly [:rfkelly] from comment #1)
> not very obvious - you send the tab from your mobile device, and it kinda
> pops up as a new background tab sometime in the next five minutes, when the
> target device syncs it down.

...

> > Can the push notification linked to the pushed tab?
> I assume by "push notification" here you mean the UI that pops up to tell
> the user about the new tab, so I'll defer to folks with more platform
> experience on that one.  (Unfortunately "push notification" seems to be a
> pretty overloaded term around here at the moment)

Yeah, there seem to be 2 things being (possibly) conflated here:

1) We should look into push notifications so that "sometime in the next five minutes" is actually closer to "immediately(ish)"

2) We should look into a better UI when a remote-tab is opened on desktop, possibly using a desktop notification.

IIUC, these are mutually exclusive - ie, we could do (1) without any additional UI, or we could do (2) without push and with the existing delay. While (1) + (2) seems ideal, is this bug really calling for both, or just (2)?

(And somewhat relatedly, it's worth considering what happens if, say, the desktop machine is running but I'm not sitting at it - the desktop notification is transient, so it's likely to be closed by the time I get back to the machine. I guess I'm wondering if there's something even better we could use in the main browser chrome UI?)

Comment 3

[Nick Alexander :nalexander [he/him]]

I agree with markh -- there's use Web Push to move tabs between devices faster, and there's use Web Notifications to surface sent tabs better; and these can proceed in parallel.

For the latter, Fennec does show a system notification as soon as a new tab arrives.  Such system notifications are persistent, and they're an integral part of the mobile paradigm.  (But we used them mostly for convenience: it saved us implementing our own UI to display tabs.)

Worth noting that I think Fennec wants to do better, and surface tabs sent to us in more places in our App, including in a persistent list of Sent Tabs or similar, although I haven't a ticket to hand.

Comment 4

[Ryan Kelly [:rfkelly]]

> While (1) + (2) seems ideal, is this bug really calling for both, or just (2)?

Let's make this bug specifically about (2), UX improvements for presenting sent tabs to the user.  Part of those improvements could be "make them appear faster" but we can spin off the implementation details of that into a separate bug.

Comment 5

[Ryan Feeley [:rfeeley]]

Agreed rfkelly!

Comment 6

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

Attachment: 0008-Bug-1244597-WIP-for-notification-of-a-remote-tab.patch

a WIP that works!

Comment 7

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

Attachment: How it looks

Comment 8

[Chris Karlof [:ckarlof]]

Ryan, given that Desktop Notifications only stay open for seconds and a user is unlikely away from her computer when she sends a tab back to her Desktop machine, I'm concerned she won't see the notification. Should we use something more persistent?

Comment 9

[Ryan Feeley [:rfeeley]]

No, I don't believe so. Likely most users will return to their desktops and will be active for the first while until Sync kicks in and the new tabs show up. Those with many tabs may not notice, this is just another way to make it noticeable without being too in-your-face.

Comment 10

[Ryan Feeley [:rfeeley]]

I'm going to look at the notification the Android receives. Perhaps we should be in parity there.

Comment 11

[Chris Karlof [:ckarlof]]

(In reply to Ryan Feeley [:rfeeley] from comment #9)
> No, I don't believe so. Likely most users will return to their desktops and
> will be active for the first while until Sync kicks in and the new tabs show
> up. Those with many tabs may not notice, this is just another way to make it
> noticeable without being too in-your-face.

Ok, unlike typical mobile notifications, Desktop Notifications show for about 5 seconds or so and then disappear. So if you're not staring at the screen, you're going to miss it. If you're away from your machine when it shows, you're definitely going to miss it. It doesn't sound like much value to me.

Comment 12

[Ryan Feeley [:rfeeley]]

Attachment: Screenshot 2016-02-04 14.05.51.png

Speaking with shorlander, we could also put the dot that we use for pinned tabs underneath. Indicates an unread state. I think this paired with however-ephemeral notifications would satisfy most users.

Comment 13

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

Attachment: Notification with the glowing indicator

(In reply to Ryan Feeley [:rfeeley] from comment #12)
> Speaking with shorlander, we could also put the dot that we use for pinned
> tabs underneath. Indicates an unread state. I think this paired with
> however-ephemeral notifications would satisfy most users.

Like this? :)

Comment 14

[Ryan Feeley [:rfeeley]]

Yeah! What do you think about the copy? I didn't think to use to site favicon. Very smart, but now the copy seems a little off to me. What do you think ahout:

FAVICON: Site favicon
TITLE: {Site/page title}
BODY: Tab from {device name}

Comment 15

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

Attachment: remotetabs-panel.png

(In reply to Ryan Feeley [:rfeeley] from comment #14)
> Yeah! What do you think about the copy? I didn't think to use to site
> favicon. Very smart, but now the copy seems a little off to me. What do you
> think ahout:
> 
> FAVICON: Site favicon
> TITLE: {Site/page title}
> BODY: Tab from {device name}

IMO that makes it look too much like the notification is from Twitter itself. Also, note that we may not have the favicon, in which case that default globe icon will appear (ie, maybe we should have a custom icon used just for this instead of the favicon)?

Comment 16

[Ryan Feeley [:rfeeley]]

I'd prefer no icon at all to the globe appearing most of the time.

Comment 17

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

There also seems to be a problem with multiple notifications at the same time not always working as expected. So the 2 options I see for that case are:

* wait for a notification to close, then show the next one. Given notifications stay around for ~5 seconds, if you send 4 tabs, notifications will be up for 20s.

* consolidate the messaging to say something like "4 tabs from {deviceName}" - but then it's not clear what clicking on the notification should do (probably opening the first would be OK - the other tabs will be directly to the right of that one)

Comment 18

[Ryan Feeley [:rfeeley]]

Attachment: incoming-tabs.png

What are your thoughts on this UX for incoming tab(s)?
1. Tabs open in the background.
2. Favicon has glow, but title is not bold.
3a. Single tab notification links to tab
3b. Multiple tab notification links to first (leftmost) tab, or simply window if not possible.

Comment 19

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

(In reply to Ryan Feeley [:rfeeley] from comment #18)
> Created attachment 8748339 [details]
> incoming-tabs.png

LGTM - I assume this is the same style of notification used in the "new device connected" notification?

> 1. Tabs open in the background.
> 2. Favicon has glow, but title is not bold.

I find the glow on tabs being very easy to miss - I wonder if we could show a badge, somewhat like the audio-mute indicator?

I don't quite understand the "bold" bit - I've never noticed the boldness of a tab title changing.

> 3a. Single tab notification links to tab
> 3b. Multiple tab notification links to first (leftmost) tab, or simply
> window if not possible.

SGTM - I guess this would remove the glow/badge or whatever we come up with - but what about the multiple-tabs case - would we remove the indicator from all tabs at this point, or leave them until the tab is actually selected?

Comment 20

[Ryan Feeley [:rfeeley]]

> LGTM - I assume this is the same style of notification used in the "new
> device connected" notification?

Yes! Same as what Hello uses too.

> > 1. Tabs open in the background.
> > 2. Favicon has glow, but title is not bold.
> 
> I find the glow on tabs being very easy to miss - I wonder if we could show
> a badge, somewhat like the audio-mute indicator?

It took some convincing to get Horlander on board with even the glow. Start there and see how it feels?

> I don't quite understand the "bold" bit - I've never noticed the boldness of
> a tab title changing.

Your screenshot had a bold title.
https://bug1244597.bmoattachments.org/attachment.cgi?id=8716023
Horlander noticed it, so I mentioned it just in case.

> SGTM - I guess this would remove the glow/badge or whatever we come up with
> - but what about the multiple-tabs case - would we remove the indicator from
> all tabs at this point, or leave them until the tab is actually selected?

Good one – I hadn't thought about that. I suppose any received tabs should stay glowed until clicked, pinned or moved to a new window?

Comment 21

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

Awesome - I think this is actionable. For whoever takes this on, I think we should consider moving the notification code into a place where all notifications we start showing for Sync (eg, when receiving a new tab, when sending a new tab, when a new device connects, etc) can reuse the same code to ensure have use consistent styling etc.

Comment 22

[Edouard Oger [:eoger]]

Attachment: Bug 1244597 - Show notification on incoming tab.

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

Comment 23

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

Comment on attachment 8768935 [details]
Bug 1244597 - Show notification on incoming tab.

https://reviewboard.mozilla.org/r/62930/#review59888

The bug mentions a number of decorations, such as a glow and bolding of the tab which don't seem to be implemented - did I just miss the magic?

::: browser/components/nsBrowserGlue.js:2472
(Diff revision 1)
> +
> +      const firstTab = openTab(URIs[0]);
> +      URIs.slice(1).forEach(URI => openTab(URI));
> +
> +      let title, body;
> +      const deviceName = Weave.Service.clientsEngine.getClientName(URIs[0].clientId);

While unlikely, it's possible there will be multiple senders here, right? We should check with Ryan what he wants to happen in that case (I *guess* we'd just make the language more neutral wrt the device name - something like "4 tabs received from your devices")

::: browser/locales/en-US/chrome/browser/accounts.properties:41
(Diff revision 1)
> +
> +# LOCALIZATION NOTE (tabArrivingNotification.title, tabArrivingNotification.body,
> +# tabsArrivingNotification.title, tabsArrivingNotification.body)
> +# These strings are used in a notification shown when we're opening tab(s) another device sent us to display.
> +tabArrivingNotification.title = Tab received
> +tabArrivingNotification.body = "%1$S" has arrived from %2$S.

We should have comments here indicating what %1 and %2 are for localizers.

::: services/sync/modules/engines/clients.js:406
(Diff revision 1)
>            case "logout":
>              this.service.logout();
>              return false;
>            case "displayURI":
> -            this._handleDisplayURI.apply(this, args);
> +            let [uri, clientId, title] = args;
> +            URIsToDisplay.push({uri, clientId, title});

spaces after "{" and before "}"

Comment 24

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

https://reviewboard.mozilla.org/r/62930/#review59888

> While unlikely, it's possible there will be multiple senders here, right? We should check with Ryan what he wants to happen in that case (I *guess* we'd just make the language more neutral wrt the device name - something like "4 tabs received from your devices")

hrm - I guess that's not possible *here* - if not, maybe the data format should be, say, { clientId, tabs: [{ url, title }] } or similar?

Comment 25

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

https://reviewboard.mozilla.org/r/62930/#review59888

> hrm - I guess that's not possible *here* - if not, maybe the data format should be, say, { clientId, tabs: [{ url, title }] } or similar?

Actually I think it is possible here - that's what happens when I look at code before I've finished my first coffee :)

Comment 26

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

Comment on attachment 8748339 [details]
incoming-tabs.png

Ryan, re this attachment. We have a 3rd case - "2 tabs have arrived from 2 different devices". The usual case will be the first - so instead of a 3rd state, we could consider rewording the second so it doesn't include the device name. Or we need copy for a 3rd. WDYT?

Comment 27

[Ryan Feeley [:rfeeley]]

For the 3rd case, just drop the device name. I'll update the mockup.

Comment 28

[Ryan Feeley [:rfeeley]]

Attachment: incoming-tabs.png

How's this?

Comment 29

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

Comment on attachment 8769340 [details]
incoming-tabs.png

LGTM, thanks!

Comment 30

[Ryan Feeley [:rfeeley]]

Is there an easter egg that we can unlock when this happens? It really is quite a feat for this to happen.

Comment 31

[Edouard Oger [:eoger]]

Comment on attachment 8768935 [details]
Bug 1244597 - Show notification on incoming tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62930/diff/1-2/

Comment 32

[Edouard Oger [:eoger]]

Comments addressed, and we show the glowing tab decoration too :)

Comment 33

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

(In reply to Ryan Feeley [:rfeeley] from comment #30)
> Is there an easter egg that we can unlock when this happens? It really is
> quite a feat for this to happen.

Sure :) Give us an asset and see what magic Edouard can come up with ;)

Comment 34

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

Comment on attachment 8768935 [details]
Bug 1244597 - Show notification on incoming tab.

https://reviewboard.mozilla.org/r/62930/#review60556

LGTM apart from the "no browser window" state on OSX - can you please confirm I'm correct and if so, open a new window, but if not, take this as r+ ;)

::: browser/components/nsBrowserGlue.js:2464
(Diff revision 2)
>      try {
> -      let tabbrowser = RecentWindow.getMostRecentBrowserWindow({private: false}).gBrowser;
> -
>        // The payload is wrapped weirdly because of how Sync does notifications.
> -      tabbrowser.addTab(data.wrappedJSObject.object.uri);
> +      const URIs = data.wrappedJSObject.object;
> +      const tabbrowser = RecentWindow.getMostRecentBrowserWindow({private: false}).gBrowser;

I should have mentioned this before, but I suspect that if you close the last window on OSX this code will fail when it should open a new one (the old code doesn't handle this either, but I think we should)

Comment 35

[Edouard Oger [:eoger]]

Comment on attachment 8768935 [details]
Bug 1244597 - Show notification on incoming tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62930/diff/2-3/

Comment 36

[Edouard Oger [:eoger]]

Thanks Mark, this one opens a new window if not present.

Comment 37

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

Comment on attachment 8768935 [details]
Bug 1244597 - Show notification on incoming tab.

https://reviewboard.mozilla.org/r/62930/#review60876

awesome, thanks!

::: browser/locales/en-US/chrome/browser/accounts.properties:41
(Diff revision 3)
> +
> +# LOCALIZATION NOTE (tabArrivingNotification.title, tabArrivingNotification.body,
> +# tabsArrivingNotification.title, tabsArrivingNotification.body)
> +# These strings are used in a notification shown when we're opening tab(s) another device sent us to display.
> +tabArrivingNotification.title = Tab received
> +# LOCALIZATION NOTE (tabArrivingNotification.body) %1 is the name of the tab and %2 is the device name.

s/name/title/ here

Comment 38

[Edouard Oger [:eoger]]

Comment on attachment 8768935 [details]
Bug 1244597 - Show notification on incoming tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62930/diff/3-4/

Comment 39

[Edouard Oger [:eoger]]

Ryan feel free to open a follow-up bug for the easter egg :)

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/49ef0b8c3cfa
Show notification on incoming tab. r=markh

Comment 41

[Carsten Book [:Tomcat]]

had to back this out to apply the backout for bug 677372

Comment 42

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a92a8ef106a5
Backed out changeset 49ef0b8c3cfa

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/53236b4393f2
Show notification on incoming tab. r=markh

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/48d6706662c0
Fix quotes to make browser_misused_characters_in_strings.js happy. a=orange

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Backed out for perma-timeouts in test_clients_engine.js. Not sure which Sync patch was actually responsible for it, but this didn't have a green Try link, so you'll need to sort it out. Be sure to fold the follow-up patch into your next patch as well.

https://treeherder.mozilla.org/logviewer.html#?job_id=10588073&repo=fx-team#L7634

Comment 46

[Edouard Oger [:eoger]]

Comment on attachment 8768935 [details]
Bug 1244597 - Show notification on incoming tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62930/diff/4-5/

Comment 47

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ade1e83511c8488b15f387ee0dfea8a2350b2019

Try looks green, sorry about that Ryan!

Comment 48

[Carsten Book [:Tomcat]]

seems this needs rebasing or so 

patching file browser/locales/en-US/chrome/browser/accounts.properties
Hunk #1 FAILED at 27
1 out of 1 hunks FAILED -- saving rejects to file browser/locales/en-US/chrome/browser/accounts.properties.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

can you take a look, thanks!

Comment 49

[Edouard Oger [:eoger]]

Comment on attachment 8768935 [details]
Bug 1244597 - Show notification on incoming tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62930/diff/5-6/

Comment 50

[Edouard Oger [:eoger]]

Rebased, thanks!

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e7a27a7538b2
Show notification on incoming tab. r=markh

Comment 52

[Francesco Lodolo [:flod]]

This looks backed out in central, not sure if you can fix it again, or it needs a follow-up at this point
https://hg.mozilla.org/mozilla-central/diff/53236b4393f2/browser/locales/en-US/chrome/browser/accounts.properties

tabsArrivingNotification.body = %1$S tabs have arrived from %2$S.
tabsArrivingNotificationMultiple.body = %S tabs have arrived from your connected devices.

Please use proper plural forms
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms

Comment 53

[Edouard Oger [:eoger]]

Let's open a follow-up bug once it lands in central.

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7a27a7538b2

Comment 55

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 x64, Ubuntu 13.10 x86 and Mac OSX 10.11 using Firefox 50 Beta 10 (buildID:  	20161024172922): a desktop notification is correctly received when a tab or multiple tabs from a mobile device/s arrives on desktop. 

Three scenarios were tested: one tab received, multiple tabs received and multiple tabs received from multiple devices - all the notifications look good.