337475 - 'Remove All' button in password dangerous!

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, enhancement)

Description

[Darrin Maunders]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060130 SeaMonkey/1.0 Mnenhy/0.7.3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060130 SeaMonkey/1.0 Mnenhy/0.7.3.0

Not so much a bug as problem in the way password manage handles password removal. I wanted to remove just one password from the list but hit 'Remove All' by accident. Instead of verifying with a popup message asking if I was really sure I wanted to 'Remove all passwords from the list' it just went ahead and removed all of them!

To me verification from the user for such a dangerous button is an an essential element to have.

Luckily I had a backup of the password files. But this should not really be needed as long as everything is laid out in a straighforward manner. 

Reproducible: Always

Steps to Reproduce:
1. Click on password manager
2. Click on 'Remove All'



Expected Results:  
A popup message verifying if the user really does want to remove all passwords from the list.

Popped up a message verifying action.

Comment 1

[Simon Bünzli]

See bug 266945 for the Firefox equivalent of this.

Comment 2

[Peter Weilbacher]

Attachment: Verbatim copy of Firefox patch

The patch for Firefox works for SeaMonkey, too, if applied to the respective Wallet files.

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I can't review this until my builds stop crashing on startup.

Comment 4

[Peter Weilbacher]

Comment on attachment 232192 [details] [diff] [review]
Verbatim copy of Firefox patch

KaiRo, perhaps?

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 232192 [details] [diff] [review]
Verbatim copy of Firefox patch

Neil helped me get my builds to start.

r=me, though I think using a custom title for the alert would be better.

Comment 6

[Peter Weilbacher]

Attachment: Same patch including dialog title

Hmm, something like "Confirm remove all passwords"? I think that would at least follow the somewhat unimaginative titles that other such dialogs get. I updated the patch to include it.

Carrying over neil's sr, but I would like to know from you if you are even more happy with this title...

Comment 7

[[e-mail]]

Comment on attachment 232192 [details] [diff] [review]
Verbatim copy of Firefox patch

I just spotted something.
You should make the "No" button the default.

Comment 8

[Peter Weilbacher]

Attachment: With title and "No" as default

Yes, no should be default. This patch has that and the custom title. Let's start again with the reviews from the beginning.

Comment 9

[[e-mail]]

Comment on attachment 232192 [details] [diff] [review]
Verbatim copy of Firefox patch

Arrrgh, stupid lack of patch edit mid-airing :-(

Comment 10

[[e-mail]]

Comment on attachment 232359 [details] [diff] [review]
With title and "No" as default

>+                         prompter.BUTTON_TITLE_YES * prompter.BUTTON_POS_0 +
>+                         prompter.BUTTON_TITLE_NO * prompter.BUTTON_POS_1 +
prompter.STD_YES_NO_BUTTONS (sorry, I know, I missed that last time...)

Comment 11

[Peter Weilbacher]

Attachment: v4

Well, we are getting closer, here is the new patch. Carrying over sr.

Comment 12

[Robert Kaiser]

Comment on attachment 232553 [details] [diff] [review]
v4

>+removeAllPasswordsTitle=Confirm remove all passwords

Shouldn't this be "Confirm removing"?

r=me for the locale parts, and Neil has already looked at the rest :)

Comment 13

[Peter Weilbacher]

Attachment: v5, with "removing" (for checkin)

OK, thanks for the reviews. Here is the version for checkin (that only differs by using "removing" instead of "remove"). 

I also need somebody else to check this in (I am only allowed to check in OS/2 stuff).

Comment 14

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #12)
> (From update of attachment 232553 [details] [diff] [review] [edit])
> >+removeAllPasswordsTitle=Confirm remove all passwords
> 
> Shouldn't this be "Confirm removing"?
> 
> r=me for the locale parts, and Neil has already looked at the rest :)
> 

I really don't like KaiRo's wording.  How about just "Remove all passwords" for the title or "Remove all passwords?" instead?  r=me with either of my suggestions.

Comment 15

[Peter Weilbacher]

"Confirm removing..." is at least consistent with tabs.closeWarningTitle (although that is capitalized). How about "Really remove all passwords?" instead? If it's too close the the text inside the dialog we can as well skip it...

Comment 16

[Peter Weilbacher]

"the the text" -> "to the text" ;-)

Comment 17

[Robert Kaiser]

Could we please get this in? We need this fixed until yesterday to make 1.1 Alpha, and we have a finished patch, we just need to correct that wording issue at checkin.

Peter, feel free to check this in as long as you correct the string like CTho requested.

Comment 18

[Robert Kaiser]

Comment on attachment 232967 [details] [diff] [review]
v5, with "removing" (for checkin)

a=me for 1.1 given it works on trunk and a title string is chosen that is OKed by CTho.

Comment 19

[Andrew Schultz]

Comment on attachment 232967 [details] [diff] [review]
v5, with "removing" (for checkin)

I landed this with "Remove all passwords" which was the consensus on IRC.

Comment 20

[Andrew Schultz]

landed on the branch too

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> See bug 266945 for the Firefox equivalent of this.

(I posted a patch there to port (XPFE) Wallet enhancements to (TK) Passwordmgr ;->)