Closed Bug 1216271 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Push Subscriptions, defect)

Product:
Core
Component:
DOM: Push Subscriptions
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox43 --- unaffected
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file, 2 obsolete files)

1216271.patch
14.40 KB, patch
lina
: review+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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?
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.).
(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.
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.
(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.
Blocks: 1192458
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.
Bill and Philipp to make a decision
Flags: needinfo?(wmaggs)
Depends on: 1217990
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.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Flags: needinfo?(wmaggs)
Summary: Migrate existing desktop notification permissions → Display a notification upon upgrade for users who currently allow notifications for at least one site
Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r?MattN
Attachment #8679670 - Flags: review?(MattN+bmo)
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
Attachment #8679670 - Flags: review?(MattN+bmo) → review+
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
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.
sorry had to back this out for test failurs like https://treeherder.mozilla.org/logviewer.html#?job_id=5452740&repo=fx-team
Flags: needinfo?(kcambridge)
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
Attachment #8679670 - Attachment description: MozReview Request: Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r?MattN → MozReview Request: Bug 1216271 - Display a notification upon upgrade for users who currently allow notifications for at least one site. r=MattN
Attached patch 285855.patch (obsolete) — — Splinter Review 
Fixed the test to use `about:robots` instead of the default "learn more" URL.
Attachment #8679670 - Attachment is obsolete: true
Flags: needinfo?(kcambridge)
Attachment #8680675 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch 1216271.patch — — Splinter Review 
Replaced `about:robots` with a fake URL in `testing/profiles/prefs_general.js`, per :MattN's suggestion.
Attachment #8680675 - Attachment is obsolete: true
Attachment #8680880 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/43d5e54e94ef
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
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?
Flags: needinfo?(kcambridge)
(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.
(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
(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 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.
Attachment #8680880 - Flags: approval-mozilla-aurora?
Note that this isn't showing all the text on Mac OS X.  I get a notification that ends in "Cl..."
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.
Blocks: 1220527
(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.
(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 on attachment 8680880 [details] [diff] [review]
1216271.patch

This landed under a blanket approval due to the early merge day in 42 cycle.
Attachment #8680880 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
Depends on: 1357812
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: