Show notification and decorate incoming tabs.

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Sync
P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: rfeeley, Assigned: eoger)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 50
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [send-tab])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(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)
(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?)
Flags: firefox-backlog+
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.
> 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.
(Reporter)

Comment 5

2 years ago
Agreed rfkelly!
Created attachment 8715536 [details] [diff] [review]
0008-Bug-1244597-WIP-for-notification-of-a-remote-tab.patch

a WIP that works!
Created attachment 8715537 [details]
How it looks
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?
Flags: needinfo?(rfeeley)
(Reporter)

Comment 9

2 years ago
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.
Flags: needinfo?(rfeeley)
(Reporter)

Comment 10

2 years ago
I'm going to look at the notification the Android receives. Perhaps we should be in parity there.
(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.
(Reporter)

Comment 12

2 years ago
Created attachment 8715942 [details]
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.
Created attachment 8716023 [details]
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? :)
Attachment #8715537 - Attachment is obsolete: true
(Reporter)

Comment 14

2 years ago
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}
Flags: needinfo?(markh)
Created attachment 8716028 [details]
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)?
Flags: needinfo?(markh)
(Reporter)

Comment 16

2 years ago
I'd prefer no icon at all to the globe appearing most of the time.
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)
Assignee: nobody → jmoradi
Flags: firefox-backlog+
(Reporter)

Comment 18

2 years ago
Created attachment 8748339 [details]
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.
Flags: needinfo?(markh)
(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?
Flags: needinfo?(markh)
(Reporter)

Comment 20

2 years ago
> 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?
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.
Assignee: jmoradi → nobody
Flags: firefox-backlog+
Summary: Discuss pairing tabs sent from mobile devices with push notifications → Show notification and decorate incoming tabs.
Assignee: nobody → edouard.oger
Blocks: 1269900
Flags: firefox-backlog+
Priority: -- → P2
(Assignee)

Updated

2 years ago
Whiteboard: [send-tab]
(Assignee)

Comment 22

2 years ago
Created attachment 8768935 [details]
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/
Attachment #8768935 - Flags: review?(markh)
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 "}"
Attachment #8768935 - Flags: review?(markh)
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?
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 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?
Flags: needinfo?(rfeeley)
(Reporter)

Comment 27

2 years ago
For the 3rd case, just drop the device name. I'll update the mockup.
Flags: needinfo?(rfeeley)
(Reporter)

Comment 28

2 years ago
Created attachment 8769340 [details]
incoming-tabs.png

How's this?
Attachment #8748339 - Attachment is obsolete: true
Flags: needinfo?(markh)
Flags: needinfo?(edouard.oger)
Comment on attachment 8769340 [details]
incoming-tabs.png

LGTM, thanks!
Flags: needinfo?(markh)
(Reporter)

Comment 30

2 years ago
Is there an easter egg that we can unlock when this happens? It really is quite a feat for this to happen.
Flags: needinfo?(markh)
(Assignee)

Comment 31

2 years ago
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/
Attachment #8768935 - Flags: review?(markh)
(Assignee)

Comment 32

2 years ago
Comments addressed, and we show the glowing tab decoration too :)
Flags: needinfo?(edouard.oger)

Updated

2 years ago
Attachment #8715536 - Attachment is obsolete: true
(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 ;)
Flags: needinfo?(markh)
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)
Attachment #8768935 - Flags: review?(markh)
(Assignee)

Comment 35

2 years ago
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/
Attachment #8768935 - Flags: review?(markh)
(Assignee)

Comment 36

2 years ago
Thanks Mark, this one opens a new window if not present.
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
Attachment #8768935 - Flags: review?(markh) → review+
(Assignee)

Comment 38

2 years ago
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/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 39

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

Comment 40

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/49ef0b8c3cfa
Show notification on incoming tab. r=markh
Keywords: checkin-needed
had to back this out to apply the backout for bug 677372
Flags: needinfo?(edouard.oger)

Comment 42

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/a92a8ef106a5
Backed out changeset 49ef0b8c3cfa
(Assignee)

Updated

2 years ago
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed

Comment 43

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/53236b4393f2
Show notification on incoming tab. r=markh
Keywords: checkin-needed

Comment 44

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/48d6706662c0
Fix quotes to make browser_misused_characters_in_strings.js happy. a=orange
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
(Assignee)

Comment 46

2 years ago
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/
(Assignee)

Comment 47

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

Try looks green, sorry about that Ryan!
Keywords: checkin-needed
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!
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed
(Assignee)

Comment 49

2 years ago
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/
(Assignee)

Comment 50

2 years ago
Rebased, thanks!
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed

Comment 51

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/e7a27a7538b2
Show notification on incoming tab. r=markh
Keywords: checkin-needed
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
Flags: needinfo?(edouard.oger)
(Assignee)

Comment 53

2 years ago
Let's open a follow-up bug once it lands in central.
Flags: needinfo?(edouard.oger)

Comment 54

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7a27a7538b2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1287478
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.