595175 - "Change password?" notification persists when leaving the site

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

The "Change password?" notification persists when leaving the site.

STR:
- Open a site which you've stored a password for
- Try to log in with the correct name but wrong password
- Navigate to a different site
- Click the key icon in the location bar. A panel appears, asking you "Would you like to change the stored password for [login name]?"

Comment 1

[Dão Gottwald [:dao]]

(In reply to comment #0)
> - Click the key icon in the location bar. A panel appears, asking you "Would
> you like to change the stored password for [login name]?"

Note that this doesn't actually say which site the password would be changed for.

Comment 2

[:Margaret Leibovic]

This behavior is the same with the notification bar, but I guess that's more acceptable because you can actually dismiss the notification with "Don't Change" or the "x" on the bar. When do we want the notification to go away? Do we want the notification to go away when the user navigates to a different page or a different site?

As a side note, we're working on changing the strings for these notifications in bug 594572, so we could add the site to the notification message. However, that doesn't solve the problem of the notification hanging around indefinitely.

Comment 3

[:Margaret Leibovic]

I think this is also going to be a problem with the remember password notifications.

Comment 4

[:Margaret Leibovic]

Attachment: patch

The notifications hang around because there's a timeout set on them to deal with messy authentication redirects. This patch makes the timeout period shorter (and now matches the timeout for the notification bars). I don't know if there's any way to really solve this problem, but putting in text about which site the notification is for would definitely help reduce confusion.

Comment 5

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

I think we should add the hostname to the string, yeah.

Comment 6

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

Adding host name to the string still breaks the interface, the anchors are visually associated with the site that identity.  If the domain name changes, we should dismiss the notification and the anchor.

Comment 7

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

Having siteA.com produce the message: "do you want to change your password for siteB.com" isn't really an acceptable solution to this UI problem, at best the user will simply be confused.

Comment 8

[:Margaret Leibovic]

(In reply to comment #6)
> Adding host name to the string still breaks the interface, the anchors are
> visually associated with the site that identity.  If the domain name changes,
> we should dismiss the notification and the anchor.

I'm not sure if there is a way to ensure this behavior because of authentication redirects. Gavin, do you have any ideas?

Comment 9

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

Moving this to betaN+, since the solution here likely won't involve changing strings after all.

There are a couple of things we could do here; we could for example pass a "host changed" flag to PopupNotification.locationChange(), to allow it to distinguish loads from the same host and loads to a different host (or "site"), and then dismiss the notification only in the latter case. We kind of run into the same issue as in bug 575561, though - some login processes might legitimately involve redirects that involve various hosts (or even various eTLD+1s).

Comment 10

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

Comment on attachment 474136 [details] [diff] [review]
patch

Wouldn't hurt to get this in to beta 6, even if we do want to follow up with a more complicated fix.

Comment 11

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

(I mean beta 7)

Comment 12

[:Margaret Leibovic]

I can land it today with the string changes from bug 594572.

Comment 13

[:Margaret Leibovic]

(In reply to comment #12)
> I can land it today with the string changes from bug 594572.

http://hg.mozilla.org/mozilla-central/rev/6095c6b1f50f

Comment 14

[Tomas]

This bug shuld be marked fixed?

Comment 15

[:Margaret Leibovic]

I don't think we want to mark it fixed until we implement the change gavin suggested in comment 9. The patch I pushed is just a temporary (but not very real) fix.

Comment 16

[:Margaret Leibovic]

Attachment: smarter patch v1

Comment 17

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

Comment on attachment 477318 [details] [diff] [review]
smarter patch v1

Playing around with this some, I think 5 seconds may be a bit short... perhaps 10 seconds would be better. Would be also good to get dolske's sign-off on the approach.

I think "persistWhileVisible" is a better name for the flag than "persistVisiblePanel". Should adjust the comment to something like: "If true, location changes will not cause the notification to be removed while it is visible."

I need to do a pass over the documentation, it's rather inconsistent with its use of dismissed/removed and activated/visible :(

We should get a test for this behavior. Something along the lines of test 12/13 in browser_popupNotification.js.

r=me with these addressed.

Comment 18

[:Margaret Leibovic]

(In reply to comment #17)
> We should get a test for this behavior. Something along the lines of test 12/13
> in browser_popupNotification.js.

Should I add a new test to that file, or make a new test file for the bug?

Comment 19

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

Whatever's easiest - probably adding to that file.

Comment 20

[:Margaret Leibovic]

Attachment: patch v1.1 (with test)

Addressed review comments.

Comment 21

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

Comment on attachment 478802 [details] [diff] [review]
patch v1.1 (with test)

>diff --git a/browser/base/content/test/browser_popupNotification.js b/browser/base/content/test/browser_popupNotification.js

>+    onShown: function (popup) {
>+      this.complete = false;
>+
>+      let self = this;
>+      loadURI("http://example.org/", function() {
>+        loadURI("http://example.com/", function() {
>+
>+          // Next load will hide the notification
>+          dismissNotification(popup);
>+          self.complete = true;
>+
>+          loadURI("http://example.org/");

What's the point of this load? The notification is already going to be dismissed, and you don't seem to check the result in any way...

Comment 22

[:Margaret Leibovic]

(In reply to comment #21)
> Comment on attachment 478802 [details] [diff] [review]
> patch v1.1 (with test)
> 
> >diff --git a/browser/base/content/test/browser_popupNotification.js b/browser/base/content/test/browser_popupNotification.js
> 
> >+    onShown: function (popup) {
> >+      this.complete = false;
> >+
> >+      let self = this;
> >+      loadURI("http://example.org/", function() {
> >+        loadURI("http://example.com/", function() {
> >+
> >+          // Next load will hide the notification
> >+          dismissNotification(popup);
> >+          self.complete = true;
> >+
> >+          loadURI("http://example.org/");
> 
> What's the point of this load? The notification is already going to be
> dismissed, and you don't seem to check the result in any way...

Hmm, I guess that's just a mistake from copy/pasting the previous test. The comment in there is also wrong, so I'll fix that :) Also, shouldn't the self.complete = true be above the dismissNotification if the dismissNotification triggers the hidden event? This test passed when I ran it, but it doesn't make sense now that I'm looking at it again.

Comment 23

[Justin Dolske [:Dolske]]

(In reply to comment #17)

> Playing around with this some, I think 5 seconds may be a bit short... perhaps
> 10 seconds would be better.

Just curious, why?

Comment 24

[Justin Dolske [:Dolske]]

Comment on attachment 478802 [details] [diff] [review]
patch v1.1 (with test)

>   locationChange: function PopupNotifications_locationChange() {
>     this._currentNotifications = this._currentNotifications.filter(function(notification) {
>+      // The persistWhileVisible option allows an open notification to persist
>+      // across location changes
>+      if (notification.options.persistWhileVisible &&
>+          this.isPanelOpen) {
>+        return true;
>+      }
>+      
>       // The persistence option allows a notification to persist across multiple
>       // page loads
>       if ("persistence" in notification.options &&
>           notification.options.persistence) {
>         notification.options.persistence--;
>         return true;
>       }

Hmm, if the persistWhileVisible case is taken, then |persistence| will not be decremented. Seems like it probably should be, if for no other reason than to avoid having to add an explanation to the documentation. :)

Anyway, I'm Justin Dolske and I approve of this patch.

Comment 25

[:Margaret Leibovic]

Attachment: patch v1.2

Fixed test and addressed dolske's comment.

Comment 26

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/002f8bd6526e

I accidentally put dolske instead of gavin as the reviewer in the commit message, but I assume that's not a big deal because Justin Dolske did declare his approval :)