Closed Bug 567814 Opened 11 years ago Closed 10 years ago

Convert Remember Password notification bar to a doorhanger notification

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: fryn, Assigned: Margaret)

References

()

Details

(Keywords: icon, Whiteboard: [strings][doorhanger])

Attachments

(3 files, 7 obsolete files)

Password Manager's notification bars could be better served as doorhanger notifications to unify UI, prevent spoofing, etc.
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.
Attachment #448856 - Flags: feedback?(dolske)
Status: NEW → ASSIGNED
Blocks: 567804
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
Attachment #448856 - Attachment is obsolete: true
Attachment #448856 - Flags: feedback?(dolske)
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?
(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.
(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?
(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.
(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.
OS: Mac OS X → All
Hardware: x86 → All
(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.)
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"?
(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.
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.
Attachment #449098 - Attachment is obsolete: true
Attachment #465198 - Flags: ui-review?(faaborg)
Attachment #465198 - Flags: review?(dolske)
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.
Attachment #465198 - Attachment is obsolete: true
Attachment #465207 - Flags: ui-review?(faaborg)
Attachment #465207 - Flags: review?(dolske)
Attachment #465198 - Flags: ui-review?(faaborg)
Attachment #465198 - Flags: review?(dolske)
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)
Duplicate of this bug: 588303
Follow up bug 588309 for the change password prompt being converted over as well.
Nominating this for blocking since it is one of the few high profile doorhanger notifications.
blocking2.0: --- → ?
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.
Attachment #465207 - Flags: review?(dolske) → review-
blocking2.0: ? → beta6+
Assignee: fryn → margaret.leibovic
Addressed most issues from comment 17. I still need the correct icons, and I need to look into fixing the tests.
Attachment #465207 - Attachment is obsolete: true
Attachment #471174 - Flags: review?(dolske)
Attachment #465207 - Flags: ui-review?(faaborg)
Attachment #471174 - Attachment is patch: true
Attachment #471174 - Attachment mime type: application/octet-stream → text/plain
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.
Keywords: icon
[strings]: probably new strings for the direct question and action button
Whiteboard: [strings]
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?
Attachment #471174 - Flags: review?(dolske) → review-
(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?
Attached patch patch v6 (new image files) (obsolete) — Splinter Review
Attachment #471174 - Attachment is obsolete: true
Attachment #472565 - Flags: review?(dolske)
Blocks: 588309
(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.
>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.
Margaret: could you post a screen shot of the notification for UI review?
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.)
Attachment #472565 - Flags: review?(dolske) → review+
Attached patch final patch (obsolete) — Splinter Review
Attachment #472565 - Attachment is obsolete: true
Attached image screenshot
bug 577927 should take care of the styling
Attachment #473096 - Flags: ui-review?(faaborg)
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.
Attachment #473096 - Flags: ui-review?(faaborg) → ui-review-
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.
dolske requested a follow up bug for string changes: bug 594572
>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.
Blocks: 594572
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.
Marking "needs updated patch" based on Faaborg's ui-r-
Whiteboard: [strings] → [strings][needs updated patch]
(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-
Attachment #473090 - Attachment is obsolete: true
Is the styling for the different OSes handled in this patch?
http://hg.mozilla.org/mozilla-central/rev/6a8048ca8b63
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [strings][needs updated patch] → [strings]
(In reply to comment #39)
> Is the styling for the different OSes handled in this patch?

No, that should be handled by bug 577927.
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b6
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.
Status: RESOLVED → VERIFIED
Blocks: 595155
Depends on: 595178
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...
(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.
Flags: in-litmus?
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
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
Depends on: 598759
Blocks: 598759
No longer depends on: 598759
No longer blocks: 598759
Depends on: 598759
Depends on: 598953
Depends on: 599348
Blocks: 613563
Depends on: 611684
Depends on: 628234
Whiteboard: [strings] → [strings][doorhanger]
Blocks: 641901
Depends on: 643912
Depends on: 645110
You need to log in before you can comment on or make changes to this bug.