1212611 - Use system notification for website notifications in Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

We should leverage system UI here as much as we can. 

The content of the notification currently breaks down to:

 - Notification message title
 - Notification message body
 - Source (URL)
 - Time/date
 - Icon 
 - - currently unsure if this will be the Firefox icon or the Source's icon right now (favicon?)
 - secondary icon 
 - - in Chrome for instance, it's a "bell" but this is only compatible for so many versions of Android, older versions won't get 2 icons.
 - Contextual action
 - - Currently, this can be a quick way to access Site settings
 - - Could we possibly put a "Mute notifications from this site" action here instead?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

We already support creating notifications in two places:
1. Notifications.jsm which is mainly for JavaScript code and uses NotificationHelper.java
2. nsIAlertsService, which is used throughout Gecko to show notifications and is handled on Android in GeckoAppShell::showAlertNotification

For this feature, we want to adapt #2, which is the codepath that handles Web Notifications. One thing I notice is that the Android codepath diverged from the Gecko codepath here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/nsAlertsService.cpp#96

Gecko has more parameters.

Comment 2

[:Margaret Leibovic]

Chenxia, can you own the UI changes here? I imagine we'll want to have this notification link back to site settings at some point, to let users revoke push notifications permissions.

Comment 3

[Chenxia Liu [:liuche]]

Sure - I took a look at Anthony's mocks (in person) and they seem fine. We just need to add another doorhanger type, but this one is simple.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chenxia Liu [:liuche] from comment #3)
> Sure - I took a look at Anthony's mocks (in person) and they seem fine. We
> just need to add another doorhanger type, but this one is simple.

No doorhangers. This bug is about the System Notifications. We need to tweak the current notification used for Web Notifications. A different bug is about the permission prompt.

Comment 5

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to Chenxia Liu [:liuche] from comment #3)
> > Sure - I took a look at Anthony's mocks (in person) and they seem fine. We
> > just need to add another doorhanger type, but this one is simple.
> 
> No doorhangers. This bug is about the System Notifications. We need to tweak
> the current notification used for Web Notifications. A different bug is
> about the permission prompt.

(bug 1212606)

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_push_notification1.png

Met with Chenxia and chatted about what to display here. We're going to try two things first. But I think either will satisfy this bug and our first version for this feature.

tl;dr

Firefox icon
# web notifications
Firefox 

or

Firefox icon
# web notifications
domain.com, domain2.com, domai... 

When expanded:
# web notifications expands to show actual notifications..

For reference: 
http://developer.android.com/design/patterns/notifications_k.html
http://developer.android.com/design/patterns/notifications.html

Comment 7

[Chenxia Liu [:liuche]]

Okay, so I'm actually a little confused on this bug now - at first I thought it was for updating the copy of system notifications to handle notifications sent specifically from a webpage, as well as stacking them correctly so that we can group all web notifications (like [1]). But it seems like there is no differentiation between "types" of notifications so that we can handle them differently on the Java-side - or am I missing something?

It looks like all our system notifications from Gecko/webcontent are made by instantiating Notification objects (from mobile/android/modules/Notifications.jsm) and then sending a Notification:Show event and getting created through JNI, but I'm not seeing anywhere where we differentiate between types of notification (web/push [is there a difference or are they the same?], Downloads notification, etc).

This could be that Desktop is able to treat all notifications in the same way because they don't need to stack them, so we need to provide some way to differentiate between Web/PushNotifications vs notifications-sent-from-Gecko (such as Downloads, or to install a new Nightly update). Is that being done in a different bug, or is that part of the scope of this bug?

Or is this something completely different, for some notifications API that hasn't been implemented yet? (GCM?)

[1] https://people.mozilla.org/~mnoorenberghe/w3c_notifications.htm

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chenxia Liu [:liuche] from comment #7)
> Okay, so I'm actually a little confused on this bug now - at first I thought
> it was for updating the copy of system notifications to handle notifications
> sent specifically from a webpage, as well as stacking them correctly so that
> we can group all web notifications (like [1]). But it seems like there is no
> differentiation between "types" of notifications so that we can handle them
> differently on the Java-side - or am I missing something?
> 
> It looks like all our system notifications from Gecko/webcontent are made by
> instantiating Notification objects (from
> mobile/android/modules/Notifications.jsm) and then sending a
> Notification:Show event and getting created through JNI, but I'm not seeing
> anywhere where we differentiate between types of notification (web/push [is
> there a difference or are they the same?], Downloads notification, etc).

See comment 1. We do not use Notifications.jsm for showing System Notifications from Web Notifications (desktop-notifications) API. We use the nsIAlertsService which has a JNI binding/bridge in Java.

> This could be that Desktop is able to treat all notifications in the same
> way because they don't need to stack them, so we need to provide some way to
> differentiate between Web/PushNotifications vs notifications-sent-from-Gecko
> (such as Downloads, or to install a new Nightly update). Is that being done
> in a different bug, or is that part of the scope of this bug?

Do not worry about stacking System Notifications at this time.
 
> Or is this something completely different, for some notifications API that
> hasn't been implemented yet? (GCM?)

Not something different. nsIAlertsService.

> [1] https://people.mozilla.org/~mnoorenberghe/w3c_notifications.htm

Comment 9

[:Margaret Leibovic]

Sounds like mfinkle helped answer your questions here.

Comment 10

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1212611 - Use system notification for website notifications in Android. r=mfinkle

Bug 1212611 - Use system notification for website notifications in Android. r=mfinkle

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Push notification

This is a screenshot of the push notification in the system notifications, where currently we display the url and the message. I'd like some feedback about how we want to display title, message, and url - would we want to combine the title and message into a single line, and have the url be the title?

I'll also add a button - text being "Disable notifications".

(I was also playing around with push notifications on Chrome, though for some reason our test push notification pages don't seem to do anything on Chrome Android, although they do work on Chrome Desktop).

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Do we want to support the same look and features as Chrome? Seems reasonable to me.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Chenxia Liu [:liuche] from comment #11)
> Created attachment 8688204 [details]
> Screenshot: Push notification
> 
> This is a screenshot of the push notification in the system notifications,
> where currently we display the url and the message. I'd like some feedback
> about how we want to display title, message, and url - would we want to
> combine the title and message into a single line, and have the url be the
> title?

I think we can pull out the URL to another line like Chrome does. As we talked about over IRC, if there's a title (like "Roost") then we should place it in the Title spot, if not, we can fall back to the domain (like "people.mozilla.org").

> I'll also add a button - text being "Disable notifications".

I think "Mute notifications" is better here. It seems less difficult to read.

But I am concerned about how many notifications will stack up since each will be taller with the addition of this action button. We'll see.

> (I was also playing around with push notifications on Chrome, though for
> some reason our test push notification pages don't seem to do anything on
> Chrome Android, although they do work on Chrome Desktop).

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Do we want to support the same look and features as Chrome? Seems reasonable
> to me.

I was thinking the same thing. If we could grab the same asset (is it the favicon?) as chrome, it'd be a nice touch compared to just our Fox logo.

Comment 14

[Nick Alexander :nalexander [he/him]]

> I was thinking the same thing. If we could grab the same asset (is it the
> favicon?) as chrome, it'd be a nice touch compared to just our Fox logo.

liuche: getting a favicon is not so hard in this context, right?

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Could we prioritize for iOS touch icons if available?

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #15)
> Could we prioritize for iOS touch icons if available?

Not in the scope of this bug.

Comment 17

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #16)
> (In reply to Anthony Lam (:antlam) from comment #15)
> > Could we prioritize for iOS touch icons if available?
> 
> Not in the scope of this bug.

I agree, let's just use whatever favicon we have stored for the site.

Comment 18

[Matthew N. [:MattN]]

Is there a bug yet for adding our own actions (disable and Notification settings)? Please CC me on it and use alertdisablecallback and alertsettingscallback so it gets recorded for free in telemetry from bug 1225336.

Comment 19

[Nick Alexander :nalexander [he/him]]

Flipkart provided two valuable pieces of feedback:

1) they want to minimize (really, remove) the Site Settings icon.  They've witnessed lots of "false taps" in Chrome due to large Site Settings and small "primary action" tap targets.

2) showing real icons (not Browser icon, not generic bell icon) is very important for recognition.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #19)
> Flipkart provided two valuable pieces of feedback:
> 
> 1) they want to minimize (really, remove) the Site Settings icon.  They've
> witnessed lots of "false taps" in Chrome due to large Site Settings and
> small "primary action" tap targets.

Let's temper this with putting the users in control. We'd rather have the button.

Comment 21

[Chenxia Liu [:liuche]]

Comment on attachment 8688203 [details]
MozReview Request: Bug 1212611 - Use system notification for website notifications in Android. r=mfinkle

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

Comment 22

[Chenxia Liu [:liuche]]

Attachment: Screenshot: unexpanded notification

Comment 23

[Chenxia Liu [:liuche]]

Attachment: Screenshot: expanded notification

Antlam, what do you think? This matches the paper mocks better, although we can move the url to the bottom of that stack too.

Also, I need the "Mute notifications" icon from you - it can match the size of drawable/ic_action_settings.

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8694013 [details]
Screenshot: expanded notification

+, looking good! 

I can get you the icon. Though, I wonder if Desktop is using a custom one...

I don't have a specific one for turning off notifications for now so maybe we can stick with a generic 'x' in the mean time. 

As for the URL, I think it kind of gets lost a bit next to the message but it's also weird to have it _after_ the action so I'd say keep it here for now. Do we have the ability to alter the style here? color or size, that might help

Comment 25

[Chenxia Liu [:liuche]]

Attachment: Screenshot: "X" and bolded url

Comment 26

[Chenxia Liu [:liuche]]

Comment on attachment 8688203 [details]
MozReview Request: Bug 1212611 - Use system notification for website notifications in Android. r=mfinkle

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

Comment 27

[Chenxia Liu [:liuche]]

The "Mute notifications" part is getting really hairy, so I'm going to r? this first part.

Mute notifications is complicated because we want to allow disabling when Firefox is not running, so we need a service (like TabQueues). Just doing this action while FF is running isn't enough, because then the button does nothing and is misleading. So the second part of this will be a Service that calls into PermissionsHelper when Firefox starts up again.

Comment 28

[Matthew N. [:MattN]]

FWIW, on desktop we don't offer a Mute/DND option when using a system backend since the OS already provides that mechanism globally.

Comment 29

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8694854 [details]
Screenshot: "X" and bolded url

Talked on IRC, I believe we've gone with the source URL under the button now.

Comment 30

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Part 1 with url on bottom

Part 2 includes a working Mute button.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8688203 [details]
MozReview Request: Bug 1212611 - Use system notification for website notifications in Android. r=mfinkle

https://reviewboard.mozilla.org/r/25307/#review24703

This all looks good to me. I assume part 2 (yet to come) will have the final changes for the notification layout.

Comment 32

[:Margaret Leibovic]

What's left to do here?

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e564d6f8056d

Comment 34

[Carsten Book [:Tomcat]]

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