958071 - PopupNotification consumers should have a way to replace the "Not Now" item

Status: RESOLVED FIXED

(Toolkit :: General, enhancement)

Description

[Florian Quèze [:florian]]

This is needed for the fix in bug 885796. dolske requested that I spin this change off to a separate bug.

Context:
(Justin Dolske [:Dolske] in bug 885796 comment #6)
> * We should add an option to PopupNotifications.show() to allow suppressing
> the default "Not Now" option. This came up with Mixed Content Blocking (MCB)
> and was mildly awkward there, but it feels really awkward here (attachment
> 8342208 [details]). An always-present "Not Now" made sense when we were only
> using doorhangers for question+action permissions, but we seem to be
> increasingly using these prompts as hidden-by-default informational panels,
> where it doesn't seem to make much sense.
> 
> * I think it would be helpful to have an explicit "Continue sharing"
> secondary option (which would do nothing). That feels a lot more natural
> than "Not Now", and provides a clear choice that isn't "Stop".
> 
> * ...upon which you'll find that, as with MCB, you can't have a secondary
> action that keeps the doorhanger around. Clicking "Continue sharing" would
> make the icon go away. :( But both this bug and MCB really just want to
> relabel the "Not Now" do-nothing action, so maybe that's the easy way to fix
> all of this (an option to .show() that provides a new label+accesskey for
> the "not now" action.
> 
> * What should the default action here be? The dialog isn't shown by default,
> so it kinda seems like the default should be "Continue Sharing" (eg, curious
> user clicks it, so default is the change-nothing choice). Especially since
> this seems purposed as fallback UI if the page itself doesn't make it clear
> how to stop sharing. I guess I don't feel very strong about it either way,
> but wanted to see if it was considered.


(In reply to Justin Dolske [:Dolske] from bug 885796 comment #14)
> Comment on attachment 8357186 [details] [diff] [review]
> Patch v3 unbitrotted
> 
> Review of attachment 8357186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bonus points if you spin off the PopupNotification.jsm/notification.xml to a
> separate bug.
> 
> r+ with the buttontype change.
> 
> ::: toolkit/modules/PopupNotifications.jsm
> @@ +195,5 @@
> >     *          - label (string): the button's label.
> >     *          - accessKey (string): the button's accessKey.
> >     *          - callback (function): a callback to be invoked when the button is
> > +   *            pressed. If the function returns true, the notification will be
> > +   *            dismissed instead of removed.
> 
> I'm a little bit wary of doing this -- previously the return value was
> ignored, and so there could be existing callers returning true
> (pointlessly), which would now break.
> 
> Not sure how big a deal that is without auditing some existing callers. A
> workaround, if we wanted, would be to have another |options| flag to opt-in
> to this.
> 
> Gavin knows this code better -- any thoughts? [Also, I vague remember
> something about wanting to make the callback invocation async, not sure if
> that matters here?]
> 
> @@ +556,5 @@
> > +          popupnotification.setAttribute("hidenotnow", "true");
> > +          // If 'Not Now' is hidden and there's no secondary action,
> > +          // make the menubutton a regular button.
> > +          if (!n.secondaryActions.length)
> > +            popupnotification.setAttribute("buttontype", "");
> 
> I'd prefer to avoid helping callers make poorly-designed single action
> prompts unless there's a good / unavoidable reason for it. The intent here
> with |hidenotnow| is to hide the default "Not now" because the caller is
> providing a _replacement_.
> 
> Just set the attribute here, and add a check in show() to require that if
> you set |hidenotnow| you must also provide at least one secondary action.


I'm attaching a patch addressing these comments and moving here the needinfo? request for Gavin.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch v4 from bug 885796, review comments addressed

Bugzilla dropped my attachment... Attaching again, and carrying forward dolske's r+ from bug 885796.

Comment 3

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #14)

> ::: toolkit/modules/PopupNotifications.jsm

> >     *          - callback (function): a callback to be invoked when the button is
> > +   *            pressed. If the function returns true, the notification will be
> > +   *            dismissed instead of removed.
> 
> I'm a little bit wary of doing this -- previously the return value was
> ignored, and so there could be existing callers returning true
> (pointlessly), which would now break.
> 
> Not sure how big a deal that is without auditing some existing callers. A
> workaround, if we wanted, would be to have another |options| flag to opt-in
> to this.

I've audited all the existing callers I could find in mozilla-central. None of them return a value. Of course that doesn't guarantee that add-ons don't return a value.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

We do want to make the callbacks be invoked asynchronously at some point, I think. I think that suggests not using their return value as an indicator of whether to keep the notification would be best. A new options flag as dolske suggests would probably be better.

> If 'Not Now' is hidden, providing at least one secondary action is required.

Why this requirement?

Comment 5

[Florian Quèze [:florian]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #4)

> > If 'Not Now' is hidden, providing at least one secondary action is required.
> 
> Why this requirement?

My previous patch didn't have this requirement, I added it per dolske's request:

(Justin Dolske [:Dolske] in bug 885796 comment #14)

> @@ +556,5 @@
> > +          popupnotification.setAttribute("hidenotnow", "true");
> > +          // If 'Not Now' is hidden and there's no secondary action,
> > +          // make the menubutton a regular button.
> > +          if (!n.secondaryActions.length)
> > +            popupnotification.setAttribute("buttontype", "");
> 
> I'd prefer to avoid helping callers make poorly-designed single action
> prompts unless there's a good / unavoidable reason for it. The intent here
> with |hidenotnow| is to hide the default "Not now" because the caller is
> providing a _replacement_.
> 
> Just set the attribute here, and add a check in show() to require that if
> you set |hidenotnow| you must also provide at least one secondary action.


So I guess you want an answer from dolske here.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> (Justin Dolske [:Dolske] in bug 885796 comment #14)
> > I'd prefer to avoid helping callers make poorly-designed single action
> > prompts unless there's a good / unavoidable reason for it.

Missed this. I suppose the concern is the same one that led to the addition of "Not Now"? I'm not sure whether that should really be enforced at the API level (we can't enforce that the secondary action provided is a suitable replacement).

Comment 7

[Justin Dolske [:Dolske]]

Sure, we can't enforce "suitable". The intent of doorhangers was to always have at least a [Do Something] and a [Do Nothing] action, that implies there should always be 1 (or more) secondary actions. The narrow scope of what we're providing here is the ability to replace the Not Now item; enabling the creation of doorhangers with just a "Ok" button doesn't feel like something we need to do. We can revisit if it's needed in the future.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

OK

Comment 9

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #7)
> Sure, we can't enforce "suitable".

Actually, if instead of the return value I use an additional option of the action object, as requested in comment 4, it becomes possible to enforce that there's at least one action that doesn't cause the notification to be removed. Would you like me to enforce that in the next patch?

Comment 10

[Justin Dolske [:Dolske]]

Oh, that's clever. Yeah, that seems like it would work. If any provided action has a "don't dismiss when clicked", we can assume that's the do-nothing item since the notification remains.

I suppose it's possible that in the future we might want some kind of "don't do anything when clicked, because the caller will later handle removing the notification itself". But we need not worry about that here -- I think it's unlikely/uncommon, and if we need it a different opt-in would suffice.

Comment 11

[Florian Quèze [:florian]]

Attachment: bug958071-2.patch

Updated patch taking into account the decisions from the discussion in comment 4 to comment 10.

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/22737775ef32

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22737775ef32