Closed Bug 1212606 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed
fennec 45+ ---

People

(Reporter: antlam, Assigned: liuche)

References

Details

Attachments

(4 files, 1 obsolete file)

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')
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.
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
(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+
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)
Attached image prev_push_prompt1.png (obsolete) —
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 :)
Attached image prev_push_prompt2.png
I found the icon!
Attachment #8677054 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attached file icon_push.zip
zipped!
Bug 1212606 - Prompt user (with a doorhanger) to allow to notifications from a website. r=ally
Attachment #8685681 - Flags: review?(ally)
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.
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
Closed: 4 years ago
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.