The default bug view has changed. See this FAQ.

"Remember passwords" check box is broken when no history - in Private Browsing mode or when Firefox set to never remember any history

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Preferences
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: u60234, Assigned: Javi Rueda)

Tracking

({useless-UI})

Trunk
Firefox 17
useless-UI
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

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

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

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

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

8 years ago
I have this problem too, it's a shame, Firefox 3.5 is so fast!
Summary: "Remember passwords" check box is broken when no history is remembered → "Remember passwords" check box is broken when no history - in Private Browsing mode or when Firefox set to never remember any history

Comment 5

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

Updated

5 years ago
Assignee: nobody → leofigueres
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

5 years ago
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.
Attachment #585722 - Attachment is obsolete: true
Attachment #611542 - Flags: review?(dao)
(Assignee)

Comment 9

5 years ago
(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 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).
Attachment #611542 - Flags: review?(dao) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 615304 [details] [diff] [review]
Accessing the preferences programmatically
Attachment #611542 - Attachment is obsolete: true
Attachment #615304 - Flags: review?(dao)
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;
(Assignee)

Comment 13

5 years ago
Created attachment 615346 [details] [diff] [review]
patch with styling changes
Attachment #615304 - Attachment is obsolete: true
Attachment #615346 - Flags: review?(dao)
Attachment #615304 - Flags: review?(dao)
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.
(Assignee)

Comment 15

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

Comment 16

5 years ago
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.
Attachment #616666 - Flags: ui-review?(ux-review)

Comment 17

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

Comment 18

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

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

Comment 20

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

Comment 21

5 years ago
Comment on attachment 615346 [details] [diff] [review]
patch with styling changes

Cancelling reviews as the code is being re-done. Sorry for the spam.
Attachment #615346 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #616666 - Flags: ui-review?(ux-review)
(Assignee)

Comment 22

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

Comment 23

5 years ago
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.
Attachment #616666 - Attachment is obsolete: true
Attachment #618314 - Flags: ui-review?(ux-review)

Comment 24

5 years ago
Comment on attachment 618314 [details]
Screenshot with the checkbox unselected and disabled.

Good work.
(Assignee)

Comment 25

5 years ago
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.
Attachment #615346 - Attachment is obsolete: true
Attachment #619143 - Flags: review?(dao)
Comment on attachment 618314 [details]
Screenshot with the checkbox unselected and disabled.

WFM!
Attachment #618314 - Flags: ui-review?(ux-review) → ui-review+
(Assignee)

Comment 27

5 years ago
ping :-)

The last patch is ready to be reviewed now, Dao.
(Assignee)

Comment 28

5 years ago
Created attachment 639074 [details] [diff] [review]
Line-width correction

One of the lines in last patch exceded the 80 chars length limit.
Attachment #619143 - Attachment is obsolete: true
Attachment #619143 - Flags: review?(dao)
Attachment #639074 - Flags: review?(dao)
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.
Attachment #639074 - Flags: review?(dao) → review?(ehsan)
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.
Attachment #639074 - Flags: review?(ehsan) → review-
(Assignee)

Comment 31

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

Comment 33

5 years ago
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.
Attachment #639074 - Attachment is obsolete: true
Attachment #651326 - Flags: review?(ehsan)
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 {
Attachment #651326 - Flags: review?(ehsan)
(Assignee)

Comment 35

5 years ago
Created attachment 653210 [details] [diff] [review]
Same changes are done for in-content settings
Attachment #651326 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
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.
Attachment #653210 - Flags: review?(ehsan)
Comment on attachment 653210 [details] [diff] [review]
Same changes are done for in-content settings

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

Looks great!
Attachment #653210 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa2e09c092e
Target Milestone: --- → Firefox 17
(Assignee)

Comment 39

5 years ago
Thank you for the check-in, Ehsan ^-^
https://hg.mozilla.org/mozilla-central/rev/0fa2e09c092e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.