567814 - Convert Remember Password notification bar to a doorhanger notification

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Frank Yan (:fryn)]

Password Manager's notification bars could be better served as doorhanger notifications to unify UI, prevent spoofing, etc.

Comment 1

[Frank Yan (:fryn)]

Attachment: prototype v1 (relies on gavin's 'updated WIP' patch)

This patch puts 'Remember Password' for html form login and http auth in a doorhanger notification.

There are some pending issues:
* The icon indicator does not yet bring up another doorhanger when clicked. (It currently just says 'ohai!' in a standard alert.)
* Should we try to detect when html form authentication fails (and remove doorhanger)?

Also, Gavin's patch is undergoing major changes, so this will evolve greatly in the coming weeks.

Comment 2

[Frank Yan (:fryn)]

Attachment: WIP v2 (relies on gavin's doorhanger patch, v1)

this patch also applies two changes to nsLoginManagerPrompter on top of gavin's patch:
* it doesn't remove all notifications when location changes. (we need this to keep the 'remember password?' notification showing)
* it doesn't hide the indicator (next to the site identity box) when all notifications are dismissed. (we need this so the user can change/forget password, etc.)

TODO:
* clicking authentication indicator should show a notification

Comment 3

[Robert Kaiser]

What I wonder here, as this has toolkit changes and doorhanger is not toolkit but browser for now, is if Thunderbird (and possibly SeaMonkey) still have password prompts functional after this or if they need changes. Frank, any idea?

Comment 4

[Frank Yan (:fryn)]

(In reply to comment #3)
> What I wonder here, as this has toolkit changes and doorhanger is not toolkit
> but browser for now, is if Thunderbird (and possibly SeaMonkey) still have
> password prompts functional after this or if they need changes. Frank, any
> idea?

yeah, it has graceful fallbacks:

if we've got doorhanger notifications, use them.
else if we've got notification bars, use them.
else, use modal dialogs.

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #2)
> * it doesn't remove all notifications when location changes. (we need this to
> keep the 'remember password?' notification showing)

Are there going to be measures to prevent passwords from being leaked in public places?

Comment 6

[Frank Yan (:fryn)]

(In reply to comment #5)
> (In reply to comment #2)
> > * it doesn't remove all notifications when location changes. (we need this to
> > keep the 'remember password?' notification showing)
> 
> Are there going to be measures to prevent passwords from being leaked in public
> places?

Absolutely. Keeping a 'Do you want Firefox to remember your password?' notification visible (until dismissed) while the website is logging you in does not affect password exposure, just as the current notification bar does not.

A more relevant measure to prevent password from being leaked in public places would be making the master password UI flow more robust and secure, which we will also be working on.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > * it doesn't remove all notifications when location changes. (we need this to
> > > keep the 'remember password?' notification showing)
> > 
> > Are there going to be measures to prevent passwords from being leaked in public
> > places?
> 
> Absolutely. Keeping a 'Do you want Firefox to remember your password?'
> notification visible (until dismissed) while the website is logging you in does
> not affect password exposure, just as the current notification bar does not.

So it's not going to be accessible once you've dismissed it? I thought this was one of the selling points of the doorhanger system.

Comment 8

[Frank Yan (:fryn)]

(In reply to comment #7)
> So it's not going to be accessible once you've dismissed it? I thought this was
> one of the selling points of the doorhanger system.

See comment #2. The "iconbox" indicator will be there, so the user can re-display a notification upon clicking it. The notification will not contain the user's password, but it will provide options to change what firefox knows about your credentials, etc.

(Gavin's doorhanger patch v.1 does not handle this, but he is working on a followup that will.)

Comment 9

[Dão Gottwald [:dao]]

Well, I read comment 2, it's where I quoted from. When you've dismissed the notification and haven't previously saved credentials for that site, what are the "options to change what firefox knows about your credentials, etc"?

Comment 10

[Frank Yan (:fryn)]

(In reply to comment #9)
> Well, I read comment 2, it's where I quoted from. When you've dismissed the
> notification and haven't previously saved credentials for that site, what are
> the "options to change what firefox knows about your credentials, etc"?

I do not know what these options will be. You can contact the UI/UX team. AFAIK, these decisions are still being worked out.

Comment 11

[Frank Yan (:fryn)]

Attachment: patch v3 (updated to tip of tree)

This patch simply migrates the existing "remember password" notification bar to a panel notification.

UI for forgetting or modifying credentials will be implemented in followup bugs.

Comment 12

[Frank Yan (:fryn)]

Attachment: patch v4 (small persistence fix from v3)

This makes the "remember password" panel notification follow the same heuristic as the notification bar that it replaces: if a location change occurs > 30 seconds after the triggering of the notification, dismiss the notification.

Comment 13

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

Comment on attachment 465207 [details] [diff] [review]
patch v4 (small persistence fix from v3)

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>+    _getChromeWindow: function (aWindow) {
>+        var chromeWin = aWindow.
>+                        QueryInterface(Ci.nsIInterfaceRequestor).
>+                        getInterface(Ci.nsIWebNavigation).
>+                        QueryInterface(Ci.nsIDocShellTreeItem).
>+                        rootTreeItem.
>+                        QueryInterface(Ci.nsIInterfaceRequestor).
>+                        getInterface(Ci.nsIDOMWindow).
>+                        QueryInterface(Ci.nsIDOMChromeWindow);
>+        return chromeWin;
>+    },

Neil came up with an alternative that's more concise/direct (now used in HUDService.jsm):
var chromeWin = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                       .getInterface(Ci.nsIWebNavigation)
                       .QueryInterface(Ci.nsIDocShell)
                       .chromeEventHandler.ownerDocument.defaultView;

(the chromeEventHandler for a given docShell is the <xul:browser> element that contains it)

Comment 15

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

Follow up bug 588309 for the change password prompt being converted over as well.

Comment 16

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

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

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 465207 [details] [diff] [review]
patch v4 (small persistence fix from v3)

>+            var popupNote = this._getPopupNote();
>+            if (popupNote)
>+                popupNote = popupNote.getNotification("password-save");
>+            if (popupNote)
>+                popupNote.remove();
>             var notifyBox = this._getNotifyBox();
>             if (notifyBox)
>                 this._removeLoginNotifications(notifyBox);

Let's just make _removeLoginNotifications() get a notifyBox and popupNote itself, and remove items from both. That way it's a 1-line call to do this clean up.

Move the _getNotifyBox down to where it's first actually needed, then.

s/popupNote/popupNotifier/? "popupNote" keeps making me wonder what notes we're looking for, but maybe I'll get used to it. popupNotifier isn't much better. :/


>+    _showSaveLoginPopupNotification : function (aPopupNote, aLogin) {

Most of this is common with the notification bar flavor. Spin the strings/buttons off into helper functions, or maybe just add an argument to a shared _showSaveLoginNotification() that controls if the notification will be a notification box or doorhanger? (Sniffing the first arg to see which type it is might be a good way.)

>+            // "Not now" button
>+            {
>+                label:     notNowButtonText,
>+                accessKey: notNowButtonAccessKey,
>+                callback:  function() { /* NOP */ }
>+            }

Don't need this one for doorhangers.

>+        aPopupNote.show(browser, "password-save", notificationText, "",
>+                        mainAction, secondaryActions, { timeout: Date.now() + 30000 });

Need to update this to use the work from bug 572967, so that there's password-manager specific anchor node in browser.xul that can persist.

Also need the 64x64 key icon for this popup -- doesn't seem to already be in the tree, faaborg or shorlander should be able to dig it up (I'm pretty sure it was _made_ for Firefox 3, just never used).

Hmm, we may want to shorten the notificationText string, can't find faaborg's newest mockups offhand.

>+    _getChromeWindow: function (aWindow) {
...
>+        return chromeWin;
>+    },

As gavin noted above, there's apparently a shorter form of this now, let's use it!


>+    _getNotifyWindow: function () {
...
>+        } catch (e) {
>+            // If any errors happen, just assume no notification box.
>+            return null;
>+        }

Add a this.log() here, so debugging any failures here is more noticeable.

Finally, hrm, I bet a bunch of tests will need to be updated, since they're looking for notification bars.

Comment 18

[:Margaret Leibovic]

Attachment: patch v5 (cleanup and added anchor icon)

Addressed most issues from comment 17. I still need the correct icons, and I need to look into fixing the tests.

Comment 19

[:Margaret Leibovic]

Attachment: old key icon as anchor icon

In addition to a 64x64 icon, I think we also need a new 16x16 icon for the anchor icon because the white is difficult to see.

Comment 20

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

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

Comment 21

[Justin Dolske [:Dolske]]

Comment on attachment 471174 [details] [diff] [review]
patch v5 (cleanup and added anchor icon)

>+.popup-notification-icon[popupid="password-save"] {
>+  list-style-image: url(chrome://mozapps/skin/passwordmgr/key.png);
>+}

Should probably just go ahead and duplicate this image file (under a new name, as well as adding a 64x64 temp image), so that when shorlander finishes the new icons it's just an image file replacement, and no CSS changes are needed.

Between the URL bar being stylized (eventually) and a different color than notification bars, I'd assume we may want two slightly different images for notifications bars vs doorhangers (especially on OS X, as you noted, since the notification bar is dark gray, and the image is white).


Also, I did a quick testrun with the patch, and I there's a bug somewhere. I tested logging onto Gmail, since that exercises the code that's supposed to  make us ignore URL changes for a brief delay when showing a notification bar (since you login on google.com, but quickly get redirected to mail.google.com). Maybe doorhangers just don't have any of that hooked up yet? Seems like sometimes the "key" anchor icon would disappear before my login finished, or it would persist longer than expected. Could you try to reproduce, and if so see what's going awry?

Comment 22

[:Margaret Leibovic]

(In reply to comment #21)
> Also, I did a quick testrun with the patch, and I there's a bug somewhere. I
> tested logging onto Gmail, since that exercises the code that's supposed to 
> make us ignore URL changes for a brief delay when showing a notification bar
> (since you login on google.com, but quickly get redirected to mail.google.com).
> Maybe doorhangers just don't have any of that hooked up yet? Seems like
> sometimes the "key" anchor icon would disappear before my login finished, or it
> would persist longer than expected. Could you try to reproduce, and if so see
> what's going awry?

Hmm, I can't reproduce this. I just did a pull/update from mozilla-central, so maybe there was another issue causing this and it was resolved?

Comment 23

[:Margaret Leibovic]

Attachment: patch v6 (new image files)

Comment 24

[Axel Hecht [:Pike]]

(In reply to comment #20)
> [strings]: probably new strings for the direct question and action button

How are we doing about those new strings? Also, what about dolske's comment
> Hmm, we may want to shorten the notificationText string, can't find faaborg's
> newest mockups offhand.

Comment 25

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

>How are we doing about those new strings?

on my radar now, about to post in all of the notification related bugs for clarification from the people currently doing development what strings we have and what strings we need.

Comment 26

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

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

Comment 27

[Justin Dolske [:Dolske]]

Comment on attachment 472565 [details] [diff] [review]
patch v6 (new image files)

>+++ b/toolkit/themes/winstripe/mozapps/jar.mn
...
>+        skin/classic/mozapps/passwordmgr/key-16.png                (passwordmgr/key-16.png)
>+        skin/classic/mozapps/passwordmgr/key-64.png                (passwordmgr/key-64.png)

You'll need to add the aero entries to the manifest too (see further down in the file, where key.png is). r+ with that fixed.

        skin/classic/aero/mozapps/passwordmgr/key-16.png                (passwordmgr/key-16.png)
        skin/classic/aero/mozapps/passwordmgr/key-64.png                (passwordmgr/key-64.png)

(Don't need aero-specific image files for now.)

Comment 28

[:Margaret Leibovic]

Attachment: final patch

Comment 29

[:Margaret Leibovic]

Attachment: screenshot

bug 577927 should take care of the styling

Comment 30

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

Comment on attachment 473096 [details]
screenshot

Three small changes:

Primary action should be "Remember Password"

Secondary action text should change from "Never for this site" to "Never Remember Password"

You can remove the secondary option of "not now," since we handle that with the user ignoring the message.

Comment 31

[Axel Hecht [:Pike]]

Three comments as not-l10n, I don't see a visual indication of where I'd click on remember and where I'd drop down for alternatives. Secondly, is this accessible? And thirdly, "Never Remember Password" sounds like "never for any site" to me, is that really what we're saying?

As l10n, make sure to add localization notes making it clear that these are subtle choices of wording in English, as translations may turn out to be way on the wrong side of life.

Comment 32

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

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

Comment 33

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

>I don't see a visual indication of where I'd click
>on remember and where I'd drop down for alternatives

yes this is very bad, we have blocking bug 581193 to address this.  Mockups here:
http://blog.stephenhorlander.com/2010/07/01/geolocation-icons/

>Secondly, is this accessible?

My understanding is that the accessibility works the same way as the notification bars, these prompt the screen reader to deliver the message, and take focus.  We may however need to provide an invisible control with the string "cancel" since the click outside to close is less obvious when you only have audio.

>And thirdly, "Never Remember Password" sounds like "never for any
>site" to me, is that really what we're saying?

The notification is visually coming out of the site identity block, which should help to establish context for the user.  If we started to say "this site" after all options, it would effect every kind of notification as well (geolocation, etc).  Also this is no worse than the current notification bar interface that implied the current site but never specifically stated it.

Comment 34

[Axel Hecht [:Pike]]

FWIW, "never for this site" is the current wording, http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties#51.

Comment 35

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

Follow up bug 594586 for the one accessibility problem with screen readers being that the user will expect to hear cancel in the tab order.

Comment 36

[Johnathan Nightingale [:johnath]]

Marking "needs updated patch" based on Faaborg's ui-r-

Comment 37

[:Margaret Leibovic]

(In reply to comment #36)
> Marking "needs updated patch" based on Faaborg's ui-r-

The patch for bug 594572 will address the ui-r-

Comment 38

[:Margaret Leibovic]

Attachment: final patch (un-bitrotted)

Comment 39

[imradyurrad]

Is the styling for the different OSes handled in this patch?

Comment 40

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/6a8048ca8b63

Comment 41

[:Margaret Leibovic]

(In reply to comment #39)
> Is the styling for the different OSes handled in this patch?

No, that should be handled by bug 577927.

Comment 42

[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

This worked 99% of the time that I tried but at one point I did get the old modal dialog box :/  I'm not sure how to reproduce though but I recall the site was taking forever to login and I might have clicked the button again to login.

Comment 43

[Csaba Kozák [:WonderCsabo]]

It is now in trunk build, but it still has low-res key image. Why? 

Also, the "Remember" button is a drop-down button, but acts like a single OK button...

Comment 44

[Josh Tumath]

(In reply to comment #43)
> It is now in trunk build, but it still has low-res key image. Why?
Because Stephen is still making the high-res image. He'll land it when it's done.

Comment 45

[antistress]

Frank Yan wrote in intial report "Password Manager's notification bars could be better served as doorhanger notifications to unify UI [...]"

I'm running Firefox on GNOME

GNOME makes use of GTKInfoBar for similar notifications, which looks like the notification that can be found in current Firefox 3.6 ("bar-style notifications")

On GNOME, the new notification system breaks consistency over the desktop : very bad from my point of view and regarding user experience (GNOME HIG etc)

I think that on GNOME Firefox should keep the current Firefox 3.6 bar-style notifications system.

Since present status bar is fixed, shall i open a new bug especially for Linux ?

My comment also applied to Bug 588309 (Convert change password to a doorhanger panel)

Thanks

Comment 46

[antistress]

For the record i've filled Bug 598337 : Keep bar-style notifications for Firefox 4 on Linux instead of doorhanger notifications which breaks consistency on GNOME desktop