226735 - replace modal pre-submit save password dialog with post-submit bar

Status: VERIFIED FIXED

(Toolkit :: Password Manager, enhancement)

Description

[Oo.et.oO]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

if the password manager asked to save the most recently used password after the
resultant page has loaded, then the user would know if they indeed had used the
correct user/pass prior to saving.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Jason Barnabe (np)]

*** This bug has been marked as a duplicate of 181816 ***

Comment 2

[David P James]

Reopening. The duped bug is not a Firebird bug and since FB and Seamonkey use
different Password Managers duping was not appropriate. 

Comment 3

[David P James]

Confirming.

The issue surrounds the potential saving of incorrect username/password
combinations. I'm not sure how big a problem this is though for the average
user; for those of us constantly hosing profiles it may well be (but there are
workarounds for that) but I don't see it being too big for most people.

Your call Brian.

Comment 4

[Sean Umphlet (nosebleed)]

*** Bug 228892 has been marked as a duplicate of this bug. ***

Comment 5

[David P James]

*** Bug 242479 has been marked as a duplicate of this bug. ***

Comment 6

[Jesse Ruderman]

This is a problem for me.  I always hesitate before clicking "Yes" to a password
manager prompt because I'm not sure I typed the password correctly.  I would
expect this to be even more of a problem for users who haven't memorized the
procedure for removing a saved password.

Comment 7

[Jesse Ruderman]

*** Bug 251278 has been marked as a duplicate of this bug. ***

Comment 8

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

*** Bug 272538 has been marked as a duplicate of this bug. ***

Comment 9

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

*** Bug 327047 has been marked as a duplicate of this bug. ***

Comment 10

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

Myk's suggestion from bug 327047 is the right way to go:

> When a user submits a login form with an unfamiliar username (i.e. one which
> is not on the list of remembered usernames nor on the list of usernames to 
> never remember), in lieu of displaying a modal save password dialog before 
> submitting the form, Firefox should instead display a save password bar (most 
> likely in the same style and placement as the popup blocker bar) after loading 
> the login results page.
> 
> The save password bar should ask the same question and provide the same 
> options as the current save password dialog box, but it should not insist on 
> an answer before allowing the user to proceed, and it should disappear once 
> the user browses to a different page.
> 
> Then users will be able to choose whether to save the password after seeing if
> their login has succeeded or failed, and they won't be forced to make a 
> choice.

It requires that this bug depend on bug 268590, which I've added here as a dependency. Also, it should be a goal to have this fix in for Firefox2.

Comment 11

[Steffen Wilberg]

Copying the summary from the dupe (bug 327047).

Comment 12

[Marc Rost]

Mike, I'm not sure I totally understand why this bug depends on 268590.

I noticed that the dependency has only one vote for it, but I guess it doesn't matter how many votes it has as long as someone is actually working on it.

Comment 13

[Mike Connor [:mconnor]]

The current infobar API only supports one button, and one style, which is styled as a warning.  I'd want this to be a lot more neutral, and have the buttons directly accessible, before we do something like this.

Comment 14

[Deven Corzine]

This bug is a constant hassle.  I'm constantly finding that I've remembered the wrong password, or avoiding remembering the password because I'm not sure if it's right or wrong.  Occasionally, I'll bother to login to verify the password, then logout and login again to remember it, but this routine is a real hassle too.

There's no need for a status bar here.  The current pop-up dialog would be just fine, if it were simply non-modal.  Then, you could see the web page behind it and tell whether the password worked or failed, before deciding what to click.  (And if the dialog obscures part of the web page that tells you whether it worked or not, you can always drag the dialog, but most sites look VERY different after login success vs. login failure.)

Surely the modal flag can simply be turned off?  Or would that break something?

Comment 15

[Mike Connor [:mconnor]]

Dietrich, if you have a WIP patch, please drop it in here, but looks like session restore has you hosed all its lonesome.

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: WIP patch

Comment 17

[Asaf Romano (gone)]

> var realm = uri.scheme + "://" + uri.host;

or uri.prePath (which will also give you the port/user/pass!).

Comment 18

[Neil Deakin]

Attachment: Use notification bar for passwords

Comment 19

[Neil Deakin]

I didn't include the string changes for this patch, as beltzner indicated it would need to be changed, but the access keys need to be separated from the labels.

Comment 20

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

Shaver raised an interesting point which is that there are some web pages which use a pop up dialog to enter the username and password. Then that window closes.

I think the logical thing to do would be to show this notification message on the window/tab which then receives focus and loads the newly logged in resource. Make sense?

Neil, if you want the new wording that I'm thinking of, it's:

[ Firefox can remember the password you just entered for this site ( Never for this Site ) ( Not now ) ( Remember ) ]

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think trying to do that will be pretty hard, since the window is dismissed before the opener is finished being reloaded (sometimes before it has even contacted the server, if Dell's site is being slow), and so we'd need to show the notification bar _after_ the reload.  (Will it survive redirects, etc.?  Should it?)

You can play with the login windows on dell.com and eq2players.com for a bit to see some of the models that are in use, though by no means all of them.

Comment 22

[Mike Connor [:mconnor]]

So, given that we're displaying the notification onsubmit, but the pageload is happening afterward, I think we have a solution to that, at least in part.

I don't think this is insurmountable, myself, and I would like to go for trying to make it work instead of resigning ourselves to modal dialogs.

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 225586 [details] [diff] [review]
Use notification bar for passwords

Clearing review until Neil addresses shaver's concerns (though dell.ca seems to log in from the main content window now...)

Comment 24

[Neil Deakin]

Neither dell.ca nor eq2players.com open a login in a popup window that I can see.

Would it suffice to just open the password notification in the opener window when triggered from a document in a window without the toolbar/menubar/etc ?

Comment 25

[Mike Schroepfer]

Given how close we are to b1 and risk of this change - taking off the blocker list.

Comment 26

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

This really speaks to a common nuisance with one of our most popular features, and I would really like to see this get some testing on trunk and then try to re-convince Schrep and the rest of the drivers that it's worth taking for Firefox 2.0 Beta 1.

Neil, can we try your heuristic in comment 24 and try to get a bunch of login URLs to test it out and get a better sense of the cost benefit ratio? I could even see us using a heuristic like "if the login form is in a popup (chromeless) window, use a modal dialog for password save confirmation, else use the notificationmessage".

Comment 27

[Neil Deakin]

Attachment: add support for opening in the opener window.

This version opens the notification in the opener window for cases where a window doesn't have a toolbar.

Comment 28

[Michael Wu [:mwu]]

(In reply to comment #27)
> Created an attachment (id=227462) [edit]
> add support for opening in the opener window.
> 
> This version opens the notification in the opener window for cases where a
> window doesn't have a toolbar.
> 
It is probably better to use a NS_FORMSUBMIT_SUBJECT observer (or NS_EARLYFORMSUBMIT_SUBJECT when bug 343182 lands) to take advantage of the work done in bug 257781.

Comment 29

[Bryce Mozilla Nesbitt]

While I agree it's better to ask the "save password" question after form submit (so you know if the password is good), I think there is an even better option.  

Simply turn the form field yellow (like the google toolbar).  Then right-click for a context menu of "save password".  That way it's easy to save passwords, but the modal box is not in your way the rest of the time.

Comment 30

[shake_78]

Please just copy Opera's wand and get it out now.  I have switched to Opera because of this password issue already, and it sucks because it doesn't have the add-ons or the compatibility of firefox.  A yes/no box should pop up after one submits the password, before the page loads, asking if she want to save the password.  It doesn't matter if the username/password saved is wrong.  The next time she tries to log into the site, she will save the correct username/password combo. Then, the next time the person tries to log in to the site, firefox will present them with the correct and incorrect combo, and also an option to delete a combo. This dialog can also present multiple correct combos under different usernames.  Is this a patent issue, which is why you all are muddling over this, or just not seeing the big picture?

Comment 31

[Bryce Mozilla Nesbitt]

See also Bug #260295 . If there's a way to do password remembering *without* adding a step to normal login, that would be great. No modal dialog at all, before or after login.

Comment 33

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

Here is a non-modal redesign for notification: http://blog.mozilla.com/faaborg/2007/03/06/would-you-like-to-redesign-notification-in-firefox-yes.-not-now.-never./

Comment 34

[u279076]

I have a suggestion on how to approach this:

First off, I like the idea of the bar notification.  Possibly make it similar to the "missing plugins" bar that pops up saying something like "login correct" and a button to "save password".

Secondly, by default, keep the dialog that pops up but add a button or checkbox to it that says something to the effect of "disable this dialog".  Clicking this should enable the "bar" notification.

From past experience, some users have selective vision and simply will ignore the bar at the top.  Some users need to be asked and need dialogs to pop out at them to grab their attention.  I think the bar feature will simply be a luxury that accommodates to the internet savvy users that have suggested this bar feature time and again.

Just my 2 cents...

Comment 35

[Justin Dolske [:Dolske]]

Just to add a note from scattered discussions I've had in the past with some people...

A major concern with using the "info bar" is that it's easily spoofable by content. That's not a deal-breaker, but I think we should seriously consider some form of Alex's notification UI (see comment 33), so that user credential entry is in a hard-to-spoof chrome location. This is especially true for supporting authentication methods more advanced than last century's method of just sending the password to the server. For example, OpenID or SRP (bug 356855).

I think most users find popups to just be annoying, so we ought not to keep them. For example, see: http://robert.accettura.com/archives/2007/05/07/disable-password-manager/ "Selective vision" is a concern, though, and I'm sure that will be factor in making notifications visible enough to catch your attention, but not annoyingly so.

Comment 36

[Dão Gottwald [:dao]]

(In reply to comment #35)
> A major concern with using the "info bar" is that it's easily spoofable by
> content.

Why is that a problem for the "save password" UI?

IMHO, it's much cleaner than the content-overlapping solution suggested by Alex.

Comment 37

[Roland]

(In reply to comment #34)
> From past experience, some users have selective vision and simply will ignore
> the bar at the top.
> 

Which is bad because of... what?

IMHO - the option to save the password is complementary feature that should in no way interrupt normal browsing. 

Having an info bar like current plug-in and pop-up blocking notices, would do exactly what can be expected from such a notification - stay out of the way until you decide to attend to it.

Additionally - since the question is now in a modeless info bar, there is no need to have "Not now" button, as this simply equals to ignoring the message altogether (or just closing the info bar), which additionally simplifies the ui by offering less choices thus causing less confusion (even better - i doubt there is any good reason to have "Never" button on this info bar - if I never want to remember the password, i'll just keep ignoring the info bar as long as I want to)

that said - I *do* like the modeless notification as presented in comment 33 even better a a general ui improvement. As such I think still that notifications like popup blocking, plug-in installation and password remembering should keep their general "info bar" appearance, to cause the least amount of distraction.

Comment 38

[Deven Corzine]

The main problem is having to answer whether to save the password before knowing whether it worked.  I like the idea of a post-submit bar, but a non-modal dialog would be sufficient to solve the problem.

Comment 39

[Mike Connor [:mconnor]]

I don't get the spoofing risk either, especially with the situations when it'd be used.

Comment 40

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

Even if spoofing isn't a concern, other reasons I like the hanging notification approach include:

*The user will feel less compelled to provide an answer, because no controls are initially displayed with a level 2 notification (http://blog.mozilla.com/faaborg/2007/03/06/would-you-like-to-redesign-notification-in-firefox-yes.-not-now.-never./)
*The notification takes up less screen real estate when it is displayed
*The notification doesn't take up any screen real estate after the user has continued on with their task.
*These notifications work well for a variety of other situations, so if they get implemented they will end up improving more than just the password manager.

Comment 41

[Dão Gottwald [:dao]]

(In reply to comment #40)
> Even if spoofing isn't a concern, other reasons I like the hanging notification
> approach include:
> 
> *The user will feel less compelled to provide an answer, because no controls
> are initially displayed with a level 2 notification
> (http://blog.mozilla.com/faaborg/2007/03/06/would-you-like-to-redesign-notification-in-firefox-yes.-not-now.-never./)

I don't see level 2 as an option for this bug -- I guess it would look more like level 3, as we need a "Save" and a "Never for this site" button; probably even "Not now", as you didn't provide another way for closing the dialog. Omitting one of them would be a functionality regress. Using the notification just as a link to a real dialog would be a workflow regress.

The "popup blocked" UI goes into the same direction, imho. With regards to the "Download finished" UI, I don't see why your proposal is better than the notification that we have right now.

> *The notification takes up less screen real estate when it is displayed

I doubt that for level 3. But anyway, did anybody actually complain about the screen real estate that a notification bar takes? It's quite narrow after all.
The fact that the proposed notifications overlay content seems more questionable to me.

> *The notification doesn't take up any screen real estate after the user has
> continued on with their task.

I don't understand this point.

> *These notifications work well for a variety of other situations, so if they
> get implemented they will end up improving more than just the password manager.

As I don't like the content-overlaying approach, I doubt that using it for many other things would be a win ;)

Comment 42

[Dão Gottwald [:dao]]

(In reply to comment #40)
More issues with your proposal:

* Your notifications need an icon and browser chrome to connect with. That makes
  it unworkable for popups and full-screen surfing.
* They overlay not only content but potentially also the browser chrome. (more
  annoyance)
* I think the distinction that we have now, i.e. between page-relevant and
  general/global notifications, is actually useful.

Comment 43

[Mike Connor [:mconnor]]

(In reply to comment #40)
> Even if spoofing isn't a concern, other reasons I like the hanging notification
> approach include:
> 
> *The user will feel less compelled to provide an answer, because no controls
> are initially displayed with a level 2 notification

Level 2 would be a regression, IMO, as Dao explained already.  I think this is relatively infrequent enough that its acceptable to display a visible choice immediately.  If we push it too far into the background, it becomes more work to save passwords, and I don't want to go down that path unless you can convince me that users don't want to be asked the question in the general case (we just want to ditch modality.

> *The notification takes up less screen real estate when it is displayed
> *The notification doesn't take up any screen real estate after the user has
> continued on with their task.

Its not a ton of space, and far less obtrusive than a level 3 with the same functionality, and the infobar will go away after the next page navigation.

> *These notifications work well for a variety of other situations, so if they
> get implemented they will end up improving more than just the password manager.

I don't think its the right UI for this interaction, and I don't think the notification design is ready to implement either, so let's go with the notification bar for now and not rathole too much here.

Comment 45

[Mike Connor [:mconnor]]

What are we waiting on here?

Comment 46

[Michael Ventnor]

Justin, are you still working on this? If not, I can post the patch in my tree for this.

Comment 47

[Justin Dolske [:Dolske]]

Sorry, missed the last comment. Yes, I'm still working on this. But if you've got a work-in-progress patch, feel free to attach!

Comment 48

[Michael Ventnor]

Attachment: Patch 1

There hasn't been any activity on this bug despite the status notes saying a patch is wanted this week. Here's the patch I implemented for myself, you can take it if you don't think anyone else's patch can be done on time.

Am I allowed to change the nsILogin interface, if I don't break any API's?

Comment 49

[Michael Ventnor]

Attachment: Patch 1 screenshot

Obligatory screenshot. This way the bar looks less cluttered, but the most common option (Remember) is the first item in the menu so it doesn't take much extra effort. A separator also makes the menu easier to interpret.

Comment 50

[Dão Gottwald [:dao]]

Comment on attachment 272955 [details] [diff] [review]
Patch 1

>+    notificationBox.appendNotification(notifyLabel, "save-password-prompt",
>+                                       "chrome://browser/skin/Info.png",

That icon is used for refresh-blocked, popup-blocked, images-blocked and chromeless-info (chromelessWindow.warningNoLocation). The latter is already questionable and should probably be changed. In any case, I don't think that icon is suitable for save-password-prompt.

Comment 51

[Justin Dolske [:Dolske]]

(In reply to comment #48)

> There hasn't been any activity on this bug despite the status notes saying a
> patch is wanted this week.

...because I was working on the code? I'm not sure what comments you were expecting, although I has asked in my last comment that if you had a patch-in-progress to attach it.

I'll take a look at your patch today.

Comment 52

[Michael Ventnor]

Comment on attachment 272955 [details] [diff] [review]
Patch 1

(In reply to comment #51)
> (In reply to comment #48)
> 
> > There hasn't been any activity on this bug despite the status notes saying a
> > patch is wanted this week.
> 
> ...because I was working on the code?

I thought about that a bit too. I suppose I was expecting some WIPs but that doesn't matter. Sorry about that.

Comment 53

[Mike Connor [:mconnor]]

I think this wasn't the right UI, the popup feels heavy to me.

We were talking about more like the following.

Password Manager can remember the password you just entered.   [Remember] [Never For This Site] [x]

Hoping to see dolske's patch today.

Comment 54

[Justin Dolske [:Dolske]]

Attachment: Draft patch, v.1

So, here's the patch (largely based on Neil's last patch), which I should have posted a day or two ago, had I not been under the weather. :(

Nits:

* HTTP Auth prompting should be smarter about watching for a failed authentication, and removing the notification bar (or delaying the notification bar until it knows the authentication was good).

* Persisting the notification bar across redirects and the like isn't in, as I didn't have a handy way to test it.

* I'd like to move all the UI prompting code out of nsLoginManager.js and into nsLoginMangerPrompter.js, but that's best done in a separate patch (probably after this lands?).

* I only commented out the old code, and didn't fix the access keys for the UI buttons. I was thinking about switching this to a pref for checkin, until we're sure the notification bar isn't going to have any weird regressions (like funky logins with redirects and cross-window loads), but now that I look again the changes are minor enough this isn't worth doing.

Any initial review comments from Gavin / Mconnor / anyone else, before I clean it up for a checkin?

Comment 55

[Justin Dolske [:Dolske]]

Bah, I also meant to add:

I think the biggest potential for regressions is what I mentioned in the last comment (and others identified), namely, funky site logins involving redirects and cross-window loads, etc. Worst case is the notification bar goes away before you click a button.

Another tweak we can add is what Michael Ventnor did in his patch, where the notification bar is forced to stay up for a minimum time period. That would probably fix any really tricky problems, but the downside is that it might not go away when you expect it to. I think it's best to defer this kind of code change until the need for it is clearer, though. 

Comment 56

[Michael Ventnor]

(In reply to comment #55)
> Another tweak we can add is what Michael Ventnor did in his patch, where the
> notification bar is forced to stay up for a minimum time period. That would
> probably fix any really tricky problems, but the downside is that it might not
> go away when you expect it to. I think it's best to defer this kind of code
> change until the need for it is clearer, though. 

The reason I made it that way in my patch was to give the user enough time to make a decision no matter what funky redirect dances the login does; if they don't then they're likely ignoring the bar. If the user is ignoring the bar outright, would they care when the bar goes away?

Also, may I make a suggestion as to include the username and hostname in the notification? The reason I did that in my patch is because I want the user to make sure they know what login they were saving.

Comment 57

[Justin Dolske [:Dolske]]

(In reply to comment #56)

> The reason I made it that way in my patch was to give the user enough time to
> make a decision no matter what funky redirect dances the login does; if they
> don't then they're likely ignoring the bar. If the user is ignoring the bar
> outright, would they care when the bar goes away?

I think there are two main problems with having a fixed timeout:

1) It's a poor user experience when the notification bar sticks around even after they start clicking links to other sites. Sure, you can close it manually, or just ignore it, but that's an annoyance.

2) Doesn't account for differing network/site speeds. Consider a site does a "funky redirect dance" [:-)], but only after a slow DB query completes or the user's bad network reconnects.

I don't think a timeout is a completely wrong idea, but I'd like to try doing something more clever first.

Comment 58

[Justin Dolske [:Dolske]]

Attachment: Draft patch, v.2

Cleaned up draft patch.

I think the main thing left to do is deal with redirects. Gmail uses that, so we probably shouldn't check this in the issue is fixed (or a temporary solution like a timeout is used). I'll check on this and do one or the other shortly.

Comment 59

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

We shouldn't use a timeout for accessibility reasons.  Ideally we should be able to tell the difference between navigations initiated by the browser, and navigations initiated by the user, and only the later would clear the notification bar.

Comment 60

[Aaron Lawrence]

Personally, I would find this quite useful even if it just had a timeout, so long as:
a) it could be cleared immediately, manually - I believe that's already in
and
b) The prompt shows the original Page title and URL the password is being remembered for.
(b) might be a good idea anyway.
But perhaps that makes the UI too large and intrusive.

Comment 61

[Michael Ventnor]

+        if (!notifyWindow.toolbar.visible && notifyWindow.opener) {
+            this.log("Using opener window for notification bar.");
+            notifyWindow = notifyWindow.opener;
+            var navNav = win.QueryInterface(Ci.nsIInterfaceRequestor).
+                             getInterface(Ci.nsIWebNavigation);
+            var rootItem = navNav.QueryInterface(Ci.nsIDocShellTreeItem)
+                                 .rootTreeItem;
+            var rootWin = rootItem.QueryInterface(Ci.nsIInterfaceRequestor).
+                                   getInterface(Ci.nsIDOMWindow);
+            notifyWindow = rootWin.document;

notifyWindow is supposed to be a window, not a document? Plus, I still think its a good idea to include the username and domain in the notification message.

Comment 62

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

The basic approach of this patch to figuring out when to dismiss the notification bar combines two triggers:

1) If the user clicks in the content area, dismiss the bar.

2) If the location changes, dismiss the bar *if* it has been at least 10 seconds since the bar was created.

I think that in most cases, #1 is doing the dismissal work. We could probably get away without having #2 at all, but I kept it in to be conservative.

I had spent time looking into how to handle HTTP redirects (and did a prototype using nsIChannelEventSink, which biesi suggested), but it was looking like it would be a little bit ugly trying to map redirects between nsIChannels to notification bars in a <browser>. And I'm still not sure how to monitor javascript-driven |document.location| redirects, but the more I thought about it the more complex that seemed. [For example, you'd have to mix in some state about what the user was doing, so you could try to determine if it was happening during a login "redirect" sequence, or by something like, say, a JS onclick handler.]

So, instead of trying to micromanage JS and HTTP activities, just watching for content-area clicks is about as good and is a lot simpler. This patch also cleans up a few other edge cases.

Comment 63

[Michael Ventnor]

(In reply to comment #62)
> I think that in most cases, #1 is doing the dismissal work. We could probably
> get away without having #2 at all, but I kept it in to be conservative.

Unless of course you don't have, or can't use, a mouse. :)

Comment 64

[Justin Dolske [:Dolske]]

(In reply to comment #63)

> Unless of course you don't have, or can't use, a mouse. :)

Well, "click" here generally includes keyboard events that are like clicks. For example, tabbing to a link or button and pressing enter. There might be other user events we want to handle, but this seemed like a good starting point from the testing I did.

Comment 65

[Deven Corzine]

I'm not sure that dismissing the bar just because you clicked in the content area makes sense.  That's not the way other bars behave, such as the popup-blocker.  The bar will still be relevant to the page on the screen, and it's possible that the user hasn't decided yet whether to save the password, but wants to interact with the page.

Triggering on location change is the most appropriate solution.  Redirects are definitely a problem.  Timeouts could be problematic, in that a user might click on a link in the first second or two after the page loads.  However, leaving the bar onscreen longer than necessary is better than losing it during a redirect.  Since refresh reloads are often 3-5 seconds, 10 seconds seems like a reasonable timeout to use.

My vote would be to skip #1 and implement #2 only.

If redirects, automatic refreshes and Javascript-initiated reloads could be distinguished from location changes triggered by user action, that would be ideal, but is it possible?

Comment 66

[Mike Connor [:mconnor]]

Missing the freeze, moving out.

Comment 67

[Marc Rost]

My vote would be to implement #1 only and skip #2.
I think dismissing the bar after the user clicks in the content area makes good sense. The original idea was to keep the dialog until the resultant page has loaded, and the user knows that the user/pass was good. I don't remember anyplace where I had to click in the content area to figure out if I got the expected page, or the 'you goofed' page.
I suppose we can get all kinds of fancy about this, but my vote would be to implement this sooner with minimum bells and whistles; instead of waiting on any extras.

Comment 68

[Roland]

I would vote strongly against implementing #1

All other such bars are not being dismissed this way and I see no reason whatsoever to change that. The "bar" is in itself out of the way enough that unless I really want to save the password i might just ignore it is even there. There is no need to dispose it before user navigates to another page.

And "navigating to another page" might very well be defined as an interaction by user (via mouse or keyboard) on a "navigation widget" like a link or a button on a page. There is no need to monitor a network activity - for all we know, the page might want to refresh itself every 5 minutes after log in...

Comment 69

[Mike Connor [:mconnor]]

I think Justin meant "click in content" as in clicking on a link, but I'm not sure...

Comment 70

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

>All other such bars are not being dismissed this way and I see no reason
>whatsoever to change that.

Madhava has been doing some design work on notification, so the UX team may soon propose a new design that replaces the notification bar entirely.  I'll make sure this bug links to the new mockups (and the ensuing discussion).  I'm pretty sure this design will allow the user to dismiss notifications with a click anywhere outside of the notification itself.

The reason we feel this is a good idea for certain types of notifications is because it allows the user to "look the other way" without feeling forced to answer a specific question.  Even if a dialog box is not modal, making the decision to choose "not now" still requires more thought than simply carrying on with the task at hand.  Kind of like if someone walks over and asks you a question and you don't even take the time to look up, but instead just zone them out.

Comment 71

[Justin Dolske [:Dolske]]

(In reply to comment #69)
> I think Justin meant "click in content" as in clicking on a link, but I'm not
> sure...

Yes. If you somehow ended up on a blank page (eg, about:blank) with a save-password notification bar, clicking anywhere in the blank content area would dismiss the notification bar.

Comment 72

[Deven Corzine]

It's actually easier to "look the other way" if you leave the bar there until they leave the page.  Otherwise, it will reflow the page and move all of the content up.  If the user doesn't notice the bar right away (which is quite possible), and the page suddenly jumps for no obvious reason when the user clicks in the window, it will likely be confusing and disconcerting for the user.

Comment 73

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

Comment on attachment 273924 [details] [diff] [review]
Patch for review, v.1

>Index: browser/base/content/browser.js

>+    // XXX I don't think the original code here is right; it causes
>+    // notification bars to persisit across reloads, page loads that
>+    // go to the same URL (eg POSTs to dynamic_content.cgi), and navigation
>+    // within a page to different anchors.

Why don't you think that's incorrect? It seems to be intentional, at least, given the comment below:

>     // This code here does not compare uris exactly when determining
>     // whether or not the message should be hidden since the message
>     // may be prematurely hidden when an install is invoked by a click
>     // on a link that looks like this:

>+        // Remove all the notifications, except for those which want to
>+        // persist across the first location change.
>+        var nBox = gBrowser.getNotificationBox(selectedBrowser);
>+        for (var n = nBox.allNotifications.length - 1; n >= 0; n--) {
>+          var notify = nBox.allNotifications[n];
>+          if (notify.ignoreFirstLocationChange)
>+            notify.ignoreFirstLocationChange = false;
>+          else if (!notify.ignoreLocationChangeTimeout ||
>+            (Date.now() / 1000) > notify.ignoreLocationChangeTimeout)
>+            nBox.removeNotification(notify);
>+        }

This sounds like a block of code that could be part of the browser API... something like gBrowser.selectedBrowser.clearNotifications() perhaps? With a boolean parameter to indicate whether the location change/timeout properties should be checked/updated?

>@@ -4069,16 +4084,26 @@ function asyncOpenWebPanel(event)

>  function contentAreaClick(event, fieldNormalClicks)

>+   // Remove notification bar for saving passwords, if it's being shown.

I don't think this is a good idea, as we had discussed on IRC. I'd rather we either just leave it indefinitely until dismissed by the user, or only do the timeout+location change thing. You should get the Mikes to make the call on this.

>Index: toolkit/components/passwordmgr/public/nsILoginManager.idl

>@@ -185,15 +187,21 @@ interface nsILoginManager : nsISupports 

>+    /*
>+     * Temp hacks.
>+     */
>+    void removeSaveLoginNotification(in nsIDOMWindow aWindow);
>+    void showSaveLoginNotification(in nsIDOMWindow aWindow,
>+                                   in nsILoginInfo aLogin);

Why are these "temp hacks"? I'm not sure it's a good idea to make the login manager implementation rely on knowing about DOM windows. Couldn't you instead have browser code listen for a observer topic ("prompt-save-password" or something), pass in the login being saved, and have it call setLoginSavingEnabled/addLogin as needed?

>Index: toolkit/components/passwordmgr/src/nsLoginManager.js

>@@ -871,28 +871,20 @@ LoginManager.prototype = {

>         // Prompt user to save a new login.
>-        var userChoice = this._promptToSaveLogin(win);
>+        // this._promptToSaveLogin(win, formLogin);

Is there any reason we want to keep this around rather than just removing it? If we go the notification route other clients could implement it on their own, right?

>     /* ---------- User Prompts ---------- */
> 
> 
> 
> 
>+
>+

Why so much whitespace?

>+        var notificationBox = browser.getTabBrowser()
>+                                     .getNotificationBox(browser);

This boils down to browser.parentNode, we should just expose a descriptive getter for that on the <browser> and use it directly (there's another caller like this in removeSaveLoginNotification).

>Index: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties

>-rememberButtonText = &Remember
> notNowButtonText = &Not Now
> neverForSiteButtonText = Ne&ver for This Site
>+notifyBarNeverForSiteButtonText = Never for This Site
>+notifyBarNeverForSiteButtonAccessKey = e
>+rememberButtonText = &Remember
>+notifyBarRememberButtonText = Remember
>+notifyBarRememberButtonAccessKey = R

Leave rememberButtonText where it was so that all of the notifyBar entities are together? Might be worth adding a localization note explaining how the two "Remember" strings differ, although I suppose the "notifyBar" prefix might be sufficient.

Comment 74

[Justin Dolske [:Dolske]]

(In reply to comment #73)

> Why don't you think that's incorrect? It seems to be intentional, at least,
> given the comment below:
> 
> >     // This code here does not compare uris exactly when determining
> >     // whether or not the message should be hidden since the message
> >     // may be prematurely hidden when an install is invoked by a click
> >     // on a link that looks like this:

> >+    /*
> >+     * Temp hacks.
> >+     */
> >+    void removeSaveLoginNotification(in nsIDOMWindow aWindow);
> >+    void showSaveLoginNotification(in nsIDOMWindow aWindow,
> >+                                   in nsILoginInfo aLogin);
> 
> Why are these "temp hacks"? I'm not sure it's a good idea to make the login
> manager implementation rely on knowing about DOM windows. Couldn't you instead
> have browser code listen for a observer topic ("prompt-save-password" or
> something), pass in the login being saved, and have it call
> setLoginSavingEnabled/addLogin as needed?

The existing prompting code lived in the same place (nsLoginManager.js), and the new code used some of the same support functions, so this was the easiest place to stick it. I think *all* the prompting code needs to move over to nsLoginManagerPrompter.js, though, since that seems a more sensible place for it. I didn't want to do that cleanup here, though.

> >-        var userChoice = this._promptToSaveLogin(win);
> >+        // this._promptToSaveLogin(win, formLogin);
> 
> Is there any reason we want to keep this around rather than just removing it?
> If we go the notification route other clients could implement it on their own,
> right?

There was some question about making this a pref or not? I think it can probably just be removed, although I should check to make sure doing so doesn't break using login manager for Seamonkey any more than it already is... :-)

 
> >+        var notificationBox = browser.getTabBrowser()
> >+                                     .getNotificationBox(browser);
> 
> This boils down to browser.parentNode, we should just expose a descriptive
> getter for that on the <browser> and use it directly (there's another caller
> like this in removeSaveLoginNotification).

So, implement |browser.getNotificationBox()|?


> >-rememberButtonText = &Remember
> > notNowButtonText = &Not Now
> > neverForSiteButtonText = Ne&ver for This Site
> >+notifyBarNeverForSiteButtonText = Never for This Site
> >+notifyBarNeverForSiteButtonAccessKey = e
> >+rememberButtonText = &Remember
> >+notifyBarRememberButtonText = Remember
> >+notifyBarRememberButtonAccessKey = R
> 
> Leave rememberButtonText where it was so that all of the notifyBar entities are
> together?

As it is, all the rememberButton entries are together. :) Maybe I can just remove the old entries if we get rid of the old prompting, so this is less confusing.

Comment 75

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

(In reply to comment #74)
> > This boils down to browser.parentNode, we should just expose a descriptive
> > getter for that on the <browser> and use it directly (there's another caller
> > like this in removeSaveLoginNotification).
> 
> So, implement |browser.getNotificationBox()|?

No, I meant just browser.notificationBox, which can just be a field that holds browser.parentNode.

Comment 76

[Justin Dolske [:Dolske]]

(In reply to comment #73)

> I don't think this is a good idea, as we had discussed on IRC. I'd rather we
> either just leave it indefinitely until dismissed by the user, or only do the
> timeout+location change thing. You should get the Mikes to make the call on
> this.

I checked with Beltzner on IRC, he agrees with you. I'll nix the click-to-dismiss in the next patch.

Comment 77

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.2

Update to address previous review comments.

I debated making a onLocationChange method in the notification box binding, and moving the browser.js code there (or maybe have the notification box get such events itself?), but in the end it seemed like more work than it was worth for now.

Comment 78

[Justin Dolske [:Dolske]]

(oops, that patch somehow included a "notificationBoxi" typo [trailing 'i'], but it otherwise fine)

Comment 79

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

(In reply to comment #74)
> The existing prompting code lived in the same place (nsLoginManager.js)

Sure, but the existing prompting code isn't browser-specific.

> and the new code used some of the same support functions, so this was the
> easiest place to stick it.

It's not clear to me what "support functions" are needed by the actual notification code, it seems to me that all you really need is a reference to the login, and a call to addLogin() or setLoginSavingEnabled() (both public). The login can be passed in the observer notification or callback.

> I think *all* the prompting code needs to move over to
> nsLoginManagerPrompter.js, though, since that seems a more sensible place for
> it. I didn't want to do that cleanup here, though.

I think having toolkit code not depend on browser internals is more than just "cleanup". It seems to me that it'd be easier to do this right the first time than to land this patch and then change it later.

> There was some question about making this a pref or not? I think it can
> probably just be removed, although I should check to make sure doing so
> doesn't break using login manager for Seamonkey any more than it already
> is... :-)

Well, leaving it there but not calling it isn't going to help SeaMonkey, right? In any case, if my notification suggestion was implemented they could just handle this themselves and choose whatever behavior they want :)
 
> So, implement |browser.getNotificationBox()|?

On second thought, this needs to check that it's parent actually is a notification box, since that's only guaranteed when it's in a <tabbrowser>. You can move this out to another bug if you'd prefer.

Comment 80

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

Comment on attachment 276567 [details] [diff] [review]
Patch for review, v.2

Removing request for now until comment 79 is addressed.

Comment 81

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.3

This addresses the comments in the previous review (I hope :).

The basic approach here is to shuffle all the prompting code from nsLoginManager.js into nsLoginManagerPrompter.js. This simplifies a few things, and consolidates the UI prompting stuff into a single place. The implementation here already required a caller to have a DOM Window around for nsIAuthPrompt2.

I think the new new interfaces in nsILoginManagerPrompter should probably take nsILoginInfos instead of strings, but I wanted to minimize changes here so that the undiffable cut'n'paste didn't have more changes than necessary. I can file a followup bug for that.

It might also be possible to make use of nsIPromptService[2], as the existing nsIAuthPrompt2 code was doing, but I don't understand that code path well enough to be comfortable with using it.

Comment 82

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

Comment on attachment 278155 [details] [diff] [review]
Patch for review, v.3

>Index: toolkit/components/passwordmgr/public/nsILoginManagerPrompter.idl

>+interface nsILoginManagerPrompter : nsISupports {

>+    void promptToChangePassword(in AString aUsername);
>+
>+    void promptToChangePasswordWithUsernames(
>+            in PRUint32 count,
>+            [array, size_is(count)] in wstring usernames);

Is there any benefit to having both of these methods? You can remove the single-username one and have the implementation do what it wants based on the array length, right?

>Index: toolkit/components/passwordmgr/src/nsLoginManager.js

>+            if (logins.length == 1) {
>                 username = logins[0].username;
>-                ok = this._promptToChangePassword(win, username)
>+                ok = prompter.promptToChangePassword(username);
>             } else {
>                 var usernames = [];
>                 logins.forEach(function(l) { usernames.push(l.username); });

I should have caught this in the initial review, but this can just be:

var usernames = logins.map(function (l) l.username);

(makes use of the new "expression closures", see bug 381113 / http://developer.mozilla.org/en/docs/New_in_JavaScript_1.8#Expression_Closures )

>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

I think you should refactor the prompting/notification methods so that by default they attempt to use notification bars, but fall back to dialogs if _getOriginatingBrowserForWindow/getNotificationBox fails, rather that just hardcoding USE_NOTIFY_BAR.

>+    promptAuth : function (aChannel, aLevel, aAuthInfo) {

>+        // If the user submits a login but it fails, we need to remove the
>+        // notification bar that was displayed. Conviently, the user will be

nit: "Conveniently"

>+        // prompted for authentication again, which brings us here.
>+        // XXX this isn't right if there are multiple logins on a page (eg,
>+        // 2 images from different http realms). That seems like an edge case
>+        // that we're probably not handling right anyway.
>+        if (this.USE_NOTIFY_BAR)
>+            this._removeSaveLoginNotification();

Doesn't the "oldBar" code in _showSaveLoginNotification make this redundant?

>+    _getOriginatingBrowserForWindow : function (aWindow) {

>+        // If the login is for a window with an opener and no toolbar,
>+        // use the opener window to show the notification bar.
>+        if (!notifyWindow.toolbar.visible && notifyWindow.opener) {

I think you need to change this code to match the change made in the patch for bug 337344.

>+            this.log("Using opener window for notification bar.");
>+            notifyWindow = notifyWindow.opener;
>+            var navNav = win.QueryInterface(Ci.nsIInterfaceRequestor).
>+                             getInterface(Ci.nsIWebNavigation);
>+            var rootItem = navNav.QueryInterface(Ci.nsIDocShellTreeItem)
>+                                 .rootTreeItem;
>+            notifyWindow = rootItem.QueryInterface(Ci.nsIInterfaceRequestor).
>+                                   getInterface(Ci.nsIDOMWindow);
>+        }

I also couldn't get this code to work - testing with "javascript:window.open("http://gavinsharp.com/secret/","mywin","location=no");void(0);" gave me a "win has no properties" error. Looks like you want s/win/notifyWindow/ ?

>+        while(!browser && enumerator.hasMoreElements()) {

nit: space after "while"

Comment 83

[Justin Dolske [:Dolske]]

(In reply to comment #82)

> >+    void promptToChangePassword(in AString aUsername);
> >+
> >+    void promptToChangePasswordWithUsernames(
> >+            in PRUint32 count,
> >+            [array, size_is(count)] in wstring usernames);
> 
> Is there any benefit to having both of these methods?

They could perhaps be combined, but I don't think that helps. Although they're both dealing with a potential password change, they're use in different situations and have completely different implementations/UI. I don't feel strongly about it, but I think it's probably best to keep them separate for now.

> >+        if (this.USE_NOTIFY_BAR)
> >+            this._removeSaveLoginNotification();
> 
> Doesn't the "oldBar" code in _showSaveLoginNotification make this redundant?

Not quite. Consider the following sequence:

0) Visit HTTP Auth site, get prompted for a login
1) Enter a invalid login
2) Site returns an HTTP 401 due to bad login
3) Enter a valid login.

We create the notification bar between steps 1 and 2. We won't call _showSaveLoginNotification again until after step 3, so when you're entering a new login in step 3 (and know the old one failed), the browser is still showing the old notification bar... That's a bit confusing.

Another case is if the login you enter in step 3 is already stored in pwmgr. Since we already have it, we won't show a notification bar to save it, and so the old invalid notification bar wouldn't be removed.

Comment 84

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.4

One more time!

This should fix the comments from the last review. I also changed nsLoginManager to get the prompter with createInstance instead of getService. [Since one might have multiple instances, each initialized with a different window.]

Comment 85

[Johnathan Nightingale [:johnath]]

Dolske:

Gavin relayed a question from you about why, over in bug 337344, we needed to go with checking for chromehidden, instead of just window.toolbar.visible, since it was relevant to your work here.  There are two reasons: 

1) toolbar.visible is true even when only the location bar (and not, for instance, the buttons, search box, etc) is visible, so it might not mean what you think it means, and 
2) once we flipped the pref to disable locbar hiding (and hence made toolbar.visible *always* true thanks to #1) it stopped being a good check for popup-ness when deciding where to open new links.

I don't know if your needs are exactly the same here, but if you want to know whether a window is a chrome-reduced popup, it seems like checking for chromehidden being present-and-non-empty is the way to go.

Just glancing at the latest patch, I see you're already doing that here:

+                // Check to see if the current window was opened with
+                // chrome disabled, and if so use the opener window.
+                if (chromeDoc.getAttribute("chromehidden")) {
+                    this.log("Using opener window for notification bar.");
+                    notifyWindow = notifyWindow.opener;
+                }

But I figured I'd set it down for the record anyhow.

Comment 86

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

Comment on attachment 279059 [details] [diff] [review]
Patch for review, v.4

>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>+    _showSaveLoginDialog : function (aLogin) {

>+            this.setLoginSavingEnabled(aLogin.hostname, false);

>+            this.addLogin(aLogin.formLogin);

These need to be called on the pwmgr now that you moved this function into the prompter.

>+    _getNotifyBox : function () {
>+            // Some sites pop up a temporary login window, when disappears
>+            // upon submission of credentials. We want to put the notification
>+            // bar in the opener window if this seems to be happening.

This seems to work OK, assuming the login window does close itself. It's not quite right if the window doesn't close itself, though - e.g. one of those sites that have a fancy intro and then launch the main page in a new window. Probably the less common case, but it'd be nice if we could deal with it. Perhaps delay showing the notification, and check whether the window is closed? Followup bug material.

>+            // Return the notificationBox assiciated with the browser.

nit: associated

Comment 87

[Justin Dolske [:Dolske]]

(In reply to comment #86)

> These need to be called on the pwmgr now that you moved this function into the
> prompter.

Bah, I thought I had checked that. I double checked (again?), and didn't find any other problems like this beyond what you found.

> >+            // Return the notificationBox assiciated with the browser.
> 
> nit: associated

Ok, ok, ok... I spellchecked the rest of the pwmgr code, and corrected a few other speling mistaks. :-)

Comment 88

[Justin Dolske [:Dolske]]

Attachment: Patch as checked in.

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.834; previous revision: 1.833
done
Checking in toolkit/components/passwordmgr/public/Makefile.in;
/cvsroot/mozilla/toolkit/components/passwordmgr/public/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/passwordmgr/public/nsILoginManager.idl;
/cvsroot/mozilla/toolkit/components/passwordmgr/public/nsILoginManager.idl,v  <--  nsILoginManager.idl
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/toolkit/components/passwordmgr/public/nsILoginManagerPrompter.idl,v
done
Checking in toolkit/components/passwordmgr/public/nsILoginManagerPrompter.idl;
/cvsroot/mozilla/toolkit/components/passwordmgr/public/nsILoginManagerPrompter.idl,v  <--  nsILoginManagerPrompter.idl
initial revision: 1.1
done
Checking in toolkit/components/passwordmgr/public/nsILoginManagerStorage.idl;
/cvsroot/mozilla/toolkit/components/passwordmgr/public/nsILoginManagerStorage.idl,v  <--  nsILoginManagerStorage.idl
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js,v  <--  nsLoginManager.js
new revision: 1.15; previous revision: 1.14
done
Checking in toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js,v  <--  nsLoginManagerPrompter.js
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js,v  <--  storage-Legacy.js
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties,v  <--  passwordmgr.properties
new revision: 1.10; previous revision: 1.9
done

Comment 89

[Nelson Bolyard (seldom reads bugmail)]

Will this affect SM too?  or only FF?

Comment 90

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

(In reply to comment #89)
> Will this affect SM too?

SM currently uses a different password manager, so it won't affect SM yet. Bug 390025 tracks their moving to the implementation that was changed in this bug.

Comment 91

[Justin Dolske [:Dolske]]

Attachment: API Change; Patch for review, v.1

Nuts!

As mentioned in comment 81, I wanted to change the API so the prompts all took nsILoginInfos, instead of just username strings. In the process of looking at that I realized things were uhm, currently broken, so I had to make some slightly deeper changes to fix them.

This patch fixes promptToChangePassword and promptToChangePasswordWithUsernames. The code was also refactored a bit, as promptToSaveLogin was, so that these popups can be changed to notification bars in the future more easily.

This patch is on top of the checked-in attachment 279167 [details] [diff] [review].

Comment 92

[Justin Dolske [:Dolske]]

[Forgot to note: this can be easily tested by changing your password here on Bugzilla. Store a 2nd bogus login to test the promptToChangePasswordWithUsernames case.]

Comment 93

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

Comment on attachment 279192 [details] [diff] [review]
API Change; Patch for review, v.1

>Index: toolkit/components/passwordmgr/public/nsILoginManagerPrompter.idl

It'd be nice to have some documentation in the IDL for this interface.

>Index: toolkit/components/passwordmgr/src/nsLoginManager.js

>+                var oldLogin = logins[0];
>+                formLogin.username      = oldLogin.username;
>+                formLogin.usernameField = oldLogin.usernameField;

Should this be done in promptToChangePassword, to closer match promptToChangePasswordWithUsernames?

Comment 94

[Justin Dolske [:Dolske]]

(In reply to comment #93)

> >+                var oldLogin = logins[0];
> >+                formLogin.username      = oldLogin.username;
> >+                formLogin.usernameField = oldLogin.usernameField;
> 
> Should this be done in promptToChangePassword, to closer match
> promptToChangePasswordWithUsernames?

That would look more symmetric, but I think that gives promptToChangePassword slightly odd semantics... The WithUsernames version *must* to change the username in newLogin (because the user is selecting one from many). This version doesn't strictly need to, and I'm wary that some future caller might not want (or expect) the old username to be copied over for some reason.

Comment 95

[Justin Dolske [:Dolske]]

Checked in patch plus IDL comments.

Checking in toolkit/components/passwordmgr/public/nsILoginManagerPrompter.idl;
/cvsroot/mozilla/toolkit/components/passwordmgr/public/nsILoginManagerPrompter.idl,v  <--  nsILoginManagerPrompter.idl
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js,v  <--  nsLoginManager.js
new revision: 1.17; previous revision: 1.16
done
Checking in toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js,v  <--  nsLoginManagerPrompter.js
new revision: 1.10; previous revision: 1.9
done

Comment 96

[Tracy Walker [:tracy]]

Verified with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a8pre) Gecko/2007090504 Minefield/3.0a8pre and Windows build of the same date.

Comment 97

[Tracy Walker [:tracy]]

Although I verified this; the Post submit bar is in place.  I ran into one issue here that seems a bit confusing.  There isn't a "Not Now" choice.  So if I entered a bad password the first time logging in to say bugzilla.mozilla.org, I get the invalid password warning from the site and the remember password bar with only "Remember" (which does nothing, a good thing) and "Never for This Site"  (wait, I don't want to never remember for this site). What am I supposed to do?  

At this point I close the notification bar with the [x], then go "Back" and retry with a correct password.  But Joseephus Newbie Firefox user may not deduce that, or the site may not suggest it as bugzilla does.

Comment 98

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

(In reply to comment #97)
> I get the invalid password warning from the site and the remember password bar
> with only "Remember" (which does nothing, a good thing)

It shouldn't "do nothing", it should remember the invalid password. If it's actually doing nothing in that case, can you file a bug?

> At this point I close the notification bar with the [x], then go "Back" and
> retry with a correct password.

You can do that, or you could also just ignore it and sign in again and get a new notification (that will replace the old one).

You're right that those options might not be obvious to the user though. Can you file a followup on that?

Comment 99

[Nelson Bolyard (seldom reads bugmail)]

Comment 97 points out a design deficiency in the new bar, the inability 
to say "not this time".  
That seems like a pretty good reason to reopen  this RFE, no?

Comment 100

[(not currently active) Ted Mielczarek]

I thought the whole point of this new design was that it didn't get in your way, so you didn't *need* a "not now" button.  But that seems to be confusing, so maybe it would make sense to replace the [x] with a "Not Now" button, to make it consistent with the earlier UI?

Comment 101

[Tracy Walker [:tracy]]

I filed bug 395038 for the lack of the Not Now button and bug 395031 for the fact the current "Remember" button does nothing at all.

Comment 102

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

>I thought the whole point of this new design was that it didn't get in your
>way, so you didn't *need* a "not now" button.

Yes, "not now" was a function of the earlier design being a modal dialog.  The user can now achieve "not now" by either ignoring the bar, or dismissing it.  When (if) this design is transitioned to the hanging notification design, a mouse click anywhere outside of the notification will dismiss it, making "not now" even easier to achieve.

Comment 103

[Shawn Wilsher :sdwilsh]

For what it is worth, when I've not dismissed it myself, the bar has not gone away.

Comment 104

[Justin Dolske [:Dolske]]

The bar goes only away by itself when you navigate to another page (after an initial short period where it ignores location changes, due to redirects being hard to handle). 

Comment 105

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

(In reply to comment #99)
> Comment 97 points out a design deficiency in the new bar, the inability 
> to say "not this time".  
> That seems like a pretty good reason to reopen  this RFE, no?

"Design deficiencies" are never good reasons to reopen fixed bugs. Unless the patch was backed out, or needs to be backed out, or doesn't fix the problem, then it's always better to file a new bug.

Comment 106

[Carsten Book [:Tomcat]]

A test case in litmus will cover the post-submit bar in the Litmus Password Manager Test Suite -> in Litmus +

Comment 107

[u294409]

I would like to see modal dialogs back, because I don't want to move my fat mouse cursors especially to confirm password save.

I beloved my Return key and I would like to use it to save passwords.
Is it possible?

Comment 108

[[e-mail]]

Comment on attachment 279167 [details] [diff] [review]
Patch as checked in.

>-        gBrowser.getNotificationBox(selectedBrowser).removeAllNotifications(true);
>+        // Remove all the notifications, except for those which want to
>+        // persist across the first location change.
>+        var nBox = gBrowser.getNotificationBox(selectedBrowser);
>+        for (var n = nBox.allNotifications.length - 1; n >= 0; n--) {
>+          var notify = nBox.allNotifications[n];
>+          if (notify.ignoreFirstLocationChange)
>+            notify.ignoreFirstLocationChange = false;
>+          else if (!notify.ignoreLocationChangeTimeout ||
>+            (Date.now() / 1000) > notify.ignoreLocationChangeTimeout)
>+            nBox.removeNotification(notify);
>+        }
notificationbox lives in toolkit... so why is this new API in browser? Sigh.

Comment 109

[u294409]

Is there a Return-key-friendly way to do this? I don't like to move mouse and especially for one password click "Remember"...