1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler (ex-Mozilla)]

From Tanvi, in bug 1192458, comment #52:

> The permission is now granting access to two things when it formally only
> granted access to one.  The text is also different ("receive"
> notifications), the Learn More link will explain that this is for local
> notifications and Push, and there is a new icon (I think?).  Hence, we
> should re-prompt to get permission from the user to enable this expanded
> privilege.  A one time prompt is not that bad from a user experience point
> of view.

Implementation-wise, since we have a single permission, what's the best way to migrate existing grants? Drop `desktop-notification` from the permissions DB and use a different name?

Comment 1

[Matthew N. [:MattN]]

I still don't think we should do this due to the added technical complexity. We're using the same permission prompt code for both but to do this properly we would have to treat push requests different than local notification requests since I don't think it's acceptable to simply drop the existing desktop-notification permission on upgrade if the site isn't using Push (e.g. Gmail, IRCCloud, etc.).

Comment 2

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] from comment #1)
> I still don't think we should do this due to the added technical complexity.
> We're using the same permission prompt code for both but to do this properly
> we would have to treat push requests different than local notification
> requests since I don't think it's acceptable to simply drop the existing
> desktop-notification permission on upgrade if the site isn't using Push
> (e.g. Gmail, IRCCloud, etc.).

The site may not be using push now, but it could be in the future.  By granting permission to "receive notifications" we are allowing both local notifications and push requests by that website.  We don't need to account for whether they are actually using push today or not.  If either push or local notification permission is requested in FF 44+, we should prompt as if it were a brand new permission.  Then we can store the value for future interactions with that site.

Comment 3

[Matthew N. [:MattN]]

I don't think intentionally losing the permission upon upgrade to Fx44 is an acceptable UX though. Most will think Firefox has a bug that lost their permission and it will reflect poorly on us.

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] from comment #3)
> I don't think intentionally losing the permission upon upgrade to Fx44 is an
> acceptable UX though. Most will think Firefox has a bug that lost their
> permission and it will reflect poorly on us.

We can't give websites the Push permission without asking the user to grant it.

Comment 5

[Matthew N. [:MattN]]

Another option to notifying users about the change without either data loss or technical complexity in the prompt code would be a one-time notification upon upgrade to Fx44.

Comment 6

[Edwin Wong [:edwong]]

Bill and Philipp to make a decision

Comment 7

[Matthew N. [:MattN]]

The plan is in bug 1217990 and this is the implementation bug. I don't think we have a good way to handle one-time migration-like things in a product-agnostic way so we may want to just put this in _migrateUI and have a separate bug for mobile.

Comment 8

[Lina Butler (ex-Mozilla)]

Attachment: MozReview Request: Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r=MattN

Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r?MattN

Comment 9

[Lina Butler (ex-Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a93ec65e7518

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8679670 [details]
MozReview Request: Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r=MattN

https://reviewboard.mozilla.org/r/23505/#review21083

r+ but I wouldn't mind taking a look at the test after changes.

::: browser/base/content/test/alerts/browser_notification_permission_migration.js:18
(Diff revision 1)
> +  let alertWindowPromise = new Promise(function(resolve, reject) {
> +    let listener = {
> +      onOpenWindow(window) {
> +        Services.wm.removeListener(listener);
> +        let alertWindow = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow);
> +        alertWindow.addEventListener("load", function onLoad() {
> +          alertWindow.removeEventListener("load", onLoad);
> +          resolve(alertWindow);
> +        });
> +      },
> +    };
> +    Services.wm.addListener(listener);
> +  });

Can you make this a helper in head.js too? You should check that it's an "alert:alert" window too.

I think there may also be custom alert notifications you could use

::: browser/base/content/test/alerts/browser_notification_permission_migration.js:48
(Diff revision 1)
> +add_task(function* cleanup() {
> +  Services.prefs.clearUserPref("browser.migration.version");

Hmm… you're leaving the browser in a different state after the test instead of restoring the version number. I think pushPrefEnv might roll this back to the correct old value for you otherwise you should remember it and manually restore.

::: browser/base/content/test/alerts/head.js:1
(Diff revision 1)
> +function waitForCloseWindow(window) {

How about promiseAlertClosed and check for windowtype "alert:alert"? I think there are already b-c helpers in BrowserTestUtils for all windows.

::: browser/components/nsBrowserGlue.js:162
(Diff revision 1)
> +  catch (e) {
> +    // nsIAlertsService is not available for this platform

I really don't think there is such a desktop platform because we have the XUL fallback but perhaps this is possible. We may be able to use defineLazyServiceGetter instead?

::: browser/components/nsBrowserGlue.js:2271
(Diff revision 1)
> +      AlertsService.showAlertNotification(null, title, text,
> +                                          true, url, clickCallback);

We discussed showing chrome://browser/skin/web-notifications-icon.svg as the image to connect it to the doorhanger prompt and I think it makes it look nicer (especially with XUL notifications)

::: browser/components/nsBrowserGlue.js:2273
(Diff revision 1)
> +    } catch(e) {}

Please do Cu.reportError(e) and put a space after `catch`

::: browser/locales/en-US/chrome/browser/browser.properties:385
(Diff revision 1)
> +webNotifications.upgradeTitle=Notifications have been upgraded
> +webNotifications.upgradeInfo=You will now be able to receive notifications from your favorite sites, even when they are not opened in a tab. Click to learn more.

Reminder to update these from bug 1217990 comment 19

Comment 11

[Lina Butler (ex-Mozilla)]

Comment on attachment 8679670 [details]
MozReview Request: Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r=MattN

Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r?MattN

Comment 12

[Lina Butler (ex-Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1482b79e0bba

Comment 13

[Lina Butler (ex-Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef8de3ab5c2d

Comment 14

[Lina Butler (ex-Mozilla)]

https://reviewboard.mozilla.org/r/23505/#review21083

> Can you make this a helper in head.js too? You should check that it's an "alert:alert" window too.
> 
> I think there may also be custom alert notifications you could use

Turns out `alertWindow.document.documentElement.getAttribute("windowtype")` works once the page is loaded. Yay!

> How about promiseAlertClosed and check for windowtype "alert:alert"? I think there are already b-c helpers in BrowserTestUtils for all windows.

Renamed and added a comment. It's identical to `BrowserTestUtils.closeWindow`, except it doesn't close the window. I tried using that first, but noticed it could hang the test if the synthetic click already dismissed the notification.

Comment 15

[Pulsebot]

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

Comment 16

[Carsten Book [:Tomcat]]

sorry had to back this out for test failurs like https://treeherder.mozilla.org/logviewer.html#?job_id=5452740&repo=fx-team

Comment 17

[Pulsebot]

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

Comment 18

[Lina Butler (ex-Mozilla)]

Comment on attachment 8679670 [details]
MozReview Request: Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r=MattN

Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r=MattN

Comment 19

[Lina Butler (ex-Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7c49f0692a0

Comment 20

[Lina Butler (ex-Mozilla)]

Attachment: 285855.patch

Fixed the test to use `about:robots` instead of the default "learn more" URL.

Comment 21

[Lina Butler (ex-Mozilla)]

Attachment: 1216271.patch

Replaced `about:robots` with a fake URL in `testing/profiles/prefs_general.js`, per :MattN's suggestion.

Comment 22

[Pulsebot]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43d5e54e94ef

Comment 24

[Francesco Lodolo [:flod]]

https://hg.mozilla.org/mozilla-central/diff/43d5e54e94ef/browser/locales/en-US/chrome/browser/browser.properties

"You will receive notifications from sites, even those not open in a tab. Click to learn more."

I found this a bit confusing. "Those open in a tab" should probably read "those not currently displayed in a tab" or similar. 

Is there a hard-limit on the amount of characters we can display, based on bug 1217990? If there is, it would be worth adding a comment in the .properties file, explaining where the string is used and its limitations.

Comment 25

[Tanvi Vyas[:tanvi]]

When I test this on Nightly and set a timer, the prompt only shows up for just under 5 seconds.  It shows up before the browser window has opened and closes shortly after.  Why doesn't the notification stay open for 10.6 seconds?

Comment 26

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #25)
> When I test this on Nightly and set a timer, the prompt only shows up for
> just under 5 seconds.  It shows up before the browser window has opened and
> closes shortly after.  Why doesn't the notification stay open for 10.6
> seconds?

There might be a bug here. If I click the notification before the browser window opens, the learn more page never opens.

Comment 27

[Matthew N. [:MattN]]

(In reply to Francesco Lodolo [:flod] from comment #24)
> https://hg.mozilla.org/mozilla-central/diff/43d5e54e94ef/browser/locales/en-
> US/chrome/browser/browser.properties
> 
> "You will receive notifications from sites, even those not open in a tab.
> Click to learn more."
> 
> I found this a bit confusing. "Those open in a tab" should probably read
> "those not currently displayed in a tab" or similar. 

I'm not sure why this is confusing? The tab doesn't have to be displayed (e.g. in a different tab group) to show a notification in the old system, it just had to be opened+loaded in a tab.

> Is there a hard-limit on the amount of characters we can display, based on
> bug 1217990? If there is, it would be worth adding a comment in the
> .properties file, explaining where the string is used and its limitations.

I had that in the version I landed which got backed out [1] along with a style fix. I'll re-land them

[1] https://hg.mozilla.org/integration/fx-team/rev/3a615fbeed50

Comment 28

[Matthew N. [:MattN]]

(In reply to Tanvi Vyas [:tanvi] from comment #25)
> When I test this on Nightly and set a timer, the prompt only shows up for
> just under 5 seconds.  It shows up before the browser window has opened and
> closes shortly after.  Why doesn't the notification stay open for 10.6
> seconds?

We aren't doing any special duration for this notification as we don't currently have support for that.

(In reply to Tanvi Vyas [:tanvi] from comment #26)
> There might be a bug here. If I click the notification before the browser
> window opens, the learn more page never opens.

That makes sense. Can you file this as a follow-up to make tracking uplift easier?

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8680880 [details] [diff] [review]
1216271.patch

Approval Request Comment
[Feature/regressing bug #]: Notifications
[User impact if declined]: No info about permission change
[Describe test coverage new/current, TreeHerder]: m-bc on m-c
[Risks and why]: Low-medium risk since on startup path
[String/UUID change made/needed]: Yes, strings for prompt.

Comment 30

[Martin Thomson [:mt:]]

Note that this isn't showing all the text on Mac OS X.  I get a notification that ends in "Cl..."

Comment 31

[Matthew N. [:MattN]]

It varies by OS X version and didn't seem like something worth worrying about since it's a one-time thing and most users are on Windows.

Comment 32

[Pulsebot]

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

Comment 33

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d509b5c1a0f

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4588cec1d882

Comment 35

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] from comment #28)
> (In reply to Tanvi Vyas [:tanvi] from comment #25)
> > When I test this on Nightly and set a timer, the prompt only shows up for
> > just under 5 seconds.  It shows up before the browser window has opened and
> > closes shortly after.  Why doesn't the notification stay open for 10.6
> > seconds?
> 
> We aren't doing any special duration for this notification as we don't
> currently have support for that.
> 
> (In reply to Tanvi Vyas [:tanvi] from comment #26)
> > There might be a bug here. If I click the notification before the browser
> > window opens, the learn more page never opens.
> 
> That makes sense. Can you file this as a follow-up to make tracking uplift
> easier?

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1220527.

Given the above bugs, we may need to reprompt again in a future version of aurora.  Also, we should consider prompting 3 times if the notification is displayed for such a short period of time.  From our discussion on this, I thought we had 10-12 seconds on all platforms, but I see now that that is not the case.

Comment 36

[Matthew N. [:MattN]]

(In reply to Tanvi Vyas [:tanvi] from comment #35)
> Given the above bugs, we may need to reprompt again in a future version of
> aurora.

I disagree since Aurora is a pre-release channel and users there are exposed to much worse (e.g. security bugs).

> Also, we should consider prompting 3 times if the notification is
> displayed for such a short period of time.  From our discussion on this, I
> thought we had 10-12 seconds on all platforms, but I see now that that is
> not the case.

In cases where they're not using XUL notifications (where we control the duration) there is usually a system tray/center where the notification will reside until manually cleared by the user which is much longer than 10-12 seconds so I don't think this is necessary.

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8680880 [details] [diff] [review]
1216271.patch

This landed under a blanket approval due to the early merge day in 42 cycle.

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/43d5e54e94ef
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4588cec1d882

Comment 39

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4d509b5c1a0f