588309 - Convert change password to a doorhanger panel

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

We should convert the change password notification from using a notification bar to using a doorhanger panel anchored to the site identity block.

More details about the general design direction for doorhanger panels can be found in the parent bug 588240, please use that bug for any discussion or concerns.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Nominating this for blocking since it is one of the few high profile doorhanger notifications.

Comment 2

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

Reasonable to slot this for beta6?

Comment 3

[:Margaret Leibovic]

Attachment: patch

I generalized _showChangeLoginNotification to take either a bar or a popup the same way I did in bug 567814.

This patch also needs the same icons that we need in bug 567814.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

[strings]: probably new strings for the direct question and action button

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 471324 [details] [diff] [review]
patch


>     promptToChangePassword : function (aOldLogin, aNewLogin) {
>+        var popupNote = this._getPopupNote();
>         var notifyBox = this._getNotifyBox();
> 
>-        if (notifyBox)
>-            this._showChangeLoginNotification(notifyBox, aOldLogin, aNewLogin.password);
>+        if (popupNote)
>+            this._showChangeLoginNotification(popupNote, aOldLogin,
>+                                              aNewLogin.password);
>+        else if (notifyBox)
>+            this._showChangeLoginNotification(notifyBox, aOldLogin,
>+                                              aNewLogin.password);
>         else
>             this._showChangeLoginDialog(aOldLogin, aNewLogin.password);

This (and the other spot) would be simpler with something like:

  var blah = this._getPopupNote() || this._getNotifyBox();
  if (blah)
      this._showChangeLoginNotification(blah, ....)


The remember-password patch could do this too.

I guess this is still a bit unfortunate, because the function is calling _getPopupNote() again to figure out what the thing it got was. I guess that's fine, though, since eventually we're remove the notification bar path. Or maybe we could tweak the old notification bar API to be compatible with doorhangers...


>+            var browser = chromeWin.gBrowser.
>+                                    getBrowserForDocument(this._window.document);

this._window.top

This'll also need to pick up some of the CSS changes from bug 567814 comment 21.

Comment 6

[:Margaret Leibovic]

Attachment: revised patch

Addressed issues in comment 5. This patch depends on the patch in bug 567814. I'm also working on a patch to update the password notification tests, and it depends on both of these patches.

Comment 7

[:Margaret Leibovic]

Attachment: save/change password test updates

The tests required a lot of updating, but they all work/pass now.

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

Margaret: could you post a screen shot of the notification for UI review?

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 472568 [details] [diff] [review]
revised patch

>+            var secondaryActions = [
>+                // "No" button            
>+                {
>+                    label:     dontChangeButtonText,
>+                    accessKey: dontChangeButtonAccessKey,
>+                    popup:     null,
>+                    callback:  function(aNotifyObj, aButton) {
>+                        // do nothing
>+                    }
>                 }

Oh, don't need a secondaryAction here. If the user doesn't want to change their password, they can just dismiss the dialog. r+ with that removed.

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 472692 [details] [diff] [review]
save/change password test updates

Basically fine, just a few minor things to touch up. r+ with the following...

>+++ b/toolkit/components/passwordmgr/test/test_notifications.html
...
> 
>       case 1:
>-        is(gotUser, "notifyu1", "Checking submitted username");
>-        is(gotPass, "notifyp1", "Checking submitted password");
>-        bar = getNotificationBar(notifyBox, "password-save");
>-        ok(bar, "got notification bar");
>-        clickNotificationButton(bar, kNotNowButton);
>+
>         break;

Not sure why these checks got removed, should just be converted like the others.

>       case 2:
...
>-        clickNotificationButton(bar, kNeverButton);
>+        clickPopupButton(popup, kNeverButton);
>+        popup.remove();

In other places you're just calling popup.remove() without clickPopupButton(xxx, kNeverButton).

The clickPopupButton seems useless here, so let's be consistent and remove it. (I suppose it could just call popup.remove() itself, but meh.)


>       case 4:
...
>-        clickNotificationButton(bar, kRememberButton);
>+        clickPopupButton(popup, kRememberButton);
>+        popup.remove();

Why popup.remove()? Shouldn't the popup dismiss itself when the button is clicked?


>       case 6:
>-        // Same subtest, make sure we're getting the bar again.
>+        // Same subtest, make sure we're getting the popup again.
>         is(gotUser, "notifyu1", "Checking submitted username");
>         is(gotPass, "notifyp1", "Checking submitted password");
>-        bar = getNotificationBar(notifyBox, "password-save");
>-        ok(bar, "got notification bar");
>-        clickNotificationButton(bar, kNotNowButton);
>+        popup = getPopup(popupNotifications, "password-save");
>+        ok(popup, "got notification popup");
>         // Change prefs to no longer remember signons
>         prefs.setBoolPref("rememberSignons", false);
>+        popup.remove();
>         break;

Nit: the diffs would be slightly cleaner with the popup.remove() call placed a couple lines up, so that it's clear it's replacing the clickNotificationButton() call. But this isn't worth the effort to change now.

Comment 11

[:Margaret Leibovic]

Attachment: screenshot

bug 577927 should take care of the styling

Comment 12

[:Margaret Leibovic]

(In reply to comment #10)
> >+++ b/toolkit/components/passwordmgr/test/test_notifications.html
> ...
> > 
> >       case 1:
> >-        is(gotUser, "notifyu1", "Checking submitted username");
> >-        is(gotPass, "notifyp1", "Checking submitted password");
> >-        bar = getNotificationBar(notifyBox, "password-save");
> >-        ok(bar, "got notification bar");
> >-        clickNotificationButton(bar, kNotNowButton);
> >+
> >         break;
> 
> Not sure why these checks got removed, should just be converted like the
> others.

I got rid of this because I got rid of the kNotNowButton, so there didn't seem to be any purpose to this test anymore.

> >       case 2:
> ...
> >-        clickNotificationButton(bar, kNeverButton);
> >+        clickPopupButton(popup, kNeverButton);
> >+        popup.remove();
> 
> In other places you're just calling popup.remove() without
> clickPopupButton(xxx, kNeverButton).
> 
> The clickPopupButton seems useless here, so let's be consistent and remove it.
> (I suppose it could just call popup.remove() itself, but meh.)

I actually think that we should get rid of the popup.remove() here, since the clickPopupButton should be getting rid of the popup. Also, the test in case 3 depends on calling clickPopupButton(popup, kNeverButton) because it tests to make sure that "never" took effect.
 
> >       case 4:
> ...
> >-        clickNotificationButton(bar, kRememberButton);
> >+        clickPopupButton(popup, kRememberButton);
> >+        popup.remove();
> 
> Why popup.remove()? Shouldn't the popup dismiss itself when the button is
> clicked?

Yes :)

Comment 13

[:Margaret Leibovic]

Attachment: final patch

Comment 14

[:Margaret Leibovic]

Attachment: revised test updates

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 473097 [details]
screenshot

Primary action should be "Change Password" (So that users can only read the button and still answer the question).

I think we should also swap two words in the question:

"Would you like to change the password stored for username?"

(instead of stored password)

Comment 16

[Justin Dolske [:Dolske]]

Updated patches look fine, checked with faaborg on IRC and we're going to land this as-is. Please follow a followup bug for changing the strings, and we'll do that ASAP.

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

dolske requested a follow up bug for string changes: bug 594572

Comment 18

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/be7a791de5aa
http://hg.mozilla.org/mozilla-central/rev/77070aa1f917

Comment 19

[u88484]

Verified fixed using hourly build Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre from cset cca361001fda

Comment 20

[antistress]

The new notification system breaks consistency over the desktop on GNOME Linux, see https://bugzilla.mozilla.org/show_bug.cgi?id=567814#c45
Thanks