404014 - The Password Manager dialog's label changes when clearing the Search box

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P4)

Description

[(no longer active)]

Steps to reproduce:

1. Open Password Manager.  Watch the label: "Password Manager has saved login information for the following sites:".
2. Press Escape.  This will trigger the clear action for the search box (see [1] and [2] below) which changes the label to: "The following passwords have been saved by the Password Manager:"

[1] <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/passwordmgr/content/passwordManager.xul&rev=1.15&mark=72-74#68>
[2] <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/passwordmgr/content/passwordManager.js&rev=1.18&mark=310-313#310>

The same discrepancy happens if you enter something in the search box before step 2 above.

The solution would be to change the string at <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd&rev=1.6&mark=46#46> to match <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties&rev=1.12&mark=59#59>.

Note that according to bug 327048 comment 27 (review from mconnor) the second form of the string used in passwordmgr.properties (starting with "The following passwords") should be used.

Comment 1

[(no longer active)]

Attachment: Patch to fix the problem

This patch simply makes the change outlined in comment 0.

Comment 2

[Mike Connor [:mconnor]]

Comment on attachment 292028 [details] [diff] [review]
Patch to fix the problem

The right way to fix this is to set the label in SignonsStartup (as we do with the show passwords button).  This avoids the need to keep two strings in sync between passwordmgr.properties and passwordManager.dtd.

Comment 3

[(no longer active)]

Attachment: Patch to fix the problem (v2)

(In reply to comment #2)
> (From update of attachment 292028 [details] [diff] [review])
> The right way to fix this is to set the label in SignonsStartup (as we do with
> the show passwords button).  This avoids the need to keep two strings in sync
> between passwordmgr.properties and passwordManager.dtd.

Done.

I didn't remove |spiel.signonsstored.label| from passwordManager.dtd because it's used by passwordManagerExceptions.xul.

Comment 4

[(no longer active)]

Comment on attachment 292107 [details] [diff] [review]
Patch to fix the problem (v2)

Asking mano to review...

Comment 5

[(no longer active)]

mano: will you be able to review this?

Comment 6

[(no longer active)]

Comment on attachment 292107 [details] [diff] [review]
Patch to fix the problem (v2)

Switching review to dolske, since it's been in mano's queue for a very long time...

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 292107 [details] [diff] [review]
Patch to fix the problem (v2)

>Index: toolkit/components/passwordmgr/content/passwordManager.xul
>===================================================================
...
>+    <description control="signonsTree" id="signonsIntro"></description>

Nit: This can just be <description .../>

Technically still need a review/rubberstamp from a module peer, so over to gavin.

Comment 8

[(no longer active)]

(In reply to comment #7)
> (From update of attachment 292107 [details] [diff] [review])
> >Index: toolkit/components/passwordmgr/content/passwordManager.xul
> >===================================================================
> ...
> >+    <description control="signonsTree" id="signonsIntro"></description>
> 
> Nit: This can just be <description .../>

I'll make that change before asking approval.  Let's hear from Gavin on this.

> Technically still need a review/rubberstamp from a module peer, so over to
> gavin.

Hmm, your name was listed on <http://www.mozilla.org/projects/toolkit/review.html> as a reviewer for password manager...  Anyway, let's have Gavin's take on this as well.

Comment 9

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

Comment on attachment 292107 [details] [diff] [review]
Patch to fix the problem (v2)

>Index: toolkit/components/passwordmgr/content/passwordManager.js

> function SignonsStartup() {

>+  document.getElementById("signonsIntro").appendChild(document.createTextNode(kSignonBundle.getString("passwordsAll")));

Why create a child textnode? Just set .value like the code in SignonClearFilter()/_filterPasswords() does.

(In reply to comment #3)
> I didn't remove |spiel.signonsstored.label| from passwordManager.dtd because
> it's used by passwordManagerExceptions.xul.

That's &spiel.signons_not_stored.label; :) But removing this string will have l10n impact at this point, so it's best to just leave it anyways. Please do file a bug on removing it after we ship, though.

Comment 10

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

(In reply to comment #8)
> > Nit: This can just be <description .../>
> 
> I'll make that change before asking approval.  Let's hear from Gavin on this.

I agree with the nit, fwiw! :)

Comment 11

[(no longer active)]

(In reply to comment #9)
> (From update of attachment 292107 [details] [diff] [review])
> >Index: toolkit/components/passwordmgr/content/passwordManager.js
> 
> > function SignonsStartup() {
> 
> >+  document.getElementById("signonsIntro").appendChild(document.createTextNode(kSignonBundle.getString("passwordsAll")));
> 
> Why create a child textnode? Just set .value like the code in
> SignonClearFilter()/_filterPasswords() does.

Hmm, I didn't catch those places in the initial patch, but I guess those two should be changed to manipulate child nodes as well, because if the text is set in description's child node, it may wrap as necessary, right?

> (In reply to comment #3)
> > I didn't remove |spiel.signonsstored.label| from passwordManager.dtd because
> > it's used by passwordManagerExceptions.xul.
> 
> That's &spiel.signons_not_stored.label; :) But removing this string will have
> l10n impact at this point, so it's best to just leave it anyways. Please do
> file a bug on removing it after we ship, though.

Yeah, my mistake :-/

I'll file the bug once this one lands.

Comment 12

[Justin Dolske [:Dolske]]

(In reply to comment #8)
> Hmm, your name was listed on
> <http://www.mozilla.org/projects/toolkit/review.html> as a reviewer for
> password manager...

Oh, hmm, so it is. No one told me! :)

Comment 13

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

(In reply to comment #11)
> Hmm, I didn't catch those places in the initial patch, but I guess those two
> should be changed to manipulate child nodes as well, because if the text is set
> in description's child node, it may wrap as necessary, right?

Does that solve any actual problems? Creating a child text node is rather inefficient so I'd rather just stick with setting .value. If the wrapping is really an issue there are other ways to deal with it and we can do that in another bug.

Comment 14

[(no longer active)]

Attachment: Patch (v2.1)

(In reply to comment #13)
> Does that solve any actual problems? Creating a child text node is rather
> inefficient so I'd rather just stick with setting .value. If the wrapping is
> really an issue there are other ways to deal with it and we can do that in
> another bug.

OK, here's the new patch.

Comment 15

[Damon Sicore (:damons)]

Comment on attachment 305317 [details] [diff] [review]
Patch (v2.1)

a1.9+=damons

Comment 16

[Reed Loden [:reed]]

Checking in toolkit/components/passwordmgr/content/passwordManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/content/passwordManager.js,v  <--  passwordManager.js
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/components/passwordmgr/content/passwordManager.xul;
/cvsroot/mozilla/toolkit/components/passwordmgr/content/passwordManager.xul,v  <--  passwordManager.xul
new revision: 1.16; previous revision: 1.15
done