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

VERIFIED FIXED in Firefox 3.6a1

Status

()

defect
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: whimboo, Assigned: adw)

Tracking

({verified1.9.1})

3.5 Branch
Firefox 3.6a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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".
Flags: blocking-firefox3.5?

Comment 1

10 years ago
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.
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.
Assignee: nobody → ehsan.akhgari
Flags: blocking-firefox3.5? → blocking-firefox3.5+
Assignee

Comment 3

10 years ago
Posted patch patch v1 (obsolete) — Splinter Review
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.
Attachment #375701 - Flags: review?(ehsan.akhgari)
Assignee

Comment 4

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

Comment 5

10 years ago
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.
Attachment #375701 - Flags: review?(ehsan.akhgari) → review+
Assignee

Comment 6

10 years ago
I'd be happy to take it since you're working on that other blocker.
Assignee: ehsan.akhgari → adw
Status: NEW → ASSIGNED
Assignee

Comment 7

10 years ago
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.
Attachment #375701 - Attachment is obsolete: true
Attachment #375832 - Flags: review?(johnath)

Updated

10 years ago
Attachment #375832 - Flags: review?(johnath) → review-

Comment 8

10 years ago
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.
Assignee

Comment 9

10 years ago
(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

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

Comment 11

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

10 years ago
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.
Whiteboard: [needs revised patch]
Assignee

Comment 13

10 years ago
(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

10 years ago
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.  :-)
Attachment #375832 - Flags: ui-review?(faaborg)

Updated

10 years ago
Attachment #375832 - Flags: review-

Updated

10 years ago
Whiteboard: [needs revised patch] → [needs ui-r faaborg]
Assignee

Comment 15

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

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

Comment 17

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

Updated

10 years ago
Attachment #375832 - Flags: ui-review?(faaborg)
Assignee

Updated

10 years ago
Whiteboard: [needs ui-r faaborg]
Assignee

Updated

10 years ago
Attachment #375832 - Flags: review?(johnath)
Assignee

Comment 18

10 years ago
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.
Whiteboard: [needs review johnath][current status in comments 16-18]

Comment 19

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

Updated

10 years ago
Whiteboard: [needs review johnath][current status in comments 16-18] → [needs review johnath][current status in comments 18-19]
Assignee

Updated

10 years ago
Attachment #375832 - Flags: review?(johnath) → review?(dao)
Assignee

Updated

10 years ago
Whiteboard: [needs review johnath][current status in comments 18-19] → [needs review dao][current status in comments 18-19]
Assignee

Updated

10 years ago
Attachment #375832 - Flags: review?(dao) → review?(robert.bugzilla)
Assignee

Updated

10 years ago
Whiteboard: [needs review dao][current status in comments 18-19] → [needs review rs][current status in comments 18-19]
Comment on attachment 375832 [details] [diff] [review]
patch v2 (with test)

r=me
Attachment #375832 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [needs review rs][current status in comments 18-19] → [current status in comments 18-19]
Assignee

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [current status in comments 18-19]

Comment 21

10 years ago
http://hg.mozilla.org/mozilla-central/rev/a31e7fe8653f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
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.
Assignee

Comment 24

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

Comment 26

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

10 years ago
(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?
Depends on: 493527
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.