1.19 KB, patch
|Details | Diff | Splinter Review|
Bug 493527 - Directly change the remember-history pref through the preference element since setting it through the checkbox required multiple attempts.
59 bytes, text/x-review-board-request
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090517 Shiretoko/3.5b5pre ID:20090517031745 With the patch on bug 490199 some custom prefs aren't reset when you choose "Remember history" or "Never remember history" from the dropdown. As result the pane always displays the custom settings. This is irritating and users cannot really see which of those prefs is causing this problem. Steps: 1. Open preferences and select privacy pane 2. Change the days to another value as 90 days 3. Switch to Remember or Never remember dropdown option 4. Close preferences and reopen again After step 4 the custom settings are shown whether which new selection the user made before closing the preferences pane.
I'm having trouble understanding this bug report. If a user selects custom settings, and changes things from the default (ex: days to 90 days), then selects "Remember History", then switches BACK to "Custom Settings" I would expect that the custom settings would return to how the user had set them before (ex: 90 days). Is that not happening?
Mike, all of those checkboxes are getting restored to their default values except the one for the textfield. It stays at the given input from the user. Alex, how do we have to behave here correctly?
(In reply to comment #1) > Is that not happening? That's not happening. This is: 1. Selecting "Remember history" resets all custom prefs to defaults except the number of days to remember. 2. Selecting "Never remember history" doesn't reset any custom prefs. 3. All the fix to bug 490199 does is show the custom pane if any custom prefs are not default and one of the two simpler panes (depending on the boolean value of auto-start) otherwise. We are thinking that all custom prefs should be reset for both points 1 and 2 above (bug 490199 comment 26 and 27). That's not the behavior described in comment 1 though. This isn't a regression BTW. The patch mentioned in comment 0 didn't change anything about prefs being reset or not. My head hurts.
Removing regression flag as per comment 3 (thanks, Drew) This will not block the final release of Firefox 3.5 As for a UI decision: we should follow the behaviour in comment 1. Eshan rightly points out, however, in comment 4 that we should be careful to only use the settings when the user has actually set them - which is the minute they check/uncheck the boxes on OSX, or when they hit Apply/OK on Windows/Linux. So, to make it a little clearer: - there are three choices (Always, Never, Custom) - when a user selects Custom and customizes the sub-settings and applies them (instant for OSX, on Apply/OK on w32/linux) those settings should be remembered and associated with "Custom" - if the user flips back to "Always", applies, and then back to "Custom" they should see the user's previously set custom settings Removing ui-wanted per this comment (thanks, me)
Keywords: regression, uiwanted
(In reply to comment #5) > - when a user selects Custom and customizes the sub-settings and applies them > (instant for OSX, on Apply/OK on w32/linux) those settings should be remembered > and associated with "Custom" This should be (instant for OSX/linux, on Apply/OD on w32), right? > - if the user flips back to "Always", applies, and then back to "Custom" they > should see the user's previously set custom settings Only on Windows, correct?
Correct, only when the preferences are applied, which is not instant on Windows. Not blocking, but nice polish to have.
Keywords: uiwanted → polish
Hi guys. I work at Cliqz, which you probably heard of, also using Firefox source for its product :) We received complaints from our users about this issue recently, and fixed it in our repo. If you like, here's the patch. It may be imperfect in the sense of delayed pref changes policy for Windows, but that's anyway, how the rest of prefs around are treated already. And it should better that current behaviour. Also, these reports seem to be all duplicates: https://bugzilla.mozilla.org/show_bug.cgi?id=1228686 https://bugzilla.mozilla.org/show_bug.cgi?id=1255473 https://bugzilla.mozilla.org/show_bug.cgi?id=1298065
Comment on attachment 8839853 [details] [diff] [review] 0001-DB-1173-History-preference-not-updated-when-selectin.patch Jared, can you take a look at this?
Attachment #8839853 - Flags: review?(jaws)
2 years ago
Assignee: ehsan → maxim
Status: NEW → ASSIGNED
Comment on attachment 8839853 [details] [diff] [review] 0001-DB-1173-History-preference-not-updated-when-selectin.patch Review of attachment 8839853 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. We don't have any delayed pref changes on Windows anymore, we switched away from that when we moved the prefs to inside of the Firefox window. I can't find out why this was setting it through the checkbox but the others were set through the preference element. Looking in the history it looks like this was always written this way, from http://searchfox.org/mozilla-central/diff/432c74dd9ba6f4b6bba660ed99402ffa69dba85a/browser/components/preferences/privacy.js and it was added by bug 520165. So, this looks fine. Can you update your commit message to (note, multiline): "Bug 493527 - Directly change the remember-history pref through the preference element since setting it through the checkbox required multiple attempts. r=jaws This looks to be happening because the binding for the checkbox may get torn down too fast when a restart is required."
Attachment #8839853 - Flags: review?(jaws) → review+
Maxim, are you going to update the commit message or should I?
2 years ago
Comment on attachment 8850624 [details] Bug 493527 - Directly change the remember-history pref through the preference element since setting it through the checkbox required multiple attempts. https://reviewboard.mozilla.org/r/123174/#review125556
Attachment #8850624 - Flags: review?(jaws) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/0ab9adff3faf Directly change the remember-history pref through the preference element since setting it through the checkbox required multiple attempts. r=jaws
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.