Last Comment Bug 503761 - "Remember passwords" check box is broken when no history - in Private Browsing mode or when Firefox set to never remember any history
: "Remember passwords" check box is broken when no history - in Private Browsin...
Status: RESOLVED FIXED
: useless-UI
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Javi Rueda
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-12 10:05 PDT by Hasse
Modified: 2012-08-21 19:09 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 WIP (2.88 KB, patch)
2012-01-04 06:03 PST, Javi Rueda
no flags Details | Diff | Review
patch (2.85 KB, patch)
2012-04-02 11:57 PDT, Javi Rueda
dao+bmo: review-
Details | Diff | Review
Accessing the preferences programmatically (1.82 KB, patch)
2012-04-16 04:53 PDT, Javi Rueda
no flags Details | Diff | Review
patch with styling changes (1.83 KB, patch)
2012-04-16 08:47 PDT, Javi Rueda
no flags Details | Diff | Review
Check-box disabled in Private Browsing mode (100.18 KB, image/png)
2012-04-19 11:37 PDT, Javi Rueda
no flags Details
Screenshot with the checkbox unselected and disabled. (87.95 KB, image/png)
2012-04-25 09:06 PDT, Javi Rueda
limi: ui‑review+
Details
patch (2.24 KB, patch)
2012-04-27 12:38 PDT, Javi Rueda
no flags Details | Diff | Review
Line-width correction (2.29 KB, patch)
2012-07-04 06:14 PDT, Javi Rueda
ehsan: review-
Details | Diff | Review
patch (1.80 KB, patch)
2012-08-13 04:51 PDT, Javi Rueda
no flags Details | Diff | Review
Same changes are done for in-content settings (3.31 KB, patch)
2012-08-19 15:46 PDT, Javi Rueda
ehsan: review+
Details | Diff | Review

Description Hasse 2009-07-12 10:05:54 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.9.2a1pre) Gecko/20090712 Minefield/3.6a1pre

In Private Browsing mode, or when Firefox is set to never remember any history, no new passwords will be saved. But the "Remember passwords for sites" check box in the Security panel is nevertheless enabled. You can check and uncheck it, as if you had a choice whether passwords should be saved or not.

Steps to reproduce:
1. Start a private browsing session, or set Firefox to never remember any history.
2. Go to Options/Preferences -> Security panel.
3. Make sure the "Remember passwords for sites" option is checked.
4. Visit a site that require login, and fill in your information. If you already have saved a password for the site, fill in a new password to trigger the "change existing password" prompt.

Result:
No prompt is displayed where Firefox offer to save the new login information
Comment 1 THEBAUS 2009-08-30 15:54:31 PDT
Yeah that's the same problem I'm having. How can I get it to save my passwords like it did with the previous version?

-THEBAUS
Comment 2 so 2009-09-18 13:40:48 PDT
I too have been having this problem, I have an add-on installed for exporting and importing saved usernames/password combos, I thought this problem was attributed to that but I disabled it and still have the same problem. Is it possible to downgrade to an earlier version of firefox?
Comment 3 so 2009-09-18 13:43:50 PDT
I too have been having this problem, I have an add-on installed for exporting and importing saved usernames/password combos, I thought this problem was attributed to that but I disabled it and still have the same problem. Is it possible to downgrade to an earlier version of firefox?
Comment 4 romain_yoda 2009-10-17 01:03:13 PDT
I have this problem too, it's a shame, Firefox 3.5 is so fast!
Comment 5 Gavin Stokes 2011-11-09 12:11:09 PST
I don't see this on one of my systems.  But on two out of two systems with freshly installed OS X Lion, Firefox never remembers any credentials.  I do have "don't remember history" on, but I have that on the system that works, too.

This has been through versions 7 and 8.  Potentially relevant bug: 517383
Comment 6 Javi Rueda 2011-12-14 12:15:14 PST
Isn't "Never remember" the same as "Private browsing" from a practical viewpoint?

Browsing the source code, the history modes aren't real modes but sets of preferences. Depending of the value of this preferences, the history mode is said to be one of the three available.
Comment 7 Javi Rueda 2012-01-04 06:03:19 PST
Created attachment 585722 [details] [diff] [review]
patch 1 WIP

Work in progress.

The Private Browsing session detection code works, but the "Never remember" detection only works when it was selected when opening the dialog box (this is probably caused by the way it the settings take effect in some platforms).

Not my final proposed code, so I am not flagging it to be reviewed.
Comment 8 Javi Rueda 2012-04-02 11:57:09 PDT
Created attachment 611542 [details] [diff] [review]
patch

Same as previous Work-In-Progress attachment, but with a simpler source code.

To disable (make not clickable) the "save passwords" checkbox when it is useless, this new code checks if the browser is in a Private mode session right now or if user has setup the browser to "not remember history" in the Privacy Pane. This last action, in fact activates the Private mode for the session, and instructs the browser to "start in Private mode" next time it is executed.

If one of those conditions are true, then the check-box and its related button for exceptions will be disabled for user interaction.

Dao, could you take a look at this patch, please? Thank you.

As noted in previous comments, it would be better if options dialog updates instantly the preferences values.

Also, I run mochitests and get as a result 13 or 16 failed tests. I didn't find any related to the issue of this particular bug.
Comment 9 Javi Rueda 2012-04-02 12:26:29 PDT
(In reply to Gavin Stokes from comment #5)
> I don't see this on one of my systems.  But on two out of two systems with
> freshly installed OS X Lion, Firefox never remembers any credentials.  I do
> have "don't remember history" on, but I have that on the system that works,
> too.
> 
> This has been through versions 7 and 8.  Potentially relevant bug: 517383

As discussed in bug 517383 you filed, this new problem is about the UI not making it clear that when the Private mode is activated, then the passwords won't be remembered. The interface shows a sentence where it says that selecting the "not remember history" option in the pop-up will, in fact, made the browser acts as if it was in Private mode.

Then: these comments are refering to another issue, not the one stated at the bug header.
Comment 10 Dão Gottwald [:dao] 2012-04-12 11:58:15 PDT
Comment on attachment 611542 [details] [diff] [review]
patch

>+    if (document.getElementById("browser.privatebrowsing.autostart").value
>+        || inPrivateBrowsingMode) {
>+      var passwords = document.getElementById("savePasswords");
>+      passwords.disabled = true;
>+      document.getElementById("passwordExceptions").disabled = true;
>+    }

>+      <preference id="browser.privatebrowsing.autostart"
>+                  name="browser.privatebrowsing.autostart"
>+                  type="bool"/>

You don't need the <preference> element, as far as I can see. You can just read the pref using the preferences service (Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch).getBoolPref).
Comment 11 Javi Rueda 2012-04-16 04:53:32 PDT
Created attachment 615304 [details] [diff] [review]
Accessing the preferences programmatically
Comment 12 Dão Gottwald [:dao] 2012-04-16 07:12:17 PDT
Comment on attachment 615304 [details] [diff] [review]
Accessing the preferences programmatically

>+    var pbs = Components.classes["@mozilla.org/privatebrowsing;1"]  
>+                        .getService(Components.interfaces.nsIPrivateBrowsingService);  
>+    var inPrivateBrowsingMode = pbs.privateBrowsingEnabled;  

Remove trailing spaces

>+    var autoStartPref = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch).getBoolPref("browser.privatebrowsing.autostart");

This line is too long, please format it like this:

var autoStartPref = Components.classes["@mozilla.org/preferences-service;1"]
                              .getService(Components.interfaces.nsIPrefBranch)
                              .getBoolPref("browser.privatebrowsing.autostart");

>+      var passwords = document.getElementById("savePasswords");
>+      passwords.disabled = true;

document.getElementById("savePasswords").disabled = true;
Comment 13 Javi Rueda 2012-04-16 08:47:33 PDT
Created attachment 615346 [details] [diff] [review]
patch with styling changes
Comment 14 Dão Gottwald [:dao] 2012-04-18 06:25:01 PDT
Comment on attachment 615346 [details] [diff] [review]
patch with styling changes

>+    // When in Private Browsing mode or when 'Don't remember' history
>+    // is selected in the Privacy panel, the "Remember passwords" is useless.
>+    var pbs = Components.classes["@mozilla.org/privatebrowsing;1"]
>+                        .getService(Components.interfaces.nsIPrivateBrowsingService);
>+    var inPrivateBrowsingMode = pbs.privateBrowsingEnabled;
>+    var autoStartPref = Components.classes["@mozilla.org/preferences-service;1"]
>+                        .getService(Components.interfaces.nsIPrefBranch)
>+                        .getBoolPref("browser.privatebrowsing.autostart");
>+
>+    // The 'Don't remember history' mode is linked with the 'Private Browsing
>+    // when start' feature.
>+    if (autoStartPref || inPrivateBrowsingMode) {

I'm confused -- what exactly represents the "Don't remember history" setting here? browser.privatebrowsing.autostart is something different, as I understand it.
Comment 15 Javi Rueda 2012-04-18 07:29:36 PDT
In privacy.js, function updateHistoryModePrefs, when the History mode selected item is "Don't remember", "browser.privatebrowsing.autostart" preference is set to true. Then, it seems to me that the way we could know if browser is in the "Don't remember mode" is to check that preference.

The other case is when "Remember history" is selected. Then, at the same code in privacy.js, the autostart preference is set to false.

In comment 0 it is said "in private browsing mode or when Firefox is set to never remember any history". For the first part, the patch queries the Private Browsing Service.

For the second part, it would be great to be able to know the value of the privacy.xul history mode list box, but I wasn't able to do this.

If there is no other way to do it, maybe it would be better to clarify the comments on the patch?

I hope this answers your question, Dao :-)
Comment 16 Javi Rueda 2012-04-19 11:37:00 PDT
Created attachment 616666 [details]
Check-box disabled in Private Browsing mode

This screenshot was taken when in Private Browsing mode and was built with attachment 615345 [details] [diff] [review] included.

When in this mode, the checkbox for the password preference is useless, so it is disabled.

I am asking for a review because maybe showing it could make the user think "Passwords will be remembered and I am not able to change this behavior".

Maybe another ways to fix this bug are:

a)hide both the checkbox, label and button

b)remove the checkbox and the button and showing instead a label stating something like "No new passwords are remembered while in Private Browsing or when 'Never remember history' is selected".

c)following with last patch, but add the explanation label in option b.

Sorry, because, as this is an UI related bug this comment should have been done some time ago.
Comment 17 Gavin Stokes 2012-04-19 11:50:02 PDT
"maybe showing it could make the user think "Passwords will be remembered and I am not able to change this behavior"."

Yes, that is what that screen shot would indicate to the user.  If passwords are not remembered in Private Browsing mode, why does the checkbox have a check in it?

Thanks for your work on this.
Comment 18 Javi Rueda 2012-04-19 12:46:21 PDT
Dao: I am not cancelling the review of the patch. Waiting for the review from the UX-team, but after the comment from Gavin Stokes, it looks that this approach is not quite good.

Gavin: there would be a fourth possibility. It would be unchecking the checkbox and then disabling both the button and the checkbox itself. As the checkbox is linked with the signon.rememberSignons preference, unchecking it would switch it to a "false" state when the session is re-established to the Normal Browsing mode (don't know if this is its name, in fact).

I didn't mention it before, but the same will happen when the 'Never remember history' item is selected in the Privacy pane.
Comment 19 Gavin Stokes 2012-04-19 12:53:37 PDT
To work around the problem of the unchecked state being retained when Normal mode is reactivated, would it be possible to overlay the real checkbox with a "fake" unchecked one?  The fake one could safely be hidden when Normal mode is re-established, allowing the real state to show again.

I'm assuming there's some reason you can't just save the previous value of the checkbox and restore it when Normal mode resumes.  Is that true?
Comment 20 Javi Rueda 2012-04-19 13:20:53 PDT
(In reply to Gavin Stokes from comment #19)
> I'm assuming there's some reason you can't just save the previous value of
> the checkbox and restore it when Normal mode resumes.  Is that true?

Yes, Gavin. I thought about adding a new preference that would contain the value of the checkbox before the browser switches to Private Mode, but I wasn't sure if it would be too much, you know, a new preference only because of this bug. Better try to fix this without touching the Private Mode code
Comment 21 Javi Rueda 2012-04-24 11:58:43 PDT
Comment on attachment 615346 [details] [diff] [review]
patch with styling changes

Cancelling reviews as the code is being re-done. Sorry for the spam.
Comment 22 Javi Rueda 2012-04-24 12:13:28 PDT
I will try it by moving the ode in the last patch to the readSavePasswords. With this change we could modify the UI without updating the preference value.

Once I had a running build a screenshot will be posted so the ux-team could review it, and if plused, the patch itself will follow :-)
Comment 23 Javi Rueda 2012-04-25 09:06:52 PDT
Created attachment 618314 [details]
Screenshot with the checkbox unselected and disabled.

Don't know if it is still needed to ask for a review from the UX tem, but... better to be sure. Also maybe they could provide us with another way to do things visually.
Comment 24 Gavin Stokes 2012-04-25 10:49:35 PDT
Comment on attachment 618314 [details]
Screenshot with the checkbox unselected and disabled.

Good work.
Comment 25 Javi Rueda 2012-04-27 12:38:50 PDT
Created attachment 619143 [details] [diff] [review]
patch

Disables both the checkbox and the Exceptions button in case browser is in Private Browsing mode or in the Never remember history.

In order to keep consistency visually, the checkbox is shown as not checked, but the preference value remains unchanged.

Note to the reviewer: I tested the functionality and also executed mozilla-browser-chrome mochitest suite. In the suite I normally get between 12 or 19 failed tests -difference is because of timed-out ones- in this and other bugs I try to fix in previous versions. Anyway, it would be fine, if source style and the code itself is ok, to run the suite to be sure of everything still ok. Thank you.
Comment 26 Alex Limi (:limi) — Firefox UX Team 2012-05-09 16:28:07 PDT
Comment on attachment 618314 [details]
Screenshot with the checkbox unselected and disabled.

WFM!
Comment 27 Javi Rueda 2012-05-23 05:20:43 PDT
ping :-)

The last patch is ready to be reviewed now, Dao.
Comment 28 Javi Rueda 2012-07-04 06:14:48 PDT
Created attachment 639074 [details] [diff] [review]
Line-width correction

One of the lines in last patch exceded the 80 chars length limit.
Comment 29 Dão Gottwald [:dao] 2012-07-18 08:12:46 PDT
Comment on attachment 639074 [details] [diff] [review]
Line-width correction

nsIPrivateBrowsingService::privateBrowsingEnabled is going away (bug 463027). I'm not sure how to deal with this here.
Comment 30 :Ehsan Akhgari (out sick) 2012-07-18 08:34:57 PDT
Comment on attachment 639074 [details] [diff] [review]
Line-width correction

Review of attachment 639074 [details] [diff] [review]:
-----------------------------------------------------------------

Firstly, this should not depend on the current private browsing status, as this UI is about setting Firefox preferences, and should not be affected by temporary changes in the PB status.  But it does make sense to make this disabled in auto-started PB mode.  However, the correct way of checking this is to use the autoStarted property of the PB service <http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIPrivateBrowsingService.idl#17>, not examining the value of the pref directly.
Comment 31 Javi Rueda 2012-07-18 11:27:29 PDT
Done the change in order to read the private browsing service member, not the preference value itself.

Then, if the UI is available while in a private browsing mode session -not autostarted with the application-, this could confuse the user.

Also, there is comment 29.

Go just with a patch only for half the description in comment 0?
Comment 32 :Ehsan Akhgari (out sick) 2012-07-18 11:44:29 PDT
(In reply to Javi Rueda from comment #31)
> Done the change in order to read the private browsing service member, not
> the preference value itself.

Good.

> Then, if the UI is available while in a private browsing mode session -not
> autostarted with the application-, this could confuse the user.

I think that's fine...

> Also, there is comment 29.

We're not planning to remove the autoStarted property for per-window PB, only the privateBrowsing member.

> Go just with a patch only for half the description in comment 0?

Yes please.
Comment 33 Javi Rueda 2012-08-13 04:51:05 PDT
Created attachment 651326 [details] [diff] [review]
patch

This patch only disables the "Remember passwords" checkbox and the "Exceptions" button when browser is instructed to "Never remember history" in the Privacy panel.
Comment 34 :Ehsan Akhgari (out sick) 2012-08-13 10:27:41 PDT
Comment on attachment 651326 [details] [diff] [review]
patch

Review of attachment 651326 [details] [diff] [review]:
-----------------------------------------------------------------

This looks generally good.  Please fix the nits below, and also make sure that you make the same changes to this file too: <http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/security.js>.

::: browser/components/preferences/security.js
@@ +91,5 @@
>      var excepts = document.getElementById("passwordExceptions");
>  
> +    var pbs = Components.classes["@mozilla.org/privatebrowsing;1"]
> +                        .getService(Components.interfaces
> +                                              .nsIPrivateBrowsingService);

Nit: please define Cc and Ci as used elsewhere in this file (like _removeMasterPassword) here too, and use them to access the PB service.  This will make this code a bit more readable.

@@ +98,5 @@
> +      document.getElementById("savePasswords").disabled = true;
> +      excepts.disabled = true;
> +      return false;
> +    }
> +    else {

Nit: } else {
Comment 35 Javi Rueda 2012-08-19 15:46:26 PDT
Created attachment 653210 [details] [diff] [review]
Same changes are done for in-content settings
Comment 36 Javi Rueda 2012-08-19 15:48:13 PDT
Comment on attachment 653210 [details] [diff] [review]
Same changes are done for in-content settings

I've done needed changes to previous patch. Could you take a look to this new one? Thank you.
Comment 37 :Ehsan Akhgari (out sick) 2012-08-21 08:03:52 PDT
Comment on attachment 653210 [details] [diff] [review]
Same changes are done for in-content settings

Review of attachment 653210 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!
Comment 38 :Ehsan Akhgari (out sick) 2012-08-21 08:04:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa2e09c092e
Comment 39 Javi Rueda 2012-08-21 09:03:31 PDT
Thank you for the check-in, Ehsan ^-^
Comment 40 Ryan VanderMeulen [:RyanVM] 2012-08-21 19:09:56 PDT
https://hg.mozilla.org/mozilla-central/rev/0fa2e09c092e

Note You need to log in before you can comment on or make changes to this bug.