Notify content when a Push permissions pop-up is dismissed by the user.

VERIFIED FIXED in Firefox 47

Status

()

Toolkit
Notifications and Alerts
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: canuckistani, Assigned: lina)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed)

Details

MozReview Requests

()

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

Attachments

(8 attachments, 5 obsolete attachments)

58 bytes, text/x-review-board-request
past
: review+
wchen
: review+
Details | Review
58 bytes, text/x-review-board-request
wchen
: review+
Details | Review
300.18 KB, image/gif
Details
332.62 KB, image/gif
Details
263.65 KB, image/gif
Details
323.50 KB, image/gif
Details
436.48 KB, image/gif
Details
428.69 KB, image/gif
Details
Currently our permissions pop-up provides no signal to the page that called it that the user has dismissed the pop-up without any interaction. Partners ( FB ) have indicated this is different from chrome ( they reject the Promise ) and the ability to handle a rejected promise in this case has affected their design process.

We have future plans for improving our permissions notifications but I would prefer to provide FB and others implementing Push with chrome-parity behaviour in this specific case in order to facilitate their deployment. I'd be supportive of uplifting a fix for this bug as far as beta assuming it is low risk.

Comment 1

2 years ago
This is related to bug 675533 and bug 1241749. There's a lot of discussion in those bugs but I'll leave to to eng on which bug they want to use in reject the promise on dismissal.

Updated

2 years ago
Blocks: 1259207
I think the simplest thing that doesn't involve a UX change is to call the `requestPermission` callback with the permission value of "default" when the doorhanger closes and leave the promise alone since the Promise can't express the three states we have.

Anne, do you think this makes sense for a spec change?
Flags: needinfo?(annevk)
Ooh, that's a good point. We could even do the same for the promise, since we never reject it; if the user declines, we fulfill with `"denied"`.
(In reply to Kit Cambridge [:kitcambridge] from comment #3)
> Ooh, that's a good point. We could even do the same for the promise, since
> we never reject it; if the user declines, we fulfill with `"denied"`.

No, that would require a UX change as you can't resolve a promise more than once and dismissing would permit an allow or deny later with the current UX.
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #4)
> No, that would require a UX change as you can't resolve a promise more than
> once and dismissing would permit an allow or deny later with the current UX.

Oh, I see now. I thought we'd report `"default"` if the user dismisses, then ignore the choice if the user allows or denies later. You're suggesting we call the callback multiple times, with "default" for every time the user dismisses the prompt. That makes sense.
I would add a `dismiss` method to `nsIContentPermissionRequest` or even better would be to add it to a subclass (e.g. nsIContentPermissionRequestDismissble or nsIDismissbleContentPermissionRequest) so we don't need to update all consumers that don't want/need this new method. 

* Then update https://mxr.mozilla.org/mozilla-central/source/dom/push/Push.js?rev=f821dec83eed#154 with the additional interface and method.
* https://mxr.mozilla.org/mozilla-central/source/dom/notification/Notification.cpp?rev=4f59de9dba41#585 would move inside `NotificationPermissionRequest::ResolvePromise()` (perhaps with a rename).
*  _promptWebNotifications and _showPrompt in nsBrowserGlue would need to listen to the dismissals to pass them down to DOM code

I've skipped some intermediate details but this gives a starting point for changes.

Comment 8

2 years ago
I didn't know we used Notification.requestPermission() also for push notifications. That seems a little weird? In any event, the callback is supposed to be invoked once. I'm not sure invoking it multiple times will do the user of the API much good and such a mechanism wouldn't scale to all the other permissions.

I'm not really sure what a better way out here would be though, we might just end up being forced to copy Chrome's rather odd model I suppose.

Tanvi, Martin, Mounir, thoughts?
Flags: needinfo?(tanvi)
Flags: needinfo?(mounir)
Flags: needinfo?(martin.thomson)
Flags: needinfo?(annevk)
Created attachment 8734427 [details]
MozReview Request: Bug 1259148 - Notify content when a Push permissions pop-up is dismissed by the user

Review commit: https://reviewboard.mozilla.org/r/42229/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42229/
Here is my understanding of what Matt suggested to help speed things up. It doesn't work yet because I don't get an nsIContentPermissionRequestDismissible in _promptWebNotifications for some reason, but seems reasonable to me. 

This is assuming that the consensus ends up being that we should take this approach of course.
(In reply to Anne (:annevk) from comment #8)
> I didn't know we used Notification.requestPermission() also for push
> notifications. That seems a little weird? In any event, the callback is
> supposed to be invoked once. I'm not sure invoking it multiple times will do
> the user of the API much good and such a mechanism wouldn't scale to all the
> other permissions.

This is a short-term fix for a push partner who wants to know about dismissal. I realize it won't work for all other permissions. Allowing the callback to be called again after it's called with a value of "default" doesn't seem like a big deal to me.

> I'm not really sure what a better way out here would be though, we might
> just end up being forced to copy Chrome's rather odd model I suppose.

That is the long term plan with UX changes but we have partners that want to know about dismissal in the short term before the UX changes are ready.
(In reply to Panos Astithas [:past] from comment #10)
> Here is my understanding of what Matt suggested to help speed things up. It
> doesn't work yet because I don't get an
> nsIContentPermissionRequestDismissible in _promptWebNotifications for some
> reason, but seems reasonable to me. 

Looks like this matches what I was describing. Thanks!

Comment 13

2 years ago
Well, you're breaking the contract specified by the standard. I guess I can't stop you, but that doesn't seem like a great idea. Maybe it works for this particular partner, but others might get confused or end up with broken code.
This is a request we have had multiple times for the getUserMedia prompt.  This is something we have resisted.  I think that we should continue to resist and instead make the dismissal more difficult to do (as we have already discussed doing).  See Adam Roach's email to push@ recently which listed:

bug 675533 	Content permission prompts don't notify content when dismissed (X, Not now, click outside, etc.)
bug 895971 	getUserMedia permission dialog should be a tab-modal prompt
bug 947266 	Navigator.getUserMedia should either remove or let webpages detect 'Not now' case
bug 1004055 	Users don't know where the doorhanger went
bug 1004061 	doorhanger should persist if the user switches app and back to the browser again
bug 1004392 	getUserMedia doorhanger is too complicated
bug 1064257 	[UX] Unify and improve behavior of doorhanger dialogs
bug 1081248 	[UX] Should doorhangers be less transient under some circumstances?
bug 1087947 	find solution for improving Firefox gUM flow for https://hello.firefox.com
bug 1119841 	[UX] Figure out a way to make the device permissions doorhanger harder to miss
bug 1171078 	easy for user to get stuck in apparently-disabled screen-share flow
bug 1236352 	pushManager.subscribe does not resolve or reject the promise on closing the permission prompt
bug 1241749 	Permission prompt does not notify the script when the user dismisses it with [X] or [Not Now]
bug 1242614 	subscribe() should reject with PermissionDeniedError if the user doesn't grant the permission
Flags: needinfo?(martin.thomson)
(In reply to Anne (:annevk) from comment #13)
> Well, you're breaking the contract specified by the standard. I guess I
> can't stop you, but that doesn't seem like a great idea. Maybe it works for
> this particular partner, but others might get confused or end up with broken
> code.

Yeah, I'm suggesting we can either change the spec or violate it for the short term. The spec isn't compatible with our UI so that's kinda a spec problem in the first place. Are you worried about code that had an `} else {` inside of the callback assuming that it would only be called for "granted" or "denied"?

(In reply to Martin Thomson [:mt:] from comment #14)
> This is a request we have had multiple times for the getUserMedia prompt. 
> This is something we have resisted.  I think that we should continue to
> resist and instead make the dismissal more difficult to do (as we have
> already discussed doing).

It sounds like you're saying we shouldn't improve the situation in the short term (with uplifts) until we have the new Firefox UX which is many Nightly cycles away. We have a partner who chose a certain site UX to show the permission prompt which isn't good for users without knowing about dismissal.

Comment 16

2 years ago
I'm not sure what would break. That's always hard to speculate. Sites usually find a way. As for the specification not supporting Firefox's UI, per the specification dismissal is up to the user agent to decide. Apparently in Chrome it means denied, in Firefox it means no decision has yet been made. No decision being made maps perfectly to forever pending.
Dismissing the doorhanger in Chrome will call the callback with "default", so I think developers can already expect this.

It's not clear to me why we would need to call the callback multiple times, though. If the user accidentally dismisses the prompt, the site can show an in-content info dialog and call `Notification.requestPermission()` again:

    Notification.requestPermission(function handlePermission(permission) {
      if (permission == "default") {
        MyApp.showInfoDialog("Please enable notifications", function overlayShown() {
          Notification.requestPermission(handlePermission);
        });
        return;
      }
      // Handle "granted" / "denied".
    });

If we called the callback multiple times, that (somewhat contrived) example would be incomplete: we'd need an extra boolean flag to make sure we don't handle "granted" or "denied" more than once. So I think the simpler approach here is to not allow the user to make a decision later (don't let the doorhanger be recalled), and let the site ask again.

I realize that's a UX change, and inconsistent with how our other doorhangers work. However, we're already talking about an exception for this one permission, and I think it's OK to have this inconsistency instead of changing the behavior of the callback.
(In reply to Martin Thomson [:mt:] from comment #14)
> This is a request we have had multiple times for the getUserMedia prompt. 
> This is something we have resisted.  I think that we should continue to
> resist and instead make the dismissal more difficult to do (as we have
> already discussed doing).  

I think instead the right thing for Push adoption is to mimic Chrome in this particular instance. Calling the callback with the 'default' value at least will be unsurprising to developers who have already worked with chrome.

It's up to browser vendors to implement these permissions dialogs, and ours are not great right now. I'm happy to move forward with the larger plan for these things in a few cycles but I think the thin edge of the wedge for Push is this particular partner's integration. I would rather bear the overhead of a small inconsistency on our side for a while and prioritize consistency in experience with chrome to reduce headaches for developers.
If the forever pending state is preventing Firefox push adoption, then lets reply with "default" or reply with "denied" when the user dismisses the notification.  Removing the ability to dismiss completely from the UX is a disservice to our users, since it forces them to make a choice and interrupts them from their current task.  Besides the annoyance, users may end up making abrupt ill-informed choice in order to get rid of the doorhanger and get back to the task at hand.
Flags: needinfo?(tanvi)
(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #19)
> If the forever pending state is preventing Firefox push adoption, then lets
> reply with "default" or reply with "denied" when the user dismisses the
> notification.  

Agreed.

> Removing the ability to dismiss completely from the UX is a
> disservice to our users, since it forces them to make a choice and
> interrupts them from their current task.  Besides the annoyance, users may
> end up making abrupt ill-informed choice in order to get rid of the
> doorhanger and get back to the task at hand.

I think the balance we're trying to strike with the proposed work is to reduce the case where users accidentally dismiss the permissions pop-up. That work should not block what I'm proposing here.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Component: DOM: Push Notifications → Notifications and Alerts
Product: Core → Toolkit
FWIW, resolving the request with 'prompt' is also how navigator.permissions.request() is meant to work. Having Navigator.requestPermissions() call the callback with 'default' would make sense to me.
Flags: needinfo?(mounir)
Created attachment 8736528 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. f?past,MattN

Review commit: https://reviewboard.mozilla.org/r/43347/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43347/
Comment on attachment 8736528 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. f?past,MattN

Thanks for the patch, Panos! It looks like we'll need to update https://dxr.mozilla.org/mozilla-central/source/dom/ipc/PContentPermissionRequest.ipdl to proxy `Dismiss`, too. If you'd like to keep the `nsIContentPermissionRequestDismissible` interface, I'm happy to carry this through and make the necessary changes.

I went with a slightly different approach and overloaded `nsIContentPermissionRequest.cancel` to mean "user denied permission" and "user dismissed the doorhanger". `NotificationPermissionRequest::ResolvePromise` then queries the permission manager to distinguish between the two. We trade the additional interface for some complexity in the Notification API.

This patch implements the behavior I suggested in comment 17, which is (AFAICT) how Chrome behaves now.
Attachment #8736528 - Flags: feedback?(past)
Attachment #8736528 - Flags: feedback?(MattN+bmo)
Comment on attachment 8736528 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. f?past,MattN

Thanks Kit, I'm fine with this approach too.
Attachment #8736528 - Flags: feedback?(past) → feedback+

Comment 25

2 years ago
I filed https://github.com/whatwg/notifications/issues/70 to align the standard.
Duplicate of this bug: 1255314
(In reply to Kit Cambridge [:kitcambridge] from comment #17)
> If we called the callback multiple times, that (somewhat contrived) example
> would be incomplete: we'd need an extra boolean flag to make sure we don't
> handle "granted" or "denied" more than once.

I don't really agree since in-practice the site wouldn't get "granted" or "denied" more than once without the user interfering. If there are multiple calls, wouldn't the first ones always be "default" (ignoring the case where a user manually changes their mind from Control Center without reloading)?
https://reviewboard.mozilla.org/r/43347/#review40929

::: dom/notification/Notification.cpp:653
(Diff revision 1)
>  NotificationPermissionRequest::ResolvePromise()
>  {
>    nsresult rv = NS_OK;
> +  if (mPermission == NotificationPermission::Default) {
> +    // This will still be "default" if the user dismissed the doorhanger,
> +    // or "denied" otherwise.
> +    mPermission = Notification::TestPermission(mPrincipal);

I thought we didn't want to resolve the promise, only call the callback. That seems to align with discussion at https://github.com/whatwg/notifications/issues/70

::: toolkit/components/telemetry/Histograms.json:10096
(Diff revision 1)
> +  "PUSH_API_PERMISSION_DISMISSED": {
> +    "alert_emails": ["push@mozilla.com"],
> +    "expires_in_version": "55",

This is duplicating info already in the popup notification probe so I'm not sure why we're gathering it in multiple places…
Thanks, Matthew! I mulled this over some more, and I don't think we should implement different behaviors for the callback and promise.

The callback is deprecated, and an addition that invokes it multiple times seems surprising. That's better suited for an event target (like 'PermissionStatus.onchange' in the Permissions API), or a method that implies multiple dispatch like 'navigator.geolocation.watchPosition'. ISTM the expectation here is "either permission is granted or it's not."

I think we should follow Chrome's behavior here and resolve with "default" exactly once if the user cancels. Left a comment on the spec bug requesting clarification.

(In reply to Matthew N. [:MattN] from comment #27)
> (In reply to Kit Cambridge [:kitcambridge] from comment #17)
> > If we called the callback multiple times, that (somewhat contrived) example
> > would be incomplete: we'd need an extra boolean flag to make sure we don't
> > handle "granted" or "denied" more than once.
> 
> I don't really agree since in-practice the site wouldn't get "granted" or
> "denied" more than once without the user interfering.

That's very true. If the user is unlikely to interfere in practice (as shown by "happened after reopen" for POPUP_NOTIFICATION_STATS), why work around it in the standard when we can avoid id entirely in the UI?
OK, it sounds like the spec change will apply to both signatures, per https://github.com/whatwg/notifications/issues/70#issuecomment-206139841.

Matt, any chance you're willing to reconsider resolving with `"default"`, and removing the doorhanger icon from the address bar if it's dismissed?
Flags: needinfo?(MattN+bmo)
(In reply to Kit Cambridge [:kitcambridge] from comment #30)
> Matt, any chance you're willing to reconsider resolving with `"default"`,
> and removing the doorhanger icon from the address bar if it's dismissed?

Changing the callback is sufficient to address this bug so I think that's all we should do. I think making the UX change with the future UI change is less surprising for users.
Flags: needinfo?(MattN+bmo)
MattN and I discussed this in person today. Per our discussion, I'm going to finish the patch that Panos started. So the behavior will be:

* If the doorhanger is dismissed (either explicitly by clicking the "x," or implicitly by clicking outside of it), the callback will be called with "default". The promise will *not* be resolved.
* If the user then restores the doorhanger and makes a final decision, the callback will be called with "granted" or "denied," and the promise will be resolved with the same value.
* If the doorhanger is restored and the user closes it again, without making a decision, the callback will be called again with "default".

I'm not fully convinced that calling the callback multiple times is web compatible. I'm also nervous about introducing an inconsistency between the two signatures, and diverging from Chrome's behavior. I'd feel much better about this approach if we:

* Emphasized the distinction in the spec. Matt suggested "final decision" for terminology, and we should also make sure to note that the callback can be called several times if the browser UI supports dismissal.
* Deprecated `Notification.requestPermission` once we support the Permissions API.
* Provisioned for this case in the redesign. This isn't very discoverable right now, and ISTM the new doorhangers (https://invis.io/Z56G11LCV) are modal. This suggests we would introduce complexity into the spec, and our implementation, for minimal gain.
One additional point: this approach only works for `Notification.requestPermission`. Since the Push API doesn't have a separate way to ask for permission (`subscribe()` uses a promise and asks for permission on first use), developers will need to do something like this:

    Notification.requestPermission(status => {
      if (status == "granted") {
        navigator.serviceWorker.register("worker.js").then(registration => {
          return registration.pushManager.subscribe();
        }).then(subscription => {
          return sendSubscriptionToServer(subscription);
        }).catch(error => {
          return handleError(error);
        });
        return;
      } else {
        // Otherwise, the user denied permission or dismissed the doorhanger.
      }
    });

Without the `Notification.requestPermission` call, `registration.pushManager.subscribe()` can hang indefinitely if the user dismisses the doorhanger and never makes a "final choice." This also assumes that the same permission is used for notifications and push; that's true for Firefox, and I think for Chrome, too.
Created attachment 8740666 [details]
MozReview Request: Bug 1259148 - Notify content when a Push permissions pop-up is dismissed by the user

Review commit: https://reviewboard.mozilla.org/r/45873/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45873/
Attachment #8736528 - Attachment is obsolete: true
Comment on attachment 8740666 [details]
MozReview Request: Bug 1259148 - Notify content when a Push permissions pop-up is dismissed by the user

I still have objections to this particular approach. Specifically, the distinction between dismissing and a final decision adds complexity for developers, with unclear user benefit. We also can't use this for `pushManager.subscribe()`, because it asks for permission as a side effect, and returns a promise.

But I've noted those in other comments. What do you all think? :-)
Attachment #8740666 - Flags: review?(wchen)
Attachment #8740666 - Flags: review?(past)
Attachment #8740666 - Flags: review?(MattN+bmo)
Attachment #8740666 - Flags: feedback?
Attachment #8740666 - Flags: feedback?
Created attachment 8740674 [details]
MozReview Request: Bug 1259148 - Test that the Notification.requestPermission callback is called upon dismissal. r=kitcambridge,wchen

Review commit: https://reviewboard.mozilla.org/r/45881/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45881/
Attachment #8740674 - Flags: review?(wchen)
Attachment #8740674 - Flags: review?(kcambridge)
Attachment #8734427 - Attachment is obsolete: true
Comment on attachment 8740666 [details]
MozReview Request: Bug 1259148 - Notify content when a Push permissions pop-up is dismissed by the user

https://reviewboard.mozilla.org/r/45873/#review42459

LGTM. Thanks

::: browser/components/nsBrowserGlue.js:2638
(Diff revision 1)
> +          aRequest.QueryInterface(Ci.nsIContentPermissionRequestDismissible);
> +          aRequest.dismiss();
> +        } catch (error) {

I believe you can use `aRequest instanceof Ci.nsIContentPermissionRequestDismissible` instead of the try…catch since this isn't very exceptional.

::: dom/interfaces/base/nsIContentPermissionPrompt.idl:105
(Diff revision 1)
> + * Interface allows access to a content to request
> + * permission to perform a privileged operation such as
> + * push notification that can be dismissed.

Nit: "to a content"

Why not just say that this interface is to handle dismissal of permission requests?

::: dom/interfaces/base/nsIContentPermissionPrompt.idl:111
(Diff revision 1)
> +  /**
> +   * dismiss the request
> +   */

Nit: "Called when the request is dismissed" seems a bit more clear.
Attachment #8740666 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8740674 [details]
MozReview Request: Bug 1259148 - Test that the Notification.requestPermission callback is called upon dismissal. r=kitcambridge,wchen

https://reviewboard.mozilla.org/r/45881/#review42485

Looks good. Thanks so much for doing this! Left a couple of nits and questions.

::: dom/notification/test/browser/browser_permission_dismiss.js:41
(Diff revision 1)
> + * argument and calls the `task` function when the permission prompt is open.
> + *
> + * @param {Function} task Task function to run to interact with the prompt.
> + * @param {String} finalPermission the value of the callback argument indicating
> + *                                 the final permission.
> + * @param {Boolean} [expectingDefaultFirst = false] whether callbacks with

What if we made the signature something like `tabWithRequest(taskFunc, ["default", "granted"])` and compared `permsReceived` against the expected permissions?

::: dom/notification/test/browser/browser_permission_dismiss.js:47
(Diff revision 1)
> + *                                                  "default" are expected before
> + *                                                  a final answer.
> + * @return {Promise} resolving when then task function is done and the tab
> + *                   closes. The value is the array of callback values.
> + */
> +function* tabWithRequest(task, finalPermission, expectingDefaultFirst = false) {

Nit: It looks like this could be a vanilla function instead of a generator.

::: dom/notification/test/browser/browser_permission_dismiss.js:99
(Diff revision 1)
> +});
> +
> +add_task(function* test_requestPermission_granted() {
> +  let perms = yield tabWithRequest(function*() {
> +    clickDoorhangerButton(PROMPT_ALLOW_BUTTON);
> +  }, "granted", true);

This doesn't make sense to me, but I'm likely missing something. Why do we expect `"default"` first, if we click "allow"? Same for "deny" below.

::: dom/notification/test/browser/browser_permission_dismiss.js:122
(Diff revision 1)
> +
> +add_task(function* test_requestPermission_minimize_granted() {
> +  let perms = yield tabWithRequest(function*() {
> +    PopupNotifications.panel.hidePopup();
> +    PopupNotifications.getNotification("web-notifications").reshow();
> +    yield BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popupshown");

Could `popupshown` fire before `waitForEvent`? (I'm guessing "no" because of the animation, but wanted to double-check).
Attachment #8740674 - Flags: review?(kcambridge) → review+
Comment on attachment 8740666 [details]
MozReview Request: Bug 1259148 - Notify content when a Push permissions pop-up is dismissed by the user

https://reviewboard.mozilla.org/r/45873/#review42533
Attachment #8740666 - Flags: review?(past) → review+

Updated

2 years ago
Attachment #8740666 - Flags: review?(wchen)
Comment on attachment 8740666 [details]
MozReview Request: Bug 1259148 - Notify content when a Push permissions pop-up is dismissed by the user

https://reviewboard.mozilla.org/r/45873/#review42803

The code looks fine, but I don't think it's good to have requestPermission be called multiple times. From the comments it looks like the justification for doing this is to provide a short term solution while this problem is solved by UX or some other means but I don't think it's worth potentional web compat issues to do this. We can also solve it by just limiting requestPermission to be called once similar to Chrome. I do understand that in our UI the prompt is actually just minimized when the users dismisses but here is how I see it in these cases:

1) If the user grants or denies permission (makes a "final decision"), we call requestPermission once with the result and the prompt disappears.
2) If the user dimisses the prompt, we call requestPermission once with "dismiss" and the prompt minimizes.
3) If the user brings back the prompt and makes a "final decision", we do nothing. Treat this as if the user changed the permission by some other means, such as going into Page Info -> Permissions -> Receive Notifications (the icon to do this is acutally right next to the icon that brings back the prompt).
4) If the user brings back the prompt and dismisses again, we do nothing which is because we already sent a "dismiss" earlier.

The idea of restricting requestPermission to be called once was brought up earlier in the bug but I don't see a good reasons for not doing it. I understand this solution is still strange but if the goal is a short term fix then I think doing this is better than allowing requestPermission to be called multiple times. With this solution we can also resolve the promise and be consistent.

Updated

2 years ago
Attachment #8740674 - Flags: review?(wchen)
(In reply to William Chen [:wchen] from comment #40)
> The code looks fine, but I don't think it's good to have requestPermission
> be called multiple times. From the comments it looks like the justification
> for doing this is to provide a short term solution while this problem is
> solved by UX or some other means but I don't think it's worth potentional
> web compat issues to do this.

I think it's very unlikely that any web compat issue is user-facing and such a small number of people actually re-open (thus calling the callbac more than once) that I don't think web-compat concerns are a real problem. I think that the extremely rare case of the callback getting called multiple times isn't user-facing because any exceptions in the error callback aren't going to stop regular page execution. Sure, the page may not be expecting a second call (though this isn't foreign for callbacks) but as long as the permission is actually set by the platform (like it will) then I don't see how this is going to break the page from the user's perspective. It seems like the worst case is that it skews telemetry but since it's so rare for a re-open that seems like a non-issue.

> We can also solve it by just limiting
> requestPermission to be called once similar to Chrome. I do understand that
> in our UI the prompt is actually just minimized when the users dismisses but
> here is how I see it in these cases:
> 
> 1) If the user grants or denies permission (makes a "final decision"), we
> call requestPermission once with the result and the prompt disappears.
> 2) If the user dimisses the prompt, we call requestPermission once with
> "dismiss" and the prompt minimizes.
> 3) If the user brings back the prompt and makes a "final decision", we do
> nothing. Treat this as if the user changed the permission by some other
> means, such as going into Page Info -> Permissions -> Receive Notifications
> (the icon to do this is acutally right next to the icon that brings back the
> prompt).
> 4) If the user brings back the prompt and dismisses again, we do nothing
> which is because we already sent a "dismiss" earlier.

I think this proposal is better than a UX change though I don't think the web compat concern is a real issue as I said above so I would still prefer the solution implemented here. What do you think about what I said above?

> The idea of restricting requestPermission to be called once was brought up
> earlier in the bug but I don't see a good reasons for not doing it. I
> understand this solution is still strange but if the goal is a short term
> fix then I think doing this is better than allowing requestPermission to be
> called multiple times.

> With this solution we can also resolve the promise and be consistent.

I still think that most developers don't want the promise resolved with "default" as they would care more about knowing they are granted permission later (which they will miss with your proposal). It's only for spammy UX like FB where the prompt isn't user-initiated and the whole UI is blacked out that developers care about the dismissal and I think for that minority of sites they should use the callback which would notify of all request state changes. i.e. the promise is for knowing about the decision (default isn't a decision) and the callback is for knowing about all changes to the request. IMO for most sites (think ones that have a button to enable notifications like in Gmail settings), they will need to do nothing if the promise is resolved with "default" so why waste the one resolution with a no-op value when knowing about a future "granted" would allow the notifications to start working sooner.
Flags: needinfo?(wchen)
Created attachment 8743436 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past,wchen

Review commit: https://reviewboard.mozilla.org/r/47757/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47757/
Attachment #8743436 - Flags: review?(wchen)
Attachment #8743436 - Flags: review?(past)
Attachment #8743437 - Flags: review?(wchen)
Created attachment 8743437 [details]
MozReview Request: Bug 1259148 - Test that dismissing the notification permission doorhanger resolves the promise with `default`. r=wchen

Review commit: https://reviewboard.mozilla.org/r/47759/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47759/
Attachment #8740666 - Attachment is obsolete: true
Attachment #8740674 - Attachment is obsolete: true
Comment on attachment 8743437 [details]
MozReview Request: Bug 1259148 - Test that dismissing the notification permission doorhanger resolves the promise with `default`. r=wchen

wchen and I discussed this on IRC the other day and compromised on calling the callback only once but only resolving the Promise for the final decision (since that's what most sites care about and it doesn't change the UX).

Please remove my name as the author of this commit (since I don't support it) if we're implementing something different.
Flags: needinfo?(wchen)
Attachment #8743437 - Flags: review-
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #44)
> wchen and I discussed this on IRC the other day and compromised on calling
> the callback only once but only resolving the Promise for the final decision
> (since that's what most sites care about and it doesn't change the UX).

From discussions with :edwong, :clarkbw, and :ckarlof, my impression is that we're going to make the UX change and match Chrome's behavior.

It's confusing to web developers that the callback and promise behave differently. It also doesn't address the case where a site call `serviceWorkerRegistration.pushManager.subscribe()`, without calling `Notification.requestPermission` first (https://people.mozilla.org/~ewong2/pushPerms/permSWTest.html).

> Please remove my name as the author of this commit (since I don't support
> it) if we're implementing something different.

Ugh, I'm very sorry I carried that forward. I'll just push a new commit instead of amending the old one.
Flags: needinfo?(edwong)
Flags: needinfo?(clarkbw)
Flags: needinfo?(ckarlof)
Comment on attachment 8743437 [details]
MozReview Request: Bug 1259148 - Test that dismissing the notification permission doorhanger resolves the promise with `default`. r=wchen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47759/diff/1-2/
Attachment #8743437 - Attachment description: MozReview Request: Bug 1259148 - Test that the Notification.requestPermission callback is called upon dismissal. r?wchen → MozReview Request: Bug 1259148 - Test that dismissing the notification permission doorhanger resolves the promise with `default`. r?wchen
Attachment #8743437 - Flags: review-
Created attachment 8743582 [details]
chrome-notification.gif

Here's how the notifications permission behaves in Chrome 50.
Created attachment 8743583 [details]
chrome-push.gif

Push permission, Chrome 50.
Created attachment 8743584 [details]
firefox-notification-ux-change.gif

Here's how Firefox behaves with the suggested UX change.

The callback and promise are resolved with the same value, but the doorhanger icon is removed from the address bar if the user dismisses it.
Created attachment 8743585 [details]
firefox-push-ux-change.gif

And Push, with the UX change.

I'm currently building the previous patch that forks the behavior of the callback/promise instead of the UX change; will post screencaps once it's ready.
Created attachment 8743588 [details]
firefox-notification-hanging-promise.gif

Here's how Firefox behaves if we call the callback with `default`. If the doorhanger is dismissed, the icon remains in the address bar. The callback is called with `default` for every dismissal, but the promise hangs until it's resolved with a final decision ("denied" or "granted").

Comment 44 suggests changing this to where the callback is called with "default", but the promise still hangs until the final decision.
Created attachment 8743590 [details]
firefox-push-hanging-promise.gif

And here's how Push behaves. Since there's no callback API for `serviceWorkerRegistration.pushManager.subscribe()`, the promise hangs until a final decision is made.
Yes, Kit, I confirmed with clarkbw that we're going to make the UX change and match Chrome's behavior.
Flags: needinfo?(ckarlof)
Comment on attachment 8743436 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past,wchen

https://reviewboard.mozilla.org/r/47757/#review44789

I'm OK with this approach.

::: dom/push/Push.js:163
(Diff revision 1)
>        allow: function() {
>          let histogram = Services.telemetry.getHistogramById("PUSH_API_PERMISSION_GRANTED");
>          histogram.add();
>          allowCallback();
>        },
> -      cancel: function() {
> +      cancel: () => {

This seems gratuitous, no functional change is made and the sibling allow() method is not using an arrow function either. Let's use one or the other for consistency. It also breaks blame.
Attachment #8743436 - Flags: review?(past) → review+
(In reply to Chris Karlof [:ckarlof] from comment #53)
> Yes, Kit, I confirmed with clarkbw that we're going to make the UX change
> and match Chrome's behavior.

+1
Flags: needinfo?(clarkbw)
Comment on attachment 8743436 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past,wchen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47757/diff/1-2/
Attachment #8743436 - Attachment description: MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r?past,wchen → MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past r?wchen
Comment on attachment 8743437 [details]
MozReview Request: Bug 1259148 - Test that dismissing the notification permission doorhanger resolves the promise with `default`. r=wchen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47759/diff/2-3/
https://reviewboard.mozilla.org/r/47757/#review44789

> This seems gratuitous, no functional change is made and the sibling allow() method is not using an arrow function either. Let's use one or the other for consistency. It also breaks blame.

Oops; thanks! I forgot to revert this when I removed the extra probe.
Comment on attachment 8743436 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past,wchen

https://reviewboard.mozilla.org/r/47757/#review44887
Attachment #8743436 - Flags: review?(wchen) → review+
Comment on attachment 8743437 [details]
MozReview Request: Bug 1259148 - Test that dismissing the notification permission doorhanger resolves the promise with `default`. r=wchen

https://reviewboard.mozilla.org/r/47759/#review44895
Attachment #8743437 - Flags: review?(wchen) → review+
Created attachment 8744120 [details]
MozReview Request: Bug 1259148 - Allow the user to restore the doorhanger using the icon.

Review commit: https://reviewboard.mozilla.org/r/48289/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48289/
Attachment #8743436 - Attachment description: MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past r?wchen → MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past,wchen
Attachment #8743437 - Attachment description: MozReview Request: Bug 1259148 - Test that dismissing the notification permission doorhanger resolves the promise with `default`. r?wchen → MozReview Request: Bug 1259148 - Test that dismissing the notification permission doorhanger resolves the promise with `default`. r=wchen
Comment on attachment 8743436 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past,wchen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47757/diff/2-3/
Comment on attachment 8743437 [details]
MozReview Request: Bug 1259148 - Test that dismissing the notification permission doorhanger resolves the promise with `default`. r=wchen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47759/diff/3-4/
Comment on attachment 8744120 [details]
MozReview Request: Bug 1259148 - Allow the user to restore the doorhanger using the icon.

Panos, we talked about this at today's meeting: minimize the doorhanger, but only update the permission state and don't notify the site. What do you think?
Attachment #8744120 - Flags: review?(past)
Comment on attachment 8744120 [details]
MozReview Request: Bug 1259148 - Allow the user to restore the doorhanger using the icon.

On second thought, I don't think this particular patch is right.

* It doesn't solve the issue from the meeting, which is that developers will come to expect Chrome's behavior of being notified on dismissal. Even if we keep dismissal and make it more prominent in the redesign, we'd still need to notify the site to avoid breakage.
* To that end, this doesn't deter developers from creating modal UIs that require the user to grant permission.
* The site can still repeatedly ask for permission. We don't have a way to discourage origins from spamming users by denying the permission if the user ignores the doorhanger.
* It's still inconsistent with how other doorhangers work. The site is notified of the dismissal, but the final "allow" or "deny" only takes effect for subsequent visits.

Either way, we're going to end up making a one-off change for this permission. The fact that it's temporary, and the impending uplift, leads me to conclude we should make the UX change. If there are objections, we can back this out. But I'd like to give this a bit of time to bake over the weekend before the merge.

I think this is also a good starting point for follow-up bugs.
Attachment #8744120 - Flags: review?(past) → review-
Attachment #8744120 - Attachment is obsolete: true
See Also: → bug 1266634

Comment 68

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c3268c97a6c
https://hg.mozilla.org/mozilla-central/rev/8c72f449815b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

2 years ago
Flags: needinfo?(edwong)
Duplicate of this bug: 1267850
Comment on attachment 8743436 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past,wchen

Approval Request Comment
[Feature/regressing bug #]: This bug.
[User impact if declined]: Sites that use modal permission requests won't know if the user dismissed the permission doorhanger, causing the site to appear frozen or broken. This is inconsistent with Chrome, and Facebook would like this addressed before deploying Web Push for Firefox.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=8c72f449815b, along with manual verification by the SV team.
[Risks and why]: Low risk. This workaround is specific to the notifications permission, and rode the train to Aurora.
[String/UUID change made/needed]: None.
Attachment #8743436 - Flags: approval-mozilla-beta?

Updated

2 years ago
status-firefox47: --- → affected
Hi Michelle, once your validation in Nightly is complete, could you please mark this bug as verified? I am going to approve this uplift to Beta47 based on your email sign-off indicating 97% testing is complete and no blocking issues were found. Thanks!
Flags: needinfo?(mfunches)
Comment on attachment 8743436 [details]
MozReview Request: Bug 1259148 - Notify content when the notification permission pop-up is dismissed by the user. r=past,wchen

This is needed to address browser parity ask from FB, was verified on Nightly by QA, Beta47+
Attachment #8743436 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Desktop:
Always Allow > callback called with: granted, promise resolve: granted
Block > callback called with: denied, promise resolve: denied
Not Now > callback called with: default, promise resolve: default

Android Device:
Always > callback called with: granted, promise resolve: granted
Never > callback called with: denied, promise resolve: denied
Setting this to Verified; however we will not sign-off, we will continue intermittent testing and all verifications on Nightly
Status: RESOLVED → VERIFIED
Flags: needinfo?(mfunches)

Updated

2 years ago
Depends on: 1328050
You need to log in before you can comment on or make changes to this bug.