408363 - Password Manager remember prompt gets dismissed too early

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[(no longer active)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1

Steps to reproduce:

1. Go to <http://www.editorialmanager.com/dapd/>.
2. Select Login from the top menu.
3. Enter any username and password, and hit Author Login.  The remember password bar appears.
4. Don't do anything, and wait to be redirected to <http://www.editorialmanager.com/dapd/default.asp?pg=login.asp%3FloginError%3D1>.

As soon as the redirection occurs, the remember password bar disappears, without allowing the user to see if her login was successful or not before attempting to save the password, thereby voiding the rationale behind the new bar.

Expected result: the bar should not be dismissed upon page redirect.

Comment 1

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

The behavior that we settled on in bug 226735 was to ignore the first location change after showing the notification bar, and all other locations until 10 seconds after the sign in. Does your test site wait more than 10 seconds to redirect you? Is that normal behavior? Not sure we want to fix this.

Comment 2

[(no longer active)]

(In reply to comment #1)
> The behavior that we settled on in bug 226735 was to ignore the first location
> change after showing the notification bar, and all other locations until 10
> seconds after the sign in. Does your test site wait more than 10 seconds to
> redirect you? Is that normal behavior? Not sure we want to fix this.
> 

Well, 10 seconds is too little on slow connections...  We intend to make the browser usable both on broad-band and dial-up, right?

What is wrong with just letting the notification bar sit there for the whole period of the page being loaded, and remove it 10 seconds after the page has finished loading, or for example when a connection timeout error occurs?

Comment 3

[Justin Dolske [:Dolske]]

I looked at handling redirects in bug 226735 comment 62, and it was looking very ugly. The timeout isn't perfect, but it was simple.(In reply to comment #2)

Comment 4

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

Not blocking on the assumption that this is the 10s timeout thing as per comment 1 and comment 3.

Ehsan: are you sure that it's the 10s thing that's causing this? Can you always reproduce as you say in comment 0 where the bar disappears at the exact same time as the redirect? If so, please re-nom.

Comment 5

[(no longer active)]

(In reply to comment #4)
> Not blocking on the assumption that this is the 10s timeout thing as per
> comment 1 and comment 3.
> 
> Ehsan: are you sure that it's the 10s thing that's causing this? Can you always
> reproduce as you say in comment 0 where the bar disappears at the exact same
> time as the redirect? If so, please re-nom.

I'm currently on a fast connection, but my further testing shows that this behavior is caused by the 10 seconds timeout, which doesn't look too good on dial-up connections.  I see this problem all the time on dial-up.  Other logins, such as gmail's, suffers from the same problem as well.

Could the 10-second timeout at least be increased?

Comment 6

[(no longer active)]

What's the harm in just leaving the notification bar open during all the redirects?  That's what happens if the redirects finish sooner than 10 seconds and the page loads: the notification bar just sits there waiting for an action from the user.  Isn't it?

Comment 7

[Justin Dolske [:Dolske]]

The "did the location change?" code doesn't know why the location changed (HTTP redirect, user click, typed URL, javascript, etc). So if we don't dismiss it at some location change, it would persist through your entire browsing session (unless you manually close it, which is annoying).

The 10 second value is fairly arbitrary. Would 15 or 20 be better? Not sure if I'd want to go above that.

Comment 8

[(no longer active)]

Attachment: Bump the timeout value from 10 to 20 seconds

(In reply to comment #7)
> The "did the location change?" code doesn't know why the location changed (HTTP
> redirect, user click, typed URL, javascript, etc). So if we don't dismiss it at
> some location change, it would persist through your entire browsing session
> (unless you manually close it, which is annoying).

That's too unfortunate, so there *has* to be a timeout, right?

By the way, is it possible to change the code responsible for handling location changes in a way to make it more smart about the source of the location change events?  Is there a bug filed on that?

> The 10 second value is fairly arbitrary. Would 15 or 20 be better? Not sure if
> I'd want to go above that.

I think 20 seconds is a better choice, since it's close to the patience of many dial-up users for a page to load before getting tired and canceling the process.  Based on my experience, that is.

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 293636 [details] [diff] [review]
Bump the timeout value from 10 to 20 seconds

I can live with this. I don't think the comment about why the timeout is bumped is needed, though.

Comment 10

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

Comment on attachment 293636 [details] [diff] [review]
Bump the timeout value from 10 to 20 seconds

Agreed with dolske on the comment being unnecessary.

Comment 11

[(no longer active)]

Beltzner: waiting for your call on this.

Comment 12

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

Comment on attachment 293636 [details] [diff] [review]
Bump the timeout value from 10 to 20 seconds

uir+a=beltzner

Comment 13

[(no longer active)]

Attachment: Patch for checkin

This patch is the same as attachment 293636 [details] [diff] [review], with the comment removed as per comments 9 and 10.

Comment 14

[(no longer active)]

Requesting check-in of attachment 299960 [details] [diff] [review]...

Comment 15

[Reed Loden [:reed]]

Checking in toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js,v  <--  nsLoginManagerPrompter.js
new revision: 1.18; previous revision: 1.17
done

Comment 16

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

Deb's commented that this makes it somewhat hard to predict when the notification will go away vs. when it won't. We might want to change the logic to be more based on user input (ie: when the user clicks vs. automatically refreshing) or crank down the delay a little.

That'd be a new bug, though. :)