Prompt user (with a doorhanger) to allow to notifications from a website

RESOLVED FIXED in Firefox 45

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: antlam, Assigned: liuche)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 45
All
Android
Points:
---

Firefox Tracking Flags

(firefox45 fixed, fennec45+)

Details

MozReview Requests

()

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

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
According to Bug 1209999, Desktop will be prompting for permissions from the user to allow notifications. I've not seen the mocks yet at this point but I assume it'll be a doorhanger experience. 

We should likely do the same through a doorhanger.

The final copy was:

Would you like to receive notifications from this site?
Keep in mind that we already have a doorhanger for allowing Web Notifications (the notifications in the system tray). It sounds like we'd be using the same copy for both Push and Web Notifications.

So we might need to:
1. Change the current copy to the new copy
2. Use the same doorhanger as a permission prompt for Push

Here is where we show the doorhanger for Web Notifications (all content permissions really):
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentPermissionPrompt.js#131

Here is the copy for Web Notifications:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties#125

(yes, the prefix is 'desktopNotification')

Comment 2

2 years ago
:antlam here's the doorhanger:
https://people.mozilla.org/~ewong2/push/perm0.png
https://people.mozilla.org/~ewong2/push/perm1.png

Comment 3

2 years ago
the screen shots have outdated copy but the icon is the final version.  You may want to use that icon and address bar icon for consistency.

Updated

2 years ago
tracking-fennec: --- → ?
(In reply to Edwin Wong [:edwong] from comment #3)
> the screen shots have outdated copy but the icon is the final version.  You
> may want to use that icon and address bar icon for consistency.

We don't use icons in the doorhangers or address bar the same way as desktop. Here is an example of the current doorhanger in Fennec:
http://people.mozilla.org/~mfinkle/fennec/screenshots/fennec-swpush-prompt.png
(Reporter)

Comment 5

2 years ago
(In reply to Edwin Wong [:edwong] from comment #3)
> the screen shots have outdated copy but the icon is the final version.  You
> may want to use that icon and address bar icon for consistency.

OK, I will ignore the copy and stick with what I saw and wrote down in comment 0.

Phlsa! Can you point me in the direction of where I might find this icon? See comment 2.
Flags: needinfo?(philipp)
tracking-fennec: ? → 45+

Comment 6

2 years ago
I'm not sure exactly what work will be required here, but I think it will be small. Chenxia, can you own whatever doorhanger UI changes are needed here?
Flags: needinfo?(liuche)
(Reporter)

Comment 7

2 years ago
Created attachment 8677054 [details]
prev_push_prompt1.png

Here's the current state of it in a static mock. I've just synced up with Chenxia to get this in code.

Note: still waiting on Phlsa to update with the push icon so I can replace the placeholder globe here :)
(Reporter)

Comment 8

2 years ago
Created attachment 8680067 [details]
prev_push_prompt2.png

I found the icon!
Attachment #8677054 - Attachment is obsolete: true
Flags: needinfo?(philipp)
(Reporter)

Comment 9

2 years ago
Created attachment 8680077 [details]
icon_push.zip

zipped!
Created attachment 8685681 [details]
MozReview Request: Bug 1212606 - Prompt user (with a doorhanger) to allow to notifications from a website. r=ally

Bug 1212606 - Prompt user (with a doorhanger) to allow to notifications from a website. r=ally
Attachment #8685681 - Flags: review?(ally)
Created attachment 8685682 [details]
Screenshot: Push notification

The "Learn more" link opens the same page that desktop points to.

https://www.mozilla.org/firefox/push/
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Flags: needinfo?(liuche)
Comment on attachment 8685681 [details]
MozReview Request: Bug 1212606 - Prompt user (with a doorhanger) to allow to notifications from a website. r=ally

https://reviewboard.mozilla.org/r/24867/#review22527

r+ with naming issue addressed. Otherwise the patch looks fine and seems to behave as expected.

::: mobile/android/base/DoorHangerPopup.java:121
(Diff revision 1)
> +        } else if (DoorHanger.Type.DESKTOPNOTIFICATION2.toString().equals(typeString)) {

I understand why the strings have to have the '2' attached for localization tooling, but why do we need it in our internal java? I don't see a reason why this can't just be DESKTOPNOTIFICATION for the enum type.

If it must be a different type of desktop notification, can we name it something like DESKTOPPUSHNOTIFICATION instead?
Attachment #8685681 - Flags: review?(ally) → review+
> ::: mobile/android/base/DoorHangerPopup.java:121
> (Diff revision 1)
> > +        } else if (DoorHanger.Type.DESKTOPNOTIFICATION2.toString().equals(typeString)) {
> 
> I understand why the strings have to have the '2' attached for localization
> tooling, but why do we need it in our internal java? I don't see a reason
> why this can't just be DESKTOPNOTIFICATION for the enum type.
> 
> If it must be a different type of desktop notification, can we name it
> something like DESKTOPPUSHNOTIFICATION instead?

We do a string compare with the "type" field of the JSON message to figure out what the notification type is, and (unfortunately but as a pretty cute shortcut) the JS side links the message type to the string name to facilitate fetching the string resource for each message type. I decided not to change the JS because there are a handful of message types that rely on this same thing, and this would need to propagate.

I'll add a comment though.

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/4b2ec935bc99
Haven't seen this merged to m-c, even though it's in the Nightlies.
Flags: needinfo?(ryanvm)
Looks like it got merged last Friday but the bug wasn't marked.
https://hg.mozilla.org/mozilla-central/rev/4b2ec935bc99

Note that hg changeset hashes are immutable, so you can easily change the fx-team URL to an m-c one to verify that your patch was merged. Also note that I haven't been sheriffing since early September and no longer pay much attention to these things :)
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Tested using latest Nightly 45.0a1 (2015-11-22) on several devices, after going to facebook.com or twitter.com and logged in with a valid account, no doorhanger is displayed telling to receive notification from that site.
Flags: needinfo?(liuche)
Teodora, here's a test site that will pop up notification: https://people.mozilla.org/~ewong2/push-notification-test/

This is still a new web API, so most websites haven't implemented this yet so we won't see it on Facebook and Twitter.
Flags: needinfo?(liuche)
You need to log in before you can comment on or make changes to this bug.