The default bug view has changed. See this FAQ.

Convert change password to a doorhanger panel

VERIFIED FIXED in Firefox 4.0b7

Status

()

Firefox
General
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: faaborg, Assigned: Margaret)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 4.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [strings][doorhanger])

Attachments

(3 attachments, 3 obsolete attachments)

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.
(Reporter)

Comment 1

7 years ago
Nominating this for blocking since it is one of the few high profile doorhanger notifications.
blocking2.0: --- → ?
Reasonable to slot this for beta6?
blocking2.0: ? → beta6+
(Assignee)

Updated

7 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 3

7 years ago
Created attachment 471324 [details] [diff] [review]
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.
Attachment #471324 - Flags: review?(dolske)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 4

7 years ago
[strings]: probably new strings for the direct question and action button
Whiteboard: [strings]
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.
Attachment #471324 - Flags: review?(dolske) → review+
(Assignee)

Comment 6

7 years ago
Created attachment 472568 [details] [diff] [review]
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.
Attachment #471324 - Attachment is obsolete: true
Attachment #472568 - Flags: review?(dolske)
(Assignee)

Updated

7 years ago
Depends on: 567814
(Assignee)

Comment 7

7 years ago
Created attachment 472692 [details] [diff] [review]
save/change password test updates

The tests required a lot of updating, but they all work/pass now.
Attachment #472692 - Flags: review?(dolske)
(Reporter)

Comment 8

7 years ago
Margaret: could you post a screen shot of the notification for UI review?
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.
Attachment #472568 - Flags: review?(dolske) → review+
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.
Attachment #472692 - Flags: review?(dolske) → review+
(Assignee)

Comment 11

7 years ago
Created attachment 473097 [details]
screenshot

bug 577927 should take care of the styling
Attachment #473097 - Flags: ui-review?(faaborg)
(Assignee)

Comment 12

7 years ago
(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 :)
(Assignee)

Comment 13

7 years ago
Created attachment 473109 [details] [diff] [review]
final patch
Attachment #472568 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
Created attachment 473110 [details] [diff] [review]
revised test updates
Attachment #472692 - Attachment is obsolete: true
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)
Attachment #473097 - Flags: ui-review?(faaborg) → ui-review-
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.
dolske requested a follow up bug for string changes: bug 594572
(Assignee)

Updated

7 years ago
Blocks: 594572
(Assignee)

Comment 18

7 years ago
http://hg.mozilla.org/mozilla-central/rev/be7a791de5aa
http://hg.mozilla.org/mozilla-central/rev/77070aa1f917
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → Firefox 4.0b6

Comment 19

7 years ago
Verified fixed using hourly build Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre from cset cca361001fda
Status: RESOLVED → VERIFIED

Updated

7 years ago
Blocks: 595155

Updated

7 years ago
Depends on: 595175

Updated

7 years ago
Depends on: 595201

Updated

7 years ago
Depends on: 595271
Depends on: 595183
Depends on: 595786
Blocks: 595786
No longer depends on: 595786

Comment 20

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

Updated

7 years ago
No longer depends on: 595201
Version: unspecified → Trunk
Whiteboard: [strings] → [strings][doorhanger]
You need to log in before you can comment on or make changes to this bug.