408797 - Implement Change Password as a notification bar

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement)

Description

[(no longer active)]

Attachment: Patch (v1)

This bug covers a subset of bug 394611, which entails implementing the change password confirmation dialog as a notification bar.

Comment 1

[(no longer active)]

Some notes on the patch:

* The patch factors out the code to show a notification bar as _showLoginNotification().
* The patch includes the increased timeout from bug 408363.

I created this bug because I was not sure how to convert the dialog in promptToChangePasswordWithUsernames() to a notification bar, so I decided to fork bug 394611 just to focus on the password change confirmation dialog for now.

Comment 2

[Daniel Veditz [:dveditz]]

Will this lead to a lot of people losing their new passwords if they don't notice the infobar?

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 293643 [details] [diff] [review]
Patch (v1)

This looks fine from a quick glance, but I think I'll defer a complete review until after the UI is ok'd.

I can see the potential for someone accidently ignoring the bar, but it is fairly noticeable.

Also, even though the bar isn't modal, I wonder if both a Yes and No button should be there (currently the patch only adds Yes). Part of the reason for dropping the "Not Now" from the Save Password bar is that 3 choices is kind of awkward. But that's not a problem for "Yes / No". I don't feel strongly either way...

Comment 4

[(no longer active)]

(In reply to comment #3)
> Also, even though the bar isn't modal, I wonder if both a Yes and No button
> should be there (currently the patch only adds Yes). Part of the reason for
> dropping the "Not Now" from the Save Password bar is that 3 choices is kind of
> awkward. But that's not a problem for "Yes / No". I don't feel strongly either
> way...

Implementing a No button is simple enough, so I can add it to the patch if you think it's necessary, but its function would be the same to closing the bar itself (because we don't perform any special action on clicking No).  I guess the ui-review should decide on this issue though...

Comment 6

[(no longer active)]

Attachment: Patch (v1) (strings only)

String changes to land before the l10n freeze...

Comment 7

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

Comment on attachment 298778 [details] [diff] [review]
Patch (v1) (strings only)

> passwordChangeText = Would you like to have Password Manager change the stored password for %S?

nit: we should drop the explicit reference to password manager here, and instead do something like:

passwordChangeText = Would you like to change the stored password for %S?

> passwordChangeTextNoUser = Would you like to have Password Manager change the stored password for this login?

passwordChangeText = Would you like to change this the stored password for this login?

>+notifyBarChangeButtonText = Change
>+notifyBarChangeButtonAccessKey = C

That's all fine.

We will want a "Don't Change" option as well, though.

Comment 8

[(no longer active)]

Attachment: Patch (v1.1) (strings only) (checked in)

Addressed all of the issues in comment 7.  I moved over the ui-r+ flag here, and thought to ask beltzner for another review *and* approval in the same go because of the scarcity of time before the l10n freeze...

Comment 9

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

Comment on attachment 299386 [details] [diff] [review]
Patch (v1.1) (strings only) (checked in)

r+a=beltzner for 1.9

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 293643 [details] [diff] [review]
Patch (v1)

>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>+        // Bug 408363 bumps the timeout value from 10 to 20 seconds, in order
>+        // to make life easier for dial-up users.
>+        var now = Date.now() / 1000;
>+        newBar.ignoreLocationChangeTimeout = now + 20; // 20 seconds

That bug's not in yet, so just be sure to use the right value for whatever the final patch ends up being.

>+        this._showLoginNotification(aNotifyBox, notificationText,
>+             buttons, "password-save");

Let's reorder the args slightly:

    this._showLoginNotification(aNotifyBox, "password-save",
                                notificationText, buttons);

>+        var buttons = [
>+            // "Yes" button
>+            {
>+                label:     changeButtonText,
>+                accessKey: changeButtonAccessKey,
>+                popup:     null,
>+                callback: function(aNotificationBar, aButton) {
>+                    prompter.log("Updating password for user " + aOldLogin.username);
>+                    pwmgr.modifyLogin(aOldLogin, aNewLogin);
>+                }
>+            },
>+
>+            // "No" button not needed, as notification bar isn't modal.

Patch needs updated to add the "Don't Change" button and new string names.

Also, no need for |prompter.log|, since modifyLogin() will log that it's doing something anyway.

Comment 11

[Reed Loden [:reed]]

Checking in toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties,v  <--  passwordmgr.properties
new revision: 1.15; previous revision: 1.14
done

Comment 12

[Francesco Lodolo [:flod]]

passwordChangeTextNoUser = Would you like to change this the stored password for this login?

Is this string correct? Maybe I'm misunderstanding the structure of this sentence, but the first "this" (change this) seems a typo.

Comment 13

[Justin Dolske [:Dolske]]

Yeah, that seems obviously wrong. I checked in a fix to remove the "this".

Checking in toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties,v  <--  passwordmgr.properties
new revision: 1.16; previous revision: 1.15
done

Comment 14

[(no longer active)]

(In reply to comment #12)
> passwordChangeTextNoUser = Would you like to change this the stored password
> for this login?
> 
> Is this string correct? Maybe I'm misunderstanding the structure of this
> sentence, but the first "this" (change this) seems a typo.

Sorry I didn't catch this :-/

Comment 15

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #10)
> (From update of attachment 293643 [details] [diff] [review])
> >Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js
> 
> >+        // Bug 408363 bumps the timeout value from 10 to 20 seconds, in order
> >+        // to make life easier for dial-up users.
> >+        var now = Date.now() / 1000;
> >+        newBar.ignoreLocationChangeTimeout = now + 20; // 20 seconds
> 
> That bug's not in yet, so just be sure to use the right value for whatever the
> final patch ends up being.

I reverted this change.

> >+        this._showLoginNotification(aNotifyBox, notificationText,
> >+             buttons, "password-save");
> 
> Let's reorder the args slightly:
> 
>     this._showLoginNotification(aNotifyBox, "password-save",
>                                 notificationText, buttons);

Done.

> >+        var buttons = [
> >+            // "Yes" button
> >+            {
> >+                label:     changeButtonText,
> >+                accessKey: changeButtonAccessKey,
> >+                popup:     null,
> >+                callback: function(aNotificationBar, aButton) {
> >+                    prompter.log("Updating password for user " + aOldLogin.username);
> >+                    pwmgr.modifyLogin(aOldLogin, aNewLogin);
> >+                }
> >+            },
> >+
> >+            // "No" button not needed, as notification bar isn't modal.
> 
> Patch needs updated to add the "Don't Change" button and new string names.

Done.  The callback for the "Don't Change" button simply does nothing, and lets the notification bar get closed.

> Also, no need for |prompter.log|, since modifyLogin() will log that it's doing
> something anyway.

I removed that line.

Carrying over beltzner's ui-r+ and dolske's r+, asking gavin for a review.

Comment 16

[Justin Dolske [:Dolske]]

Please don't set the + flags yourself for previous patches, it's confusing (it harder to look at the progression of patches to see who actually reviewed what). The general assumption is that reviews carry forward.

Comment 17

[(no longer active)]

Comment on attachment 299950 [details] [diff] [review]
Patch (v2)

(In reply to comment #16)
> Please don't set the + flags yourself for previous patches, it's confusing (it
> harder to look at the progression of patches to see who actually reviewed
> what). The general assumption is that reviews carry forward.

Sorry for the confusion.  I thought I'm expected to do that: <http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree>

Comment 18

[(no longer active)]

Comment on attachment 299950 [details] [diff] [review]
Patch (v2)

(In reply to comment #16)
> Please don't set the + flags yourself for previous patches, it's confusing (it
> harder to look at the progression of patches to see who actually reviewed
> what). The general assumption is that reviews carry forward.

Sorry for the confusion.  I thought I'm expected to do that: <http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree>

Comment 19

[Justin Dolske [:Dolske]]

Huh. So, apparently this is done by some people in some areas, and not by other people in other areas. I hadn't seen it commonly done, guess I'm not watching the right bugs. :)

Comment 20

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

I don't like "carrying over" review flags, personally. It just makes it more confusing to see who actually granted the review, and really doesn't serve any purpose.

Comment 21

[(no longer active)]

gavin: ping...

Comment 22

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

This looks fine; needs to be updated to deal with the changes from bug 413963, though.

Comment 23

[(no longer active)]

Attachment: Patch (v2.1)

Updated patch which applies cleanly on trunk.  Would you mind taking a look at this one, Gavin?  Thanks!

Comment 24

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

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

>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>+        var prompter = this;

This is unused.

Looks good otherwise!

Comment 25

[(no longer active)]

Attachment: Patch (v2.2)

(In reply to comment #24)
> (From update of attachment 305134 [details] [diff] [review])
> >Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js
> 
> >+        var prompter = this;
> 
> This is unused.

Oops, fixed.  Asking for approval.

Comment 26

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

Axel: this would call strings that are already checked in and translated, but not used in the UI yet. As per our previous agreement, that would be late-l10n and to land it now would be after the beta 4 string freeze. Do you have an opinion if we should take this or wait?

Comment 27

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

(fwiw, I think it's worth taking, as the context should be similar and the notification bar already wraps)

Comment 28

[Justin Dolske [:Dolske]]

Actually, the two long strings in attachment 299386 [details] [diff] [review] would already appear in the existing change-password modal dialog. The only new strings added but not yet used are:

+notifyBarChangeButtonText = Change
+notifyBarChangeButtonAccessKey = C
+notifyBarDontChangeButtonText = Don't Change
+notifyBarDontChangeButtonAccessKey = D

Comment 29

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

I think Axel's intent when he asked for bugs to be flagged late-l10n when we check in patches that use already-landed strings was to ensure that localizers were aware that the strings were now being used, and that they could verify them in the UI. I don't think that desire to notify localizers should have any bearing on whether or not we take the patch.

I think we should take the patch, otherwise the whole exercise of landing strings early and having localizers translate them will have been for naught.

Comment 30

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

Comment on attachment 305137 [details] [diff] [review]
Patch (v2.2)

a=beltzner, please mark late-l10n when landing:

Localizers: these strings should already be translated.

Comment 31

[Reed Loden [:reed]]

Checking in toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js,v  <--  nsLoginManagerPrompter.js
new revision: 1.25; previous revision: 1.24
done