Closed Bug 1423564 Opened 2 years ago Closed 2 years ago

Custom settings for browser history checkboxes have a misleading behavior after selecting the "Always use private browsing mode" option

Categories

(Firefox :: Preferences, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- verified

People

(Reporter: emilghitta, Assigned: nhnt11)

References

Details

(Keywords: regression)

Attachments

(2 files)

[Affected versions]:
59.0a1 (BuildId:20171205220055)
58.0b9 (BuildId:20171204150510)
57.0 (BuildId:20171112125346)

[Unaffected versions]:
52.5.0 esr (BuildId:20171107091003)

[Affected platforms]:
Windows 10 64bit.
macOS 10.13.
Ubuntu 16.04 64bit.

[Steps to reproduce]:
1. Launch Firefox with a clean profile.
2. Go to about:preferences#security page.
3. Select the "Use custom settings for history" option from the History dropdown list.
4. Tick the "Always use private browsing mode" checkbox.
5. Go to about:preferences#security page after restarting Firefox.

[Expected result]:
The following options are unticked and grayed out:
- "Remember my browsing and download history"
- "Remember search and form history"
- "Clear history when Nightly closes"

[Actual result]:
The mentioned options in the expected result are displayed ticked and grayed out which is quite misleading.

Refreshing the about:preferences#privacy page unlocks the up mentioned options making changes to them possible.

[Regression range]:

This is a regression:

Last good revision: 58c1de79d494b91b1d70de5ae18833c8f297c3b7
First bad revision: 6d87add067739ed60262f9246d05606b61ca3025

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=58c1de79d494b91b1d70de5ae18833c8f297c3b7&tochange=6d87add067739ed60262f9246d05606b61ca3025
Hi Nihanth!

Can you please have a look into this?

Thanks!
Flags: needinfo?(nhnt11)
This seemed to be working before by a fluke:
> Before bug 1375870, _constructAfterChildren was getting called several times,
  resulting in change events triggering updatePrivacyMicroControls repeatedly.
> updatePrivacyMicroControls was using the auto-private-browsing checkbox to 
  decide whether to disable and hide the checkmarks of the other checkboxes.  
> After bug 1375870, at the point when updatePrivacyMicroControls is called,
  the preference value has not yet propagated to the auto-private-browsing
  checkbox, so uPMC doesn't work correctly and is not called again.

Solution:
> Use the actual preference value in uPMC.
> Get rid of the checkmarks in CSS, for consistency. The checked attribute
  will still be true, but this will reflect the actual pref value in case
  something tries to read the attribute in lieu of looking up the pref.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Flags: needinfo?(nhnt11)
Comment on attachment 8935167 [details]
Bug 1423564 - Correctly set disabled and checked states of custom history pref checkboxes.

https://reviewboard.mozilla.org/r/206032/#review212794

We've had the should-disabled-control-elements-retain-their-pref-value-or-reflect-the-real-ux discussion countless times and it's a little frustrating that we still don't have a good guideline for this. FWIW, in Firefox there are more precedent cases for just reflecting the pref value (it's usually less work), so I'm leaning towards just considering that part of this bug WONTFIX until we have a definitive ruling in our style guide.

The uncontroversial part of this bug seems to be that you can refresh the page and edit the controls. This patch solves that, and I'd prefer to de-couple it from the above mentioned discussion. Aside from that I'm really not a big fan of unchecking the checkbox in CSS, this is not about style but about very specific logic that should live in JS code, IMO.

In short, I'd r+ a patch that either removes the CSS part or replaces it with JS :)

I also noticed another bug with refreshing: You can avoid getting the "restart Firefox now" prompt when UNchecking "Always use private browsing mode" after reloading about:preferences. Maybe it's worth fixing that in this bug as well.

Thanks!
Attachment #8935167 - Flags: review?(jhofmann)
(In reply to Johann Hofmann [:johannh] from comment #4)
> Comment on attachment 8935167 [details]
> Bug 1423564 - Correctly set disabled and checked states of custom history
> pref checkboxes.
> 
> https://reviewboard.mozilla.org/r/206032/#review212794
> 
> We've had the
> should-disabled-control-elements-retain-their-pref-value-or-reflect-the-real-
> ux discussion countless times and it's a little frustrating that we still
> don't have a good guideline for this. FWIW, in Firefox there are more
> precedent cases for just reflecting the pref value (it's usually less work),
> so I'm leaning towards just considering that part of this bug WONTFIX until
> we have a definitive ruling in our style guide.
> 
> The uncontroversial part of this bug seems to be that you can refresh the
> page and edit the controls. This patch solves that, and I'd prefer to
> de-couple it from the above mentioned discussion. Aside from that I'm really
> not a big fan of unchecking the checkbox in CSS, this is not about style but
> about very specific logic that should live in JS code, IMO.
> 
> In short, I'd r+ a patch that either removes the CSS part or replaces it
> with JS :)

FWIW, this patch doesn't change any UX/visual stuff compared to before this regressed. The only reason I switched it to CSS was to prevent such fragile checks from causing regressions in the future (in case something tries to use the `checked` attribute value with the assumption that it reflects the pref).

Having said this, of course we should ideally not be writing code that uses UI state as source-of-truth (IMO).

> I also noticed another bug with refreshing: You can avoid getting the
> "restart Firefox now" prompt when UNchecking "Always use private browsing
> mode" after reloading about:preferences. Maybe it's worth fixing that in
> this bug as well.

Ah, interesting, thanks! I'll take a look.
> Having said this, of course we should ideally not be writing code that uses
> UI state as source-of-truth (IMO).

Just wanted to say that I meant this as a general principle and can imagine some cases where it would actually be the right thing to do. With that I'm done musing out loud. :P
> I also noticed another bug with refreshing: You can avoid getting the "restart Firefox now" prompt when UNchecking "Always use private browsing mode" after reloading about:preferences. Maybe it's worth fixing that in this bug as well.

Are you still looking into this? It'd be great to solve it in this bug as well, no? :)
I haven't had time to look at that yet but I'll try and get to it tomorrow. If you have time to do the review before I upload a new patch, I think we can go ahead and land this regression fix ASAP and just use a new bug for the restart prompt issue. :)
Comment on attachment 8935167 [details]
Bug 1423564 - Correctly set disabled and checked states of custom history pref checkboxes.

https://reviewboard.mozilla.org/r/206032/#review213776

r=me, but please file a follow-up bug for the other issue :)
Attachment #8935167 - Flags: review?(jhofmann) → review+
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee642303953a
Correctly set disabled and checked states of custom history pref checkboxes. r=johannh
See Also: → 1425699
I landed this but I've been staring intently at this code for a bit and I am convinced that we should fire an event after _constructAfterChildren in preferences.xml, and defer UI initialization to after this event. Another option (that I would prefer, if it can be done elegantly) is to expose a promise in preferences.xml that resolves after _constructAfterChildren is complete. Filed bug 1425699.
https://hg.mozilla.org/mozilla-central/rev/ee642303953a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
The controls are no longer editable after refreshing the about:preferences#privacy page using Firefox 59.0a1 (BuildId:20171219100203) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.

Thanks!
Status: RESOLVED → VERIFIED
Do we want to get this fix in 58, or let it ride with 59?
Flags: needinfo?(nhnt11)
Let's let it ride the train on 59. Mark 58 won't fix.
Flags: in-qa-testsuite+
Flags: needinfo?(nhnt11)
You need to log in before you can comment on or make changes to this bug.