Closed Bug 408797 Opened 14 years ago Closed 14 years ago

Implement Change Password as a notification bar

Categories

(Toolkit :: Password Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: late-l10n)

Attachments

(2 files, 4 obsolete files)

Attached patch Patch (v1) (obsolete) — Splinter Review
This bug covers a subset of bug 394611, which entails implementing the change password confirmation dialog as a notification bar.
Attachment #293643 - Flags: review?(dolske)
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.
Will this lead to a lot of people losing their new passwords if they don't notice the infobar?
Keywords: uiwanted
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...
Attachment #293643 - Flags: ui-review?(beltzner)
(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...
Duplicate of this bug: 409721
Attached patch Patch (v1) (strings only) (obsolete) — Splinter Review
String changes to land before the l10n freeze...
Attachment #298778 - Flags: ui-review?(beltzner)
Attachment #298778 - Flags: review?(beltzner)
Whiteboard: [has patch] [needs review dolske][needs ui-review beltzner]
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.
Attachment #298778 - Flags: ui-review?(beltzner)
Attachment #298778 - Flags: ui-review+
Attachment #298778 - Flags: review?(beltzner)
Attachment #298778 - Flags: review-
Attachment #293643 - Flags: ui-review?(beltzner) → ui-review+
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...
Attachment #298778 - Attachment is obsolete: true
Attachment #299386 - Flags: ui-review+
Attachment #299386 - Flags: review?(beltzner)
Attachment #299386 - Flags: approval1.9?
Whiteboard: [has patch] [needs review dolske][needs ui-review beltzner] → [has patch] [needs review dolske][has ui-review+] [string changes need to land before l10n freeze]
Comment on attachment 299386 [details] [diff] [review]
Patch (v1.1) (strings only) (checked in)

r+a=beltzner for 1.9
Attachment #299386 - Flags: review?(beltzner)
Attachment #299386 - Flags: review+
Attachment #299386 - Flags: approval1.9?
Attachment #299386 - Flags: approval1.9+
Attachment #299386 - Attachment description: Patch (v1.1) (strings only) → Patch (v1.1) (strings only) (for check-in before the l10n freeze)
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.
Attachment #293643 - Flags: review?(gavin.sharp)
Attachment #293643 - Flags: review?(dolske)
Attachment #293643 - Flags: review+
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
Keywords: checkin-needed
Whiteboard: [has patch] [needs review dolske][has ui-review+] [string changes need to land before l10n freeze] → [has patch][has review dolske][needs review gavin][has ui-review+]
Attachment #299386 - Attachment description: Patch (v1.1) (strings only) (for check-in before the l10n freeze) → Patch (v1.1) (strings only) (checked in)
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.
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
(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 :-/
Attached patch Patch (v2) (obsolete) — Splinter Review
(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.
Attachment #293643 - Attachment is obsolete: true
Attachment #299950 - Flags: ui-review+
Attachment #299950 - Flags: review?(gavin.sharp)
Attachment #293643 - Flags: review?(gavin.sharp)
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 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>
Attachment #299950 - Flags: review?(gavin.sharp)
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>
Attachment #299950 - Flags: ui-review+
Attachment #299950 - Flags: review?(gavin.sharp)
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. :)
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.
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
gavin: ping...
This looks fine; needs to be updated to deal with the changes from bug 413963, though.
Attached patch Patch (v2.1) (obsolete) — Splinter Review
Updated patch which applies cleanly on trunk.  Would you mind taking a look at this one, Gavin?  Thanks!
Attachment #299950 - Attachment is obsolete: true
Attachment #305134 - Flags: review?(gavin.sharp)
Attachment #299950 - Flags: review?(gavin.sharp)
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!
Attachment #305134 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][has review dolske][needs review gavin][has ui-review+] → [needs app
Whiteboard: [needs app → [needs approval]
Attached patch Patch (v2.2)Splinter Review
(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.
Attachment #305134 - Attachment is obsolete: true
Attachment #305137 - Flags: approval1.9?
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?
Keywords: late-l10n
(fwiw, I think it's worth taking, as the context should be similar and the notification bar already wraps)
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
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 on attachment 305137 [details] [diff] [review]
Patch (v2.2)

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

Localizers: these strings should already be translated.
Attachment #305137 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Whiteboard: [needs approval]
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 421058
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.