Closed Bug 1019986 Opened 6 years ago Closed 6 years ago

Clarify "Don't Show" label in Desktop Notification bar and doorhanger

Categories

(SeaMonkey :: Passwords & Permissions, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.30

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

This is very similar to bug 998787 for desktop notifications, but with a twist, hence I'm filing it as a separate bug in case it needs more discussion.

While the "Don't Share" selection for Geolocation called the cancel function without any parameters, the "Don't Show" option here comes with an argument cancelCallback(true, nsIPermissionManager.EXPIRE_SESSION); which may indicate a different behavior. Thus, I'm not sure if it is equivalent to closing the notification and thus safe to remove it. On the other hand, the notification bar wouldn't show if the default wasn't to block it and ask, thus it should effectively be equivalent.
Attached patch Proposed patch (obsolete) — Splinter Review
This patch removes the dontShowForSession code from the "showWebNotificationPrompt" method. In contrast to the Geolocation doorhanger, the order of the remaining options is already [ alwaysShow, neverShow ] and thus no further action is needed.

Like the patch in bug 998787, this works fine in a SM 2.25, but no notification shows up on current trunk builds, which likely is a different bug (and considering bug 1019583, it may indicate a more general problem).

FWIW, Firefox doesn't offer the "Don't Show" option either, thus it appears to be safe to remove it (assuming the implementations are sufficiently similar).
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #8433697 - Flags: ui-review?(neil)
Attachment #8433697 - Flags: review?(iann_bugzilla)
Comment on attachment 8433697 [details] [diff] [review]
Proposed patch

Ok, so this is not going to work. Indeed there is a semantic difference between "Don't Show" ("stop asking me about this page until I close SeaMonkey even if the site asks for permission again") vs. "Not Now" ("no permission this time but ask me again the next time the site asks for it"). Thus, the option I'm removing with this patch is a temporary "shut up" for the current session, while in the case of bug 998787 the "Don't Share" and "Not Now" options were clearly equivalent ("shut up now but ask me next time again even if I'm just reloading the page"). I will hence retarget this but to clarify the label(s).
Attachment #8433697 - Attachment is obsolete: true
Attachment #8433697 - Flags: ui-review?(neil)
Attachment #8433697 - Flags: review?(iann_bugzilla)
Summary: Remove redundant "Don't Show" option from Desktop Notification doorhanger → Clarify "Don't Show" label in Desktop Notification bar and doorhanger
Depends on: 1020630
Blocks: 595437
Blocks: 1020657
Currently, there are

> webNotifications.showForSession=Show Notifications
> webNotifications.dontShowForSession=Don't Show
> webNotifications.alwaysShow=Always Show
> webNotifications.neverShow=Never Show

where "Show Notifications" as the button title is always visible. Hence, the "Show" in the menuitems are redundant and can be replaced with some more specific information. How about:

  [Show Notifications v]
   +-----------------------+
   | Not for This Session  |
   | Always for This Site  |
   | Never for This Site   |
   |-----------------------|
   | Not Now           [x] |
   +-----------------------+

This should be unambiguous that the second item covers the full duration of the session whereas "Now" should sufficiently distinguish that the current request is meant.
Seeing that there are no immediate objections, let's give this a try.
Attachment #8436327 - Flags: ui-review?(neil)
Attachment #8436327 - Flags: review?(iann_bugzilla)
Attached image Screenshots (v2)
Both doorhanger and notification bar with the new labels.
Attachment #8436327 - Flags: ui-review?(neil) → ui-review+
Attachment #8436327 - Flags: review?(iann_bugzilla) → review+
Thanks for the reviews, push for comm-central please (once reopened).
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/1d790ac928a0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.30
You need to log in before you can comment on or make changes to this bug.