314557 - "Remove All" button in Privacy: Edit Exceptions sheet has no warning (cookies UI)

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The "Remove All" button in the Privacy pane's "Edit Exceptions List" sheet performs its action without any warning dialogue.  All the rest of our destructive removal UI (including the sheet called by the adjacent "Show Cookies" button in that pane) shows a warning first.

Given the roughness of that UI (bug 314479, bug 314478, bug 303577), this behavior can cause dataloss.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This, of all the cookies UI bugs, most needs to block 1.0 :-)

Comment 2

[Bill Kocik]

Attachment: Patch for PrivacyPane.mm

This patches PrivacyPane.mm to call NSRunCriticalAlertPanel when "Remove All" is selected. It specifically does not address the issue that more than just the exceptions are removed (bug 314478). Also required is the separately attached Localizable.strings.zip.

Comment 3

[Bill Kocik]

Attachment: Localizable.strings file

Replaces mozilla/camino/resources/localized/English.lproj/Localizable.strings

Comment 4

[Wevah (Nate Weaver)]

Comment on attachment 201514 [details] [diff] [review]
Patch for PrivacyPane.mm

Patch looks good. r=me

Comment 5

[Wevah (Nate Weaver)]

Comment on attachment 201515 [details]
Localizable.strings file

Strings changes look good too. r=me, again.

There's no need to zip a strings file though, you can just attach it directly and specify "text/plain; charset=UTF-16" in the future. :)

Comment 6

[Simon Fraser [no longer active]]

I'd like to see the "RemoveAllCookiePermissionsWarning" string be a little more detailed about the consequences (e.g. web site forgetting your identity, losing shopping carts etc).

Comment 7

[Bill Kocik]

(In reply to comment #6)
> I'd like to see the "RemoveAllCookiePermissionsWarning" string be a little more
> detailed about the consequences

That particular warning is not part of this bug or this patch. If you meant you'd like to see some more exposition on what happens if you remove all site exceptions, I'll be glad to add that, but I'll need some suggestions. 

As for the warning when removing all cookies, maybe that's best handled as a separate bug report?

Comment 8

[Simon Fraser [no longer active]]

(In reply to comment #7)
> (In reply to comment #6)
> > I'd like to see the "RemoveAllCookiePermissionsWarning" string be a little more
> > detailed about the consequences
> 
> That particular warning is not part of this bug or this patch.

Same applies to "RemoveAllCookiePermissionsWarning". Still need to say more than "You cannot undo this action". You need to explain what the consequences are.

Comment 9

[Bill Kocik]

(In reply to comment #8)
> Same applies to "RemoveAllCookiePermissionsWarning". Still need to say more
> than "You cannot undo this action". You need to explain what the consequences
> are.

Okay. How about this: "You will be prompted to accept cookies for any web site that attempts to set them. You cannot undo this action."?

I'm also of the opinion that we might consider "This action cannot be undone" in place of "You cannot undo this action." Isn't that the standard way of saying that? But at the same time, I don't want to become inconsistent with the RemoveAllCookiesWarning, so maybe we shouldn't change that part right now. Or maybe we should change them both, or even define a whole new string: "CannotUndoWarning" = "This action cannot be undone." I'm wide open for suggestions. 

Comment 10

[Simon Fraser [no longer active]]

Someone remind me: does the exceptions list only apply if you have "ask before accepting each cookie" turned on?

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #10)
> Someone remind me: does the exceptions list only apply if you have "ask before
> accepting each cookie" turned on?

No--if I understand what you're asking.  I deleted my trinity.neooffice.org cookies, changed the Exceptions List entry for trinity to "Deny", unchecked the "Ask before accepting cookies", went to trinity and (attempted to) login.  No trinity cookies when I went to look.

But you can only add items to the Exceptions List (via the Camino GUI) with the "Ask" pref checked.

Comment 12

[Simon Fraser [no longer active]]

(In reply to comment #11)
> (In reply to comment #10)
> > Someone remind me: does the exceptions list only apply if you have "ask before
> > accepting each cookie" turned on?
> 
> No--if I understand what you're asking.  I deleted my trinity.neooffice.org
> cookies, changed the Exceptions List entry for trinity to "Deny", unchecked the
> "Ask before accepting cookies", went to trinity and (attempted to) login.  No
> trinity cookies when I went to look.

OK

> But you can only add items to the Exceptions List (via the Camino GUI) with the
> "Ask" pref checked.

It would be nice to allow the user to add sites there by hand.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #12)
> > But you can only add items to the Exceptions List (via the Camino GUI) with
> > "the Ask" pref checked.
> 
> It would be nice to allow the user to add sites there by hand.

Bug 269084.  

(You can do manual additions already for the Popup Blocker Exceptions sheet--but that only has one state, whitelist, so the UI is easier.)

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #9)

> I'm also of the opinion that we might consider "This action cannot be undone"
> in place of "You cannot undo this action." Isn't that the standard way of
> saying that? 

The existing UI apparently uses both.  Reset Camino is "cannot be undone", Empty Cache is "cannot undo", Clear History doesn't have either one in its message, and in the Prefs, neither Clear Visited Pages nor Clear Cache have an alert at all.  (Clear Visited Pages should--and it should use the same text as Clear History; I'll file a bug on that.)

(The Remove All button in the "Show Cookies..." sheet needs the elipsis, as does this Remove All, once this bug gets wired up.)

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Updated Privacy.nib with elipsis added to Remove All buttons

Here's the nib with the elipsis added to the Remove All buttons.

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: proposed Localizable.strings

The Finder/trash says "You cannot undo this action", so I think we should, too.  I've fixed the "Reset Camino" message to use that instead of "cannot be undone" and added that sentence to the "Clear History" string.

Here are the proposals for "RemoveAllCookiesWarning" and "RemoveAllCookiePermissionsWarning"; they're not perfect yet, but starting points (let's build off this localizable.strings file for changes, though, so we keep the cache and history changes all rolled up in one strings change/file):

"RemoveAllCookiesWarning" = "Removing cookies will cause web sites to forget your identity, any online shopping carts, and other personalized site settings. You cannot undo this action.";

"RemoveAllCookiePermissionsWarning" = "Camino will no longer remember which sites you wish to accept cookies from and which sites do not want cookies from.  If you have \"Ask before accepting each cookie\" selected, you will be prompted again to accept cookies for any web site that attempts to set them. You cannot undo this action.";

The second one is more awakward (and, until bug 314478 is fixed, incorrect, too, because it currently blanks hostperm.1 instead of just the cookies entries).

Comment 17

[Chris Lawson (gone)]

(In reply to comment #16)
> "RemoveAllCookiesWarning" = "Removing cookies will cause web sites to forget
> your identity, any online shopping carts, and other personalized site settings.
> You cannot undo this action.";

"May" rather than "will". Not all these are cookie-based, obviously.

> "RemoveAllCookiePermissionsWarning" = "Camino will no longer remember which
> sites you wish to accept cookies from and which sites do not want cookies from.
>  If you have \"Ask before accepting each cookie\" selected, you will be
> prompted again to accept cookies for any web site that attempts to set them.
> You cannot undo this action.";

How about (for the first sentence):

"Camino will no longer remember which sites you do or do not want to accept cookies from."

I'm not exactly thrilled with that, either, but it's shorter and a little less awkward.

cl

Comment 18

[Simon Fraser [no longer active]]

> "Camino will no longer remember which sites you do or do not want to accept
> cookies from."

Or perhaps
"Camino will no longer remember which sites you chose to accept or reject cookies for"?

Curly quotes in the Localizable.strings, please.

Comment 19

[Chris Lawson (gone)]

--> Smokey.

cl

Comment 20

[Simon Fraser [no longer active]]

Fixed.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Updated Localizable.strings

Simon, I didn't see a Localizable.strings checkin associated with any of these bugs in bonsai (only the .strings files that live within the prefPanes).  Here's the updated file with all changes mentioned in this bug and bug 315886.

Comment 22

[Simon Fraser [no longer active]]

Smokey: each prefs bundle has its own Localizable.strings, which is where the strings should be for that pref bundle. We shouldn't use NSLocalizedString() in the pref pane code.