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

Status: RESOLVED DUPLICATE of bug 958071

(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, carrying forward dolske's r+, and moving here the needinfo? request for Gavin.

Comment 1

[Florian Quèze [:florian]]

Uh, bugzilla complained that I couldn't request review without setting a reviewer (I was setting the r+ flag, not requesting review), then showed a large and scary red error message about me being redirected or not having a token, then said I needed to upload the attachment again because it wasn't kept between requests, which I did... and I ended up with a second bug without the attachment.

Resolving as duplicate of the other bug where I've already set the flag on the attachment, and needinfo request. Sorry for the noise.