Closed Bug 337475 Opened 15 years ago Closed 14 years ago

'Remove All' button in password dangerous!

Categories

(SeaMonkey :: Passwords & Permissions, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: darrin, Assigned: mozilla)

References

Details

(Keywords: fixed-seamonkey1.1a)

Attachments

(2 files, 3 obsolete files)

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.
See bug 266945 for the Firefox equivalent of this.
Assignee: dveditz → nobody
Status: UNCONFIRMED → NEW
Depends on: 266945
Ever confirmed: true
Flags: blocking-seamonkey1.1a?
Flags: blocking-seamonkey1.1a? → blocking-seamonkey1.1a+
OS: Windows XP → All
Hardware: PC → All
Attached patch Verbatim copy of Firefox patch (obsolete) — Splinter Review
The patch for Firefox works for SeaMonkey, too, if applied to the respective Wallet files.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #232192 - Flags: superreview?(neil)
Attachment #232192 - Flags: review?
Attachment #232192 - Flags: review? → review?(cst)
Attachment #232192 - Flags: superreview?(neil) → superreview+
I can't review this until my builds stop crashing on startup.
Comment on attachment 232192 [details] [diff] [review]
Verbatim copy of Firefox patch

KaiRo, perhaps?
Attachment #232192 - Flags: review?(cst) → review?(kairo)
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.
Attachment #232192 - Flags: review?(kairo) → review+
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...
Attachment #232358 - Flags: superreview+
Attachment #232358 - Flags: review?(cst)
Comment on attachment 232192 [details] [diff] [review]
Verbatim copy of Firefox patch

I just spotted something.
You should make the "No" button the default.
Attachment #232192 - Flags: review+ → review?(kairo)
Attached patch With title and "No" as default (obsolete) — Splinter Review
Yes, no should be default. This patch has that and the custom title. Let's start again with the reviews from the beginning.
Attachment #232192 - Attachment is obsolete: true
Attachment #232358 - Attachment is obsolete: true
Attachment #232359 - Flags: superreview?(neil)
Attachment #232359 - Flags: review?(kairo)
Attachment #232358 - Flags: review?(cst)
Attachment #232192 - Flags: review?(kairo)
Comment on attachment 232192 [details] [diff] [review]
Verbatim copy of Firefox patch

Arrrgh, stupid lack of patch edit mid-airing :-(
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...)
Attachment #232359 - Flags: superreview?(neil) → superreview+
Attached patch v4Splinter Review
Well, we are getting closer, here is the new patch. Carrying over sr.
Attachment #232359 - Attachment is obsolete: true
Attachment #232553 - Flags: superreview+
Attachment #232553 - Flags: review?(kairo)
Attachment #232359 - Flags: review?(kairo)
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 :)
Attachment #232553 - Flags: review?(kairo) → review+
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).
(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.
"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...
"the the text" -> "to the text" ;-)
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 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.
Attachment #232967 - Flags: approval-seamonkey1.1a+
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.
landed on the branch too
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk
(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 ;->)
You need to log in before you can comment on or make changes to this bug.