Convert Remember Password notification bar to a doorhanger notification

VERIFIED FIXED in mozilla2.0b7

Status

()

VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: fryn, Assigned: Margaret)

Tracking

(Blocks: 1 bug, {icon})

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus ?

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [strings][doorhanger], URL)

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

9 years ago
Password Manager's notification bars could be better served as doorhanger notifications to unify UI, prevent spoofing, etc.
(Reporter)

Comment 1

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

Updated

9 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

9 years ago
Blocks: 567804
(Reporter)

Comment 2

9 years ago
Created attachment 449098 [details] [diff] [review]
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
Attachment #448856 - Attachment is obsolete: true
Attachment #448856 - Flags: feedback?(dolske)

Comment 3

9 years ago
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?
(Reporter)

Comment 4

9 years ago
(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?
(Reporter)

Comment 6

9 years ago
(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
(Reporter)

Comment 8

9 years ago
(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"?
(Reporter)

Comment 10

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

Comment 11

8 years ago
Created attachment 465198 [details] [diff] [review]
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.
Attachment #449098 - Attachment is obsolete: true
Attachment #465198 - Flags: ui-review?(faaborg)
Attachment #465198 - Flags: review?(dolske)
(Reporter)

Comment 12

8 years ago
Created attachment 465207 [details] [diff] [review]
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.
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)

Updated

8 years ago
Assignee: fryn → margaret.leibovic
(Assignee)

Comment 18

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

Updated

8 years ago
Attachment #471174 - Attachment is patch: true
Attachment #471174 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 19

8 years ago
Created attachment 471175 [details]
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.
(Reporter)

Updated

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

Comment 22

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

Comment 23

8 years ago
Created attachment 472565 [details] [diff] [review]
patch v6 (new image files)
Attachment #471174 - Attachment is obsolete: true
Attachment #472565 - Flags: review?(dolske)
(Assignee)

Updated

8 years ago
Blocks: 588309

Comment 24

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

Comment 28

8 years ago
Created attachment 473090 [details] [diff] [review]
final patch
Attachment #472565 - Attachment is obsolete: true
(Assignee)

Comment 29

8 years ago
Created attachment 473096 [details]
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-

Comment 31

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

Updated

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

Comment 37

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

Comment 38

8 years ago
Created attachment 473836 [details] [diff] [review]
final patch (un-bitrotted)
Attachment #473090 - Attachment is obsolete: true

Comment 39

8 years ago
Is the styling for the different OSes handled in this patch?
(Assignee)

Comment 40

8 years ago
http://hg.mozilla.org/mozilla-central/rev/6a8048ca8b63
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [strings][needs updated patch] → [strings]
(Assignee)

Comment 41

8 years ago
(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

Comment 42

8 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

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

Updated

8 years ago
Blocks: 595155

Updated

8 years ago
Depends on: 595178
Depends on: 595183
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

8 years ago
(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.

Updated

8 years ago
Flags: in-litmus?

Comment 45

8 years ago
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

8 years ago
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

Updated

8 years ago
Depends on: 598759
(Reporter)

Updated

8 years ago
Blocks: 598759
No longer depends on: 598759

Updated

8 years ago
No longer blocks: 598759
Depends on: 598759

Updated

8 years ago
Depends on: 598953
Depends on: 599348
(Assignee)

Updated

8 years ago
Blocks: 613563

Updated

8 years ago
Depends on: 611684

Updated

8 years ago
Depends on: 628234
Whiteboard: [strings] → [strings][doorhanger]
(Assignee)

Updated

8 years ago
Blocks: 641901

Updated

8 years ago
Depends on: 643912

Updated

8 years ago
Depends on: 645110
You need to log in before you can comment on or make changes to this bug.