Need UI fixup in "Set Master Password" dialog

VERIFIED FIXED in psm2.1

Status

Core Graveyard
Security: UI
P3
trivial
VERIFIED FIXED
17 years ago
2 years ago

People

(Reporter: Henrik Gemal, Assigned: Kai Engert)

Tracking

1.0 Branch
psm2.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

(Reporter)

Description

17 years ago
The "Set Master Password" dialog has some problems:
- Title should be "Change Master Password" since thats what the button is 
called.
- the text "Security Device:Software Security Device" is nothing but weird.
- Missing ":" after each line. "Old password" should be "Old password:"
- Bar for password quality should be aligned under the text. Not under and 
indended.
- Dialog seem to be to big. A lot of space under the Ok buttons and a some 
space to the right in the dialog.
- Quality meter is al kind of weird. It seems impossible to get it to 100%
- When entering wrong "old password" you get a alert saying "bad password". I 
have no idea what password the alert is taking about. The new or the old 
password.
- When entering wrong "old password" you get a text saying "bad password". When 
I press Ok I should be returned to the "Set master password" dialog and not 
just back to the prefs.

build 20010629 on win2k

Updated

17 years ago
Priority: -- → P3
Target Milestone: --- → 2.1
Version: 1.01 → 1.5

Comment 1

17 years ago
The title issue is tricky. This same dialog comes up whether you're changing an
existing master password or setting a new one. Also, you could be changing a
password on one token, but setting a new one for another token, all from the
same dialog. Not sure what the best solution is for this, if any.

"Security Device: Software Security Device is correct, though I agree it's
confusing when only one token is available. 

If more than one token is available (for example, if you have a smart card
reader attached to your computer), this becomes a pup-up menu allowing you to
choose which token (i.e., security device) you want to set the password for.
"Software Security Device" is the name of the default token for the browser.

This is mentioned in help for this dialog, although the help text could be
improved in several respects (I'm working on it). At least the help button works
  now.

Other things mentioned are definitely worth fixing. The cryptic message if you
type the wrong old password is especially irksome. Also, you don't get any
message--just a disable OK button--if the second new password is different from
the first. I think, but am not certain, you should be able to click OK and then
get a message telling you what you did wrong; clicking OK in that message should
put you back in the change password dialog. 

One nit: I'd like to see "Enter a new password:" and "Enter the password again:"
changed to "New password:" and "New password (again):"

(Assignee)

Comment 2

17 years ago
-> Kai
Assignee: ssaux → kai.engert
(Assignee)

Comment 3

17 years ago
Regarding the title, depending on who started the dialog:
I agree with the reporter about the title, in my opinion it should always be
"change", never "set", even if there isn't a password yet, as the term "change"
is acceptable in that situation (in my opinion), too.
In addition, this dialog is really executed from different places, i.e. from
device_manager.js (which doesn't seem to change the title). Therefore I suggest
to remove the word "master" from the title.
I modified it and the title is now simply "Change password".

Regarding the labels of passwords:
Changed as suggested.
In addition, I think, it shouldn't display "old password", I changed it to
"current password".

Regarding alignment of the quality meter bar:
Aligning text labels, edit fields and labels seems to be difficult, as they all
start at a different pixel offset (from the left).
I think the best thing is to use a boxed layout, which gives the eye lines to
pay attention to and forget the different offsets within the boxes.
Please see the attached screenshot for my suggested layout.
    
Regarding size of dialog / too big:
I removed the with coded into XUL, and let the layout engine do the magic. Looks
good for me on Linux.
    
You ask: how can quality reach 100%?
Try the password:
  a,1:-45
This gives me a full bar.

We now display more descriptive texts in the mentioned error conditions.

Pressing ok without being able to change the password does no longer close it.

When changing the code, I noticed the return value is always set to 1 (file
password.js, function setPassword). This seems incorrect to me when I look at
the code in nsNSSDialogs::SetPassword, which relies on the return value.
I therefore changed the code to set a return code of 1 only when we were able to
change the password. And the current code never inits the "return value" to
zero. I added this default value in onLoad.

(Assignee)

Comment 4

17 years ago
Created attachment 41208 [details]
Screenshot as described in comment / New dialog layout
(Assignee)

Comment 5

17 years ago
Created attachment 41209 [details] [diff] [review]
Patch implementing the changes described in my comment
(Assignee)

Comment 6

17 years ago
David, please review.

(To apply the patch, go to security/manager/pki/resources and use "patch -p0".)

Comment 7

17 years ago
"Master password" is the term used throughout the interface for the password 
that protects a token, as distinct from the certificate backup password, 
passwords entered on web sites, etc. Changing the term here without changing it 
anywhere else (such as the prefs panel or menu from which you open the 
dialog) would be bad. This is especially important for the Password 
Manager, whose master key is stored on default token ("Software Security 
Device").

Unless we want to reopen the whole master password issue, please leave the 
master in "Change Master Password". (And of course even this text change 
should be made to PSM 2.1 only; too late to make UI text changes for 2.0.)
(Assignee)

Comment 8

17 years ago
Sean, thanks for your comment. I didn't know that Master Password is the term
for all security devices (I assumed it's only used for the internal Software
device). You arguments convice me, and we indeed should leave the term Master
Password.

I'm attaching a new patch.
(Assignee)

Comment 9

17 years ago
Created attachment 41259 [details] [diff] [review]
New patch / consistent usage of term Master Password / please review
(Assignee)

Updated

17 years ago
Keywords: patch, review

Comment 10

17 years ago
The term "Master Password" should always be displayed with capital M and capital
P. There are a few instances in the patch, where "master password" is used. Fix
that and r=ddrinan.
(Assignee)

Comment 11

17 years ago
Created attachment 41325 [details] [diff] [review]
Slightly modifed patch as requested by ddrinan

Comment 12

17 years ago
r=ddrinan.

Comment 13

17 years ago
Change target to 2.0? Also, do we need to inform localization about such UI
changes after "UI freeze," or do they do another sweep at some point?
(Assignee)

Comment 14

17 years ago
Created attachment 41344 [details] [diff] [review]
Additional patch / fixes "set p12 password" dialog, too.
(Assignee)

Comment 15

17 years ago
Created attachment 41346 [details]
Screenshot of new layout of "set p12 password" dialog.
(Assignee)

Comment 16

17 years ago
Sorry for fiddling around so much. But the dialog used to set the password when
backing up a certificate to a p12 file has the same problems (size and quality
meter bar).

So, if we really wanted to re-target this bug to 2.0, we shouldn't forget to fix
this other dialog, too.

This time, only the dialog layout changed, using the same strategy as before
(see screenshot).
Status: NEW → ASSIGNED
sr=blizzard on both patches

Comment 18

17 years ago
The attached Certificate Backup screen shot ("set p12 password") includes the
following text:

Important: If you forget your portable security password, you will not be able
to restore this backup later. Please record it in a safe location.

The phrase "portable security password" in the above should be "certificate
backup password" to be consistent with the rest of the dialog. This will require
an additional patch--should it be a separate bug?
(Assignee)

Comment 19

17 years ago
Created attachment 42796 [details] [diff] [review]
Changing "portable security password" to "certificate backup password" as requested
(Assignee)

Comment 20

17 years ago
I don't think we need an additional bug.

Sean, Can you please review?

Comment 21

17 years ago
Wording change in latest patch for "certificate backup password" looks fine.
sr=blizzard

Comment 23

17 years ago
Moving all P3 and P4 bugs targetted to 2.1 to future.
Target Milestone: 2.1 → Future
(Reporter)

Comment 24

17 years ago
How can this be "future" when we have a sr and just missing the checkin? Give 
Kai a brake and help check it in....

Comment 25

17 years ago
Mass assigning QA to ckritzer.
QA Contact: junruh → ckritzer

Comment 26

17 years ago
Checked in patch for Kai.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 27

17 years ago
Ok, I confess this bug was confusing. Javi, you checked in only the latest
patch. There are two more patches that should be checked in.

However, meanwhile the patches do no longer work because of conflicts caused by
other checkins. Nothing problematic, though.

I've created a new patch, which is the same as contained in attachments 41344
and 41325. IMHO, no additional review is required.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

17 years ago
Created attachment 44990 [details] [diff] [review]
Parts of above patches not yet checked in

Comment 29

17 years ago
Checked in latest patch as well.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 30

17 years ago
Change the target of bugs with state 'RESOLVED' and target 'Future' to target
'2.1' since they were fixed for the 2.1 release.
Target Milestone: Future → 2.1

Comment 31

17 years ago
Verified.
Status: RESOLVED → VERIFIED

Updated

13 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

10 years ago
Version: psm1.5 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.