324063 - "Passwords never saved" (exceptions) is too hard to find

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[:Gavin Sharp [email: [e-mail]]]

There's no reason "passwords never saved" should be in a background tab of the "View Saved Passwords" window. This makes it hard to find. There should instead be a "Exceptions" button next to the "Remember passwords" checkbox, much like on the "Cookies" tab.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This splits the old "passwordManager.xul" into two - the "view passwords" part (which remains in passwordManager.xul) and the "never save passwords for these sites" part, which becomes passwordManagerExceptions.xul. Common code is factored out into passwordManagerCommon.js. A new "Exceptions" button is added next to the "Remember passwords" checkbox on the Privacy tab, to match the one on the "Cookies" tab. I wondered if maybe the button should be right aligned and wider to match the other buttons, but then thought it'd be best to stay consistent with "Cookies" and also keep it closely tied to the checkbox. I've tested the basic functionality (add/remove items, show passwords) but I'd like to test that the observer stuff is still working before asking for code review.

Other minor changes I just want to note:
1) The dynamic setting of the "close" button's label has been removed in favor of the "buttonlabelaccept" attribute (corresponding key in passwordmanager.properies was removed)
2) The TrimString function has been removed because it is unused
3) Seems as though the "preference" attributes on buttons aren't working as they should... I should look into/file a new bug on that

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: screenshot

Guess this is easier to review. See last comment for details.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Seeing those screenshots makes me think I could use a better title for the windows, like "Password Manager - Never Saved" for the exceptions one, and just  leave "Password Manager" for the other one?

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 216082 [details]
screenshot

Yeah, the windows should be titled "Don't Remember Passwords" and "Remember Passwords". I kinda feel like the exceptions button should be flush right, but this is consistent with cookies atm, so let's do it this way.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch v2 (UI review comments addressed)

With Beltzner's UI comments addressed. This is mostly moving code around. One thing I changed was the dynamic setting of the "close" button label from a string bundle, I changed it to use an entity on the dialog element itself instead.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: as checked in

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk.

Comment 8

[u60234]

Comment on attachment 222733 [details] [diff] [review]
as checked in

>Index: toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd
>===================================================================

>-<!ENTITY      windowtitle.label               "Password Manager">
>+<!ENTITY      rememberPasswords.title         "Remember Passwords">
>+<!ENTITY      dontRememberPasswords.title     "Don't Remember Passwords">
>+<!ENTITY      closebutton.label               "Close">
> <!ENTITY      tab.signonsstored.label         "Passwords Saved">
> <!ENTITY      tab.signonsnotstored.label      "Passwords Never Saved">

tab.signonsstored.label and tab.signonsnotstored.label could perhaps be removed also if they are no longer used?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: remove unused entities

Indeed, thanks for catching that.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 222802 [details] [diff] [review]
remove unused entities

Checked in on the trunk.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Checked in attachment 222733 [details] [diff] [review] and attachment 222802 [details] [diff] [review] on the 1.8 branch at 2006-05-27 14:04.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

VERIFIED, with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060822 BonEcho/2.0b2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060821 Minefield/3.0a1