88565 - Need UI fixup in "Set Master Password" dialog

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Description

[Henrik Gemal]

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

Comment 1

[Sean Cotter]

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):"

Comment 2

[(Don't use, old account) - Kai Engert]

-> Kai

Comment 3

[(Don't use, old account) - Kai Engert]

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.

Comment 4

[(Don't use, old account) - Kai Engert]

Attachment: Screenshot as described in comment / New dialog layout

Comment 5

[(Don't use, old account) - Kai Engert]

Attachment: Patch implementing the changes described in my comment

Comment 6

[(Don't use, old account) - Kai Engert]

David, please review.

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

Comment 7

[Sean Cotter]

"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.)

Comment 8

[(Don't use, old account) - Kai Engert]

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.

Comment 9

[(Don't use, old account) - Kai Engert]

Attachment: New patch / consistent usage of term Master Password / please review

Comment 10

[David P. Drinan]

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.

Comment 11

[(Don't use, old account) - Kai Engert]

Attachment: Slightly modifed patch as requested by ddrinan

Comment 12

[David P. Drinan]

r=ddrinan.

Comment 13

[Sean Cotter]

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?

Comment 14

[(Don't use, old account) - Kai Engert]

Attachment: Additional patch / fixes "set p12 password" dialog, too.

Comment 15

[(Don't use, old account) - Kai Engert]

Attachment: Screenshot of new layout of "set p12 password" dialog.

Comment 16

[(Don't use, old account) - Kai Engert]

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).

Comment 17

[Christopher Blizzard (:blizzard)]

sr=blizzard on both patches

Comment 18

[Sean Cotter]

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?

Comment 19

[(Don't use, old account) - Kai Engert]

Attachment: Changing "portable security password" to "certificate backup password" as requested

Comment 20

[(Don't use, old account) - Kai Engert]

I don't think we need an additional bug.

Sean, Can you please review?

Comment 21

[Sean Cotter]

Wording change in latest patch for "certificate backup password" looks fine.

Comment 22

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 23

[Stephane Saux]

Moving all P3 and P4 bugs targetted to 2.1 to future.

Comment 24

[Henrik Gemal]

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

[John Unruh]

Mass assigning QA to ckritzer.

Comment 26

[Javier Delgadillo]

Checked in patch for Kai.

Comment 27

[(Don't use, old account) - Kai Engert]

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.

Comment 28

[(Don't use, old account) - Kai Engert]

Attachment: Parts of above patches not yet checked in

Comment 29

[Javier Delgadillo]

Checked in latest patch as well.

Comment 30

[Stephane Saux]

Change the target of bugs with state 'RESOLVED' and target 'Future' to target
'2.1' since they were fixed for the 2.1 release.

Comment 31

[John Unruh]

Verified.