Use system notification for website notifications in Android

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: antlam, Assigned: liuche)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 46
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed, fennec45+)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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?
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.

Updated

3 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → 45+

Comment 2

3 years ago
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.
Flags: needinfo?(liuche)
(Assignee)

Comment 3

3 years ago
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.
Assignee: nobody → liuche
Flags: needinfo?(liuche)
(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

3 years ago
(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)
(Reporter)

Comment 6

3 years ago
Created attachment 8677064 [details]
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
Flags: needinfo?(liuche)
(Assignee)

Comment 7

3 years ago
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
Flags: needinfo?(mark.finkle)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(liuche)
(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
Flags: needinfo?(mark.finkle)

Comment 9

3 years ago
Sounds like mfinkle helped answer your questions here.
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 10

3 years ago
Created attachment 8688203 [details]
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
(Assignee)

Comment 11

3 years ago
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'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).
Attachment #8688204 - Flags: feedback?(alam)
Do we want to support the same look and features as Chrome? Seems reasonable to me.
(Reporter)

Comment 13

3 years ago
(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.
(Reporter)

Updated

3 years ago
Attachment #8688204 - Flags: feedback?(alam) → feedback-
> 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?
Flags: needinfo?(liuche)
(Reporter)

Comment 15

3 years ago
Could we prioritize for iOS touch icons if available?
(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

3 years ago
(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.
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.
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.
(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.
(Assignee)

Comment 21

3 years ago
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/
Attachment #8688203 - Flags: review?(mark.finkle)
(Assignee)

Updated

3 years ago
Flags: needinfo?(liuche)
Attachment #8688203 - Flags: review?(mark.finkle)
(Assignee)

Comment 22

3 years ago
Created attachment 8694010 [details]
Screenshot: unexpanded notification
(Assignee)

Comment 23

3 years ago
Created attachment 8694013 [details]
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.
Attachment #8694013 - Flags: feedback?(alam)
(Reporter)

Comment 24

3 years ago
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
Flags: needinfo?(liuche)
Attachment #8694013 - Flags: feedback?(alam) → feedback+
(Assignee)

Comment 25

3 years ago
Created attachment 8694854 [details]
Screenshot: "X" and bolded url
Attachment #8688204 - Attachment is obsolete: true
Attachment #8694010 - Attachment is obsolete: true
Attachment #8694013 - Attachment is obsolete: true
Flags: needinfo?(liuche)
Attachment #8694854 - Flags: feedback?(alam)
(Assignee)

Comment 26

3 years ago
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/
Attachment #8688203 - Flags: review?(mark.finkle)
(Assignee)

Comment 27

3 years ago
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.
FWIW, on desktop we don't offer a Mute/DND option when using a system backend since the OS already provides that mechanism globally.
(Reporter)

Comment 29

3 years ago
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.
Attachment #8694854 - Flags: feedback?(alam) → feedback-
(Assignee)

Comment 30

3 years ago
Created attachment 8695932 [details]
Screenshot: Part 1 with url on bottom

Part 2 includes a working Mute button.
Attachment #8694854 - Attachment is obsolete: true
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.
Attachment #8688203 - Flags: review?(mark.finkle) → review+

Comment 32

3 years ago
What's left to do here?
Flags: needinfo?(liuche)
(Assignee)

Updated

3 years ago
Blocks: 1233150

Comment 34

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e564d6f8056d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(Assignee)

Updated

3 years ago
Flags: needinfo?(liuche)
You need to log in before you can comment on or make changes to this bug.