bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.
Please report any other irregularities here.
5 years ago
13.22 KB, patch
|Details | Diff | Splinter Review|
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.
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.
Assignee: florian → nobody
No longer blocks: 885796
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 958071
You need to log in before you can comment on or make changes to this bug.