Closed Bug 1439082 Opened 2 years ago Closed 2 years ago

"Allow Firefox to install and run studies" disabled state is different from before and after editing parent checkbox or reloading the page

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 + verified
firefox60 --- verified

People

(Reporter: magicp.jp, Assigned: Gijs)

References

Details

Attachments

(2 files)

Steps to reproduce:
1. Launch Nightly with a new profile
2. Go to about:preferences#privacy > Nightly Data Collection and Use
3. Check off "Allow Nightly to send technical and interaction data to Mozilla"
4. Reload the page
5. Check on/off "Allow Nightly to send technical and interaction data to Mozilla"

Actual results:
In step 2, "Allow Firefox to install and run studies" is editable.
In step 3, it is disabled.
In step 4, it is editable.
In step 5. it is disabled.


Expected results:
Which disabled state is correct? It should apply same behavior.


Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d760cf06ca40056077e2a354b8b69350a5765ae3&tochange=0adedb70b7883ab39239b8531d07f1308ffc10c7
Blocks: 1379338
Has Regression Range: --- → yes
Has STR: --- → yes
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8951855 [details]
Bug 1439082 - ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off,

Myk, to me the regression window here points to the change event not firing (or perhaps firing earlier?) than it did before. Is that expected? Is this a reasonable fix, or should we look at doing something else and/or should we audit any other cases where we dynamically add preference bindings?
Attachment #8951855 - Flags: feedback?(myk)
Comment on attachment 8951855 [details]
Bug 1439082 - ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off,

https://reviewboard.mozilla.org/r/221148/#review227610
Attachment #8951855 - Flags: review?(mcooper) → review+
Comment on attachment 8951855 [details]
Bug 1439082 - ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off,

https://reviewboard.mozilla.org/r/221148/#review227654

::: browser/extensions/shield-recipe-client/lib/ShieldPreferences.jsm:137
(Diff revision 1)
>      doc.defaultView.Preferences.add({ id: OPT_OUT_STUDIES_ENABLED_PREF, type: "bool" });
>  
>      // Weirdly, FHR doesn't have a Preference instance on the page, so we create it.
>      const fhrPref = doc.defaultView.Preferences.add({ id: FHR_UPLOAD_ENABLED_PREF, type: "bool" });
> -    function onChangeFHRPref(event) {
> -      checkbox.disabled = !Services.prefs.getBoolPref(FHR_UPLOAD_ENABLED_PREF) || !allowedByPolicy;
> +    function onChangeFHRPref() {
> +      checkbox.setAttribute("disabled", Services.prefs.prefIsLocked(FHR_UPLOAD_ENABLED_PREF) ||

Setting the "disabled" attribute to the string value "false" (which is what will happen here when the conditional evaluates to boolean false, since setAttribute expects its second argument to be a string) should cause the checkbox to be disabled, since XUL checkboxes should become disabled whenever the value of their "disabled" attribute is set to any value, per <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/checkbox#a-disabled>.

Thus this should either remove the attribute when the checkbox should be enabled or continue to set the value using the disabled property <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/checkbox#p-disabled>.
Attachment #8951855 - Flags: review-
Comment on attachment 8951855 [details]
Bug 1439082 - ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off,

(In reply to :Gijs from comment #2)
> Comment on attachment 8951855 [details]
> Bug 1439082 - ensure shield checkbox is disabled when about:preferences
> loads while FHR/Telemetry is preffed off,
> 
> Myk, to me the regression window here points to the change event not firing
> (or perhaps firing earlier?) than it did before. Is that expected? Is this a
> reasonable fix, or should we look at doing something else and/or should we
> audit any other cases where we dynamically add preference bindings?

It's possible that the change event is firing differently than it did before.  However, the code that initially sets the disabled state of the checkbox doesn't depend on the change event handler, so it isn't clear to me that this issue is related to any changes to the change event.

Note that bug 1431274 switched from checking the value to checking the locked status of FHR_UPLOAD_ENABLED_PREF when initially setting the disabled state of the checkbox:

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

However, that bug didn't change the logic for setting the disabled state of that checkbox in the change event handler.  (It also continued to set the initial state via setAttribute, which will cause the checkbox to be disabled in some cases when it should be enabled.)

It isn't clear to me why the logic for setting the disabled state should diverge between the initial setter and the change event handler.  Assuming that was a mistake, your fix to refactor it into the change event handler makes sense and will avoid such accidental divergence in the future.
Attachment #8951855 - Flags: feedback?(myk) → feedback+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> Setting the "disabled" attribute to the string value "false" (which is what
> will happen here when the conditional evaluates to boolean false, since
> setAttribute expects its second argument to be a string) should cause the
> checkbox to be disabled, since XUL checkboxes should become disabled
> whenever the value of their "disabled" attribute is set to any value, per
> <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/checkbox#a-
> disabled>.

Erm, this is actually incorrect.  The checkbox XBL binding extends basetext, which extends basecontrol, whose setter for the "disabled" property does remove the attribute when setting the property to boolean false.

However, that setter sets the attribute to the string "true" when setting the property to boolean true.  And the getter returns true only if the attribute is the string "true"; otherwise it returns false.

https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/toolkit/content/widgets/general.xml#12-23

Also, style rules for checkboxes seem to depend on whether or not the "disabled" attribute has the specific value "true", not just whether or not it's defined:

https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/toolkit/themes/linux/global/checkbox.css#41-43
https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/toolkit/themes/osx/global/checkbox.css#23-25
https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/toolkit/themes/windows/global/checkbox.css#39-41

So it's actually fine to use setAttribute to set "disabled" to either "false" or "true".

checkbox[disabled="true"] {
Comment on attachment 8951855 [details]
Bug 1439082 - ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off,

https://reviewboard.mozilla.org/r/221148/#review227684
Attachment #8951855 - Flags: review+
Comment on attachment 8951855 [details]
Bug 1439082 - ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off,

https://reviewboard.mozilla.org/r/221148/#review227654

> Setting the "disabled" attribute to the string value "false" (which is what will happen here when the conditional evaluates to boolean false, since setAttribute expects its second argument to be a string) should cause the checkbox to be disabled, since XUL checkboxes should become disabled whenever the value of their "disabled" attribute is set to any value, per <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/checkbox#a-disabled>.
> 
> Thus this should either remove the attribute when the checkbox should be enabled or continue to set the value using the disabled property <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/checkbox#p-disabled>.

Gah. I blame the stupid MOZ_TELEMETRY_REPORTING thing requiring hoop-jumping to test this locally (which I did do, fwiw, and I'm still not sure how I missed this).

Switched to removing the attribute. Setting the property doesn't work because the first time this code runs the element isn't in the DOM so no XBL binding, and although that 'property' is documented as being XUL-y in MDN, it actually comes from https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/general.xml#15-18 and so it doesn't work. :-\
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b3946f6cb5f2
ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off, r=myk,mythmon
https://hg.mozilla.org/mozilla-central/rev/b3946f6cb5f2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Gijs, want to request uplift?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8951855 [details]
Bug 1439082 - ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1379338
[User impact if declined]: confusing disabled state of pref in the UI in some cases
[Is this code covered by automated tests?]: no :-(
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: small fix, two people reviewed it, limited impact to the pref UI that is already affected by this bug ("how much worse could it get")
[String changes made/needed]: no
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8951855 - Flags: approval-mozilla-beta?
Comment on attachment 8951855 [details]
Bug 1439082 - ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off,

I'd like the prefs to accurately reflect what users are choosing for data privacy options so let's uplift for beta 14. 

Andrei can you ask your team to verify the fix in beta 59?
Flags: needinfo?(andrei.vaida)
Attachment #8951855 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I've hit some merge conflicts whren trying to graft this from central. Could you please take a look at this?

grafting 464381:b3946f6cb5f2 "Bug 1439082 - ensure shield checkbox is disabled when about:preferences loads while FHR/Telemetry is preffed off, r=myk,mythmon"
merging browser/extensions/shield-recipe-client/lib/ShieldPreferences.jsm
warning: conflicts while merging browser/extensions/shield-recipe-client/lib/ShieldPreferences.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(gijskruitbosch+bugs)
I have reproduced this issue using Firefox  60.0a1 (2018.02.17) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox latest nightly 60.0a1 on Win 8.1 x64, Mac OS X 10.13.4 and Ubuntu 14.04 x64. This fix is not yet on 59.0b14, I will verify on Firefox 59.0b15.
I can confirm this issue is fixed, I verified using Firefox 59.0-RC build on Win 8.1 x64, Mac OS X 10.13.2 and Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
You need to log in before you can comment on or make changes to this bug.