490199 - Selected option in new privacy pane not sticky when switching from never remember to customized view

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090423 Shiretoko/3.5b4pre ID:20090423163827

The option selected by the user is not sticky in the new privacy pane when you do the following steps:

1. Open preferences and select privacy pane
2. Select "never remember history" and close the dialog
3. Open it again
4. Select "use custom settings for history" and close it again
5. Open it again
6. Set back to custom settings and change the cookie settings
7. Close and reopen again

After step 5 and 7 the selected option has been reverted to "never remember history".

Comment 1

[klonos]

Confirming this on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090424 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090424041640 as well.

Just a note (step 8 perhaps): Set back to custom settings once again... the cookies settings seem to be remembered nevertheless.

Comment 2

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

Seems like this is a pretty easy fix: I bet we're taking a shorthand for showing "Never remember history" and we shouldn't take that shorthand when the cookie settings are changed.

I don't think we need to make the setting sticky based on what the user last selected; inferring "Never remember history" if all settings are default and the pref for always starting in private browsing is set is the right thing to do. However, if the cookie preferences aren't set to defaults, we need to show the dialog in the "Custom settings" state.

Comment 3

[Drew Willcoxon :adw]

Attachment: patch v1

Yep, pretty easy.  I think this should do it.  This patch follows comment 2: if any pref is not default, show the custom option.  Otherwise show remember or don't-remember appropriately.

Comment 4

[Drew Willcoxon :adw]

Sorry Ehsan, I didn't reload the page between the time Beltzner assigned the bug to you and I attached the patch.

Comment 5

[(no longer active)]

Comment on attachment 375701 [details] [diff] [review]
patch v1

r=me on the code, but this needs automated tests as well.  (Not to mention that my review here is not sufficient as well :-) ).

Drew, if you're willing to take this, please submit a patch with automated tests (you can look at the existing tests for the privacy prefpane for reference).  If not, I'd be happy to augment this with tests and submit it for review.

Comment 6

[Drew Willcoxon :adw]

I'd be happy to take it since you're working on that other blocker.

Comment 7

[Drew Willcoxon :adw]

Attachment: patch v2 (with test)

I guess johnath is the right reviewer?  Ehsan, I'd appreciate your fly-by though, because...

The behavior here is a little more complex than I first thought.  Selecting the remember option in the dropdown resets all the "micro-prefs" that may be toggled when the custom option is selected in the dropdown.  In other words, selecting remember wipes out all custom prefs, but selecting dontremember doesn't.  To summarize (where "dropdown == remember" means remember is selected in the dropdown and "dropdown = remember" means select remember in dropdown):

dropdown == remember -> dropdown = custom -> toggle cookies -> dropdown = remember -> all micro-prefs reset, when window reopened dropdown == remember

dropdown == remember -> dropdown = custom -> toggle cookies -> dropdown = dontremember -> all micro-prefs not reset, when window reopened dropdown == custom

So that's what this test checks.

To button it up, re: comment 0 the "after step 5" comment in comment 0 is invalid; that's the expected behavior.  The "after step 7" portion is valid; "use custom settings for history" should be selected after step 7.

Comment 8

[(no longer active)]

Comment on attachment 375832 [details] [diff] [review]
patch v2 (with test)

OK, I gave this some more thought, and I don't think this is the right thing to do.  It _may_ be a good way to solve the UI inconsistency this way, but it's not correct because if the auto-start mode is active, none of the "custom" prefs will take effect.  So what the code does currently is exactly what happens in reality.

Comment 9

[Drew Willcoxon :adw]

(In reply to comment #8)
Oh.  So it sounds like we ought to turn off auto-start whenever the user toggles any custom prefs.  Would that be right?

Comment 10

[(no longer active)]

(In reply to comment #9)
> (In reply to comment #8)
> Oh.  So it sounds like we ought to turn off auto-start whenever the user
> toggles any custom prefs.  Would that be right?

Yes, that sounds right.  Testing that is a bit more involved though, since you need to make sure that pref is disabled upon changing any of the custom prefs.  :-)

Comment 11

[Drew Willcoxon :adw]

The only prefs that the UI allows the user to toggle when auto-start is active are the cookie prefs.  Why is that?  If the widgets for the other custom prefs are disabled when auto-start is active, why aren't the cookie widgets?  Either the cookie widgets ought to be disabled when auto-start is active, or they are not really custom micro-prefs.

Comment 12

[(no longer active)]

I'm not 100% sure, as I modeled it after the mockup <http://people.mozilla.com/~faaborg/files/shiretoko/privacyPrefpanei4.png>, provided by Alex.

I think the reason that these controls remain enabled is that we would like to give finer grained control on cookies even when in private browsing mode.

Comment 13

[Drew Willcoxon :adw]

(In reply to comment #12)
> I think the reason that these controls remain enabled is that we would like to
> give finer grained control on cookies even when in private browsing mode.

If that's the case, then this part from comment 8 is not right:

> do.  It _may_ be a good way to solve the UI inconsistency this way, but it's
> not correct because if the auto-start mode is active, none of the "custom"
> prefs will take effect.

Given comment 12, I think patch v2 is the right solution.  When auto-start is enabled and there are custom prefs toggled, show the custom pane in the UI.  The auto-start checkbox at the top is checked, so there is feedback that auto-start is enabled.  If auto-start is enabled but all prefs are default, then show the simpler auto-start pane.

Comment 14

[(no longer active)]

Comment on attachment 375832 [details] [diff] [review]
patch v2 (with test)

Hmm, I can't seem to make up my mind here, and this is ultimately a UI issue, so let's first have Alex's ui-review here.  :-)

Comment 15

[Drew Willcoxon :adw]

Executive summary for Alex:

Either cookie prefs should be toggleable when private browsing auto-start is enabled or they shouldn't (comment 11).  Currently they are.  In that case, I argue that the last paragraph of comment 13 should be the behavior.  Ehsan is thinking (I think?) auto-start should be switched off once the cookie prefs are toggled (comment 9 and comment 10).  But if that's true, I argue that the cookie prefs should be non-toggleable in the first place while auto-start is enabled.

Comment 16

[(no longer active)]

Given the discussion that has happened here, and especially with regard to the last paragraph in comment 13, I now think that Drew's solution is the best one possible, so let's move on with that.

BTW, I noticed that I had accidentally added "clearDataSettings" to gPrivacyPane.dependentControls.  Drew, would you mind removing that in this patch (even though it's not related to this bug)?

Comment 17

[Drew Willcoxon :adw]

(In reply to comment #16)
> BTW, I noticed that I had accidentally added "clearDataSettings" to
> gPrivacyPane.dependentControls.  Drew, would you mind removing that in this
> patch (even though it's not related to this bug)?

Can do, but are you sure that's correct?  It seems strange that alwaysClear would be in dependentControls but clearDataSettings wouldn't.  When the checkbox is disabled, shouldn't its companion button also be?  Any changes I make in the dialog that opens when I click the button won't be relevant if the checkbox is unchecked and disabled.

Comment 18

[Drew Willcoxon :adw]

Ehsan, I've marked the patch for Johnathan's review to get this blocker finished.  If there's an issue with clearDataSettings (I still don't think there is), we should fix it in a different bug.

Comment 19

[(no longer active)]

(In reply to comment #17)
> Can do, but are you sure that's correct?  It seems strange that alwaysClear
> would be in dependentControls but clearDataSettings wouldn't.  When the
> checkbox is disabled, shouldn't its companion button also be?  Any changes I
> make in the dialog that opens when I click the button won't be relevant if the
> checkbox is unchecked and disabled.

Hmm, please ignore my comment, I was confused and obviously mistaken...

Comment 20

[Robert Strong (they/them - no direct email)]

Comment on attachment 375832 [details] [diff] [review]
patch v2 (with test)

r=me

Comment 21

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a31e7fe8653f

Comment 22

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b64bb2904663

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So we don't show "Remember history" and "Never remember history" after a user reopens the preferences dialog when having set both of the above settings? For me the privacy pane always switches to custom settings with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090517 Minefield/3.6a1pre ID:20090517031025.

Comment 24

[Drew Willcoxon :adw]

If any custom prefs are set, you should see the custom pane.  Otherwise you should see either of the simpler remember or don't-remember panes.

If you switch to "Never remember" but have custom prefs set, you'll see the custom pane next time you open the dialog, but the "Automatically start Minefield in a private browsing" checkbox at the top will be checked.  However, when you switch to "Remember history", your custom prefs -- with the exception of the number of days to store history -- are reset to their defaults, and you should see the simple "Remember history" pane next time you open the dialog (see comment 7).

If you still see the custom pane after selecting "Remember history", are you sure you didn't set a custom number of days to store history?  (90 is the default.)

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, I've found the exception. Choose custom settings and set the history days to another value. Then switch to remember history and reopen the preferences dialog. In comparison to all the other prefs we don't reset the days value. I would tend to say that we should do that. I will file a new bug when it's a valid request.

Comment 26

[Drew Willcoxon :adw]

Actually Ehsan, do you know why the custom prefs are reset when selecting the remember option in the dropdown but not reset for dontremember?  I can see how that can cause confusion.  Seems like it might be a good idea to reset them in both cases.

Comment 27

[(no longer active)]

(In reply to comment #26)
> Actually Ehsan, do you know why the custom prefs are reset when selecting the
> remember option in the dropdown but not reset for dontremember?  I can see how
> that can cause confusion.  Seems like it might be a good idea to reset them in
> both cases.

Before the patch in this bug, that was not an issue, but I guess we should do that with the new behavior of the privacy prefpane...

Can you please file a new bug on that?

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on all platforms with builds on trunk and 1.9.1:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090517 Minefield/3.6a1pre ID:20090517031025

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090517 Shiretoko/3.5b5pre ID:20090517031745

(In reply to comment #27)
> Can you please file a new bug on that?

Filed as bug 493527.