Emil Ghitta, QA [:emilghitta] Reporter
	Description • 7 years ago

[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

	Emil Ghitta, QA [:emilghitta] Reporter
	Comment 1 • 7 years ago

Hi Nihanth!

Can you please have a look into this?

Thanks!

	(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Comment 3 • 7 years ago

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.

	Johann Hofmann [:johannh]
	Comment 4 • 7 years ago
mozreview-review

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!

	(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Comment 6 • 6 years ago

(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.

	(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Comment 7 • 6 years ago

> 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

	Johann Hofmann [:johannh]
	Comment 8 • 6 years ago

> 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? :)

	(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Comment 9 • 6 years ago

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. :)

	Johann Hofmann [:johannh]
	Comment 10 • 6 years ago
mozreview-review

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 :)

	Pulsebot
	Comment 11 • 6 years ago

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

	(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Comment 12 • 6 years ago

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.

	Noemi Erli[:noemi_erli]
	Comment 13 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ee642303953a

	Emil Ghitta, QA [:emilghitta] Reporter
	Comment 14 • 6 years ago

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!

	Julien Cristau [:jcristau]
	Comment 15 • 6 years ago

Do we want to get this fix in 58, or let it ride with 59?

	Gerry Chang [:gchang]
	Comment 16 • 6 years ago

Let's let it ride the train on 59. Mark 58 won't fix.