Closed Bug 1392960 Opened 7 years ago Closed 7 years ago

Privacy preference checkboxes don't match preference values when reached via about:preferences#privacy-reports

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: aflorinescu, Assigned: osmose)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Bug 1392960: Do not hide <preferences> elements for subcategories.
59 bytes, text/x-review-board-request
jaws
: review+
gchang
: approval-mozilla-beta+
Details 
[Environment:]
57.0a1 20170822142709
Ubuntu 16.04 x64, OsX 10.12, Windows 10 Pro x64


[Description:]
The Nightly Data collection and Use section from Privacy and Security has a new checkbox that I'd assume should be controlling the on/off for shield studies. 

[Steps:]
1. Open FF with a new profile.
2. Open about:studies.
3. Click on "Update preferences"

[Actual Result:]
The about:preferences#privacy page, filtered to section "Nightly Data Collection and Use" is opened and the "Allow Firefox to install and run studies" is ticked off.

Checking it on/off apparently has no result.

[Expected Result:]
My assumption is:
1.-"Allow Firefox to install and run studies" should be checked by default?
2.-clicking off "Allow Firefox to install and run studies" would opt-out from all active studies the user has active?
3.-Once the Allow Firefox to install and run studies" has been checked off, I think we should be listing a different message in about:studies to hint to the user that he should still consider supporting Mozilla, etc.
Depends on: 1392967
Depends on: 1392982
No longer depends on: 1392982
Ok, so update on this bug: there are differences when accessing the about:preferences#privacy from Options/Privacy and security and from about:studies.
The checkbox being checked/unchecked when it's not supposed to be is a bug that I'll be investigating.

As to the point that current studies should be ended when the user unchecks the box, I think that's up to Matt_G. An argument we've made in the past is that studies for features (like Screenshots) wouldn't work well with the checkbox if it auto-ended studies, because from a user's perspective, it's not clear that unchecking that checkbox will disable a feature they may not know is part of a study. But it also seems like users who uncheck the box wouldn't have wanted these studies in the first place.

mythmon suggested that we could show the number of in-progress studies on the preferences page if the box is unchecked as a signal to the user that they still need to manually leave them.

Matt_G: Should the opt-out checkbox in the preferences end any in-progress studies if it is unchecked?
Flags: needinfo?(mgrimes)
I think that at this point unchecking the box should disable all studies. If a user loses a cool new feature as a result they'll be able to see that in the study history and it may encourage them to opt back in. Leaving you enrolled after you've used the global opt-out seems like a more confusing experience for the end user. If we get a lot of feedback that people "lost" features by accident we can revisit in the future.
Flags: needinfo?(mgrimes)
Assignee: nobody → mkelly
The checkbox mismatch is a bug with the preferences page; it's marking the privacy <preferences> element as hidden when the page is reached via about:preferences#privacy-reports, which blocks the XBL bindings that hook up the checkboxes to the preference values.

I'll file a separate issue for stopping studies when the preference changes.
Component: General → Preferences
Product: Shield → Firefox
Summary: [Shield] "Allow Firefox to install and run studies" checkbox lacks functionality → Privacy preference checkboxes don't match preference values when reached via about:preferences#privacy-reports
See Also: → 1393257
Comment on attachment 8900478 [details]
Bug 1392960: Do not hide <preferences> elements for subcategories.

https://reviewboard.mozilla.org/r/171844/#review177110

::: browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js:48
(Diff revision 1)
> +// Test opening to a subcategory displays the correct values for preferences
> +add_task(async function() {
> +  await SpecialPowers.pushPrefEnv({
> +    set: [["browser.crashReports.unsubmittedCheck.autoSubmit", true]],

Can you add a second test that sets this pref to false and verifies that the checkbox is unchecked?
Attachment #8900478 - Flags: review?(jaws) → review+
Pushed by mkelly@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8b169184e59
Do not hide <preferences> elements for subcategories. r=jaws
Backed out for failing new test after merge:

https://hg.mozilla.org/integration/autoland/rev/dd5f0e80d0679cd1e9ab9d83a871b9f1f65ed4d8

Merge with which the failure started: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b395d209667f8689d31ff48ee64392c85621aacb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log (only hits on asan): https://treeherder.mozilla.org/logviewer.html#?job_id=126082323&repo=autoland

[task 2017-08-26T01:15:30.569916Z] 01:15:30     INFO - Entering test bound 
[task 2017-08-26T01:15:30.571954Z] 01:15:30     INFO - Buffered messages logged at 01:15:26
[task 2017-08-26T01:15:30.577984Z] 01:15:30     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js | General pane is selected by default - 
[task 2017-08-26T01:15:30.580261Z] 01:15:30     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js | The subcategory should be removed from the URI - 
[task 2017-08-26T01:15:30.585567Z] 01:15:30     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js | Privacy pane should be selected - 
[task 2017-08-26T01:15:30.588817Z] 01:15:30     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js | The subcategory should be removed from the URI - 
[task 2017-08-26T01:15:30.594983Z] 01:15:30     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js | Location Bar prefs should be hidden when only Reports are requested - 
[task 2017-08-26T01:15:30.596481Z] 01:15:30     INFO - Leaving test bound 
[task 2017-08-26T01:15:30.603852Z] 01:15:30     INFO - Entering test bound 
[task 2017-08-26T01:15:30.605052Z] 01:15:30     INFO - Buffered messages finished
[task 2017-08-26T01:15:30.610507Z] 01:15:30     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js:57 - TypeError: doc.querySelector(...) is null
[task 2017-08-26T01:15:30.613179Z] 01:15:30     INFO - Stack trace:
[task 2017-08-26T01:15:30.614792Z] 01:15:30     INFO -     @chrome://mochitests/content/browser/browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js:57:5
[task 2017-08-26T01:15:30.616354Z] 01:15:30     INFO -     Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:796:21
[task 2017-08-26T01:15:30.617762Z] 01:15:30     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:787:9
[task 2017-08-26T01:15:30.619244Z] 01:15:30     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:687:7
[task 2017-08-26T01:15:30.620904Z] 01:15:30     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
Flags: needinfo?(mkelly)
Comment on attachment 8900478 [details]
Bug 1392960: Do not hide <preferences> elements for subcategories.

jaws: I updated the tests so that they abort if the checkbox we're testing against is missing (since it's hidden when crash reporting is disabled in the build), can you r? this again before I try and re-land it?
Flags: needinfo?(mkelly)
Attachment #8900478 - Flags: review+ → review?(jaws)
Comment on attachment 8900478 [details]
Bug 1392960: Do not hide <preferences> elements for subcategories.

https://reviewboard.mozilla.org/r/171844/#review179580

::: browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js:51
(Diff revisions 1 - 3)
>  });
>  
>  // Test opening to a subcategory displays the correct values for preferences
>  add_task(async function() {
> +  // Skip if crash reporting isn't enabled since the checkbox will be missing.
> +  if (!("nsICrashReporter" in Ci)) {

This should check `AppConstants.MOZ_CRASHREPORTER` instead of Ci.nsICrashReporter.

```
if (!AppConstants.MOZ_CRASHREPORTER) {
  return;
}
```

::: browser/components/preferences/in-content-new/tests/browser_bug1020245_openPreferences_to_paneContent.js:72
(Diff revisions 1 - 3)
> +  if (!("nsICrashReporter" in Ci)) {
> +    return;
> +  }

Same change here.
Attachment #8900478 - Flags: review?(jaws) → review+
Pushed by mkelly@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7158e7a52d15
Do not hide <preferences> elements for subcategories. r=jaws
https://hg.mozilla.org/mozilla-central/rev/7158e7a52d15
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment on attachment 8900478 [details]
Bug 1392960: Do not hide <preferences> elements for subcategories.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1392960: When viewing the filtered privacy preferences (reached by links on about:telemetry, about:studies, etc.) in the updated preferences organization, the checkboxes for submitting crash reports and opt-out participation in Shield studies do not show the correct value (i.e. they're always unchecked, even if the prefs are set to true).

[User impact if declined]:
Users will, if they follow the links in about:telemetry and about:studies, see incorrect info about how Firefox is configured. IMO this is particularly bad in the case of opt-out study participation, as we expect most users clicking this button to be specifically searching for whether they are being enrolled in studies they may not want to be enrolled in.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
By me, yes. Not by QE or anything though.

[Needs manual test from QE? If yes, steps to reproduce]: 
I don't think so, it's a small fix. But, if they do:

1. Open about:preferences and navigate to the Privacy and Security section.
2. Scroll to the "Nightly Data Collection and Use" section and ensure the "Allow Firefox to install and run studies" or "Allow Nightly to send crash reports to Mozilla" checkboxes are checked.
3. Open about:studies and click the "Update Preferences" button.

With the bug, both of the aforementioned checkboxes will appear unchecked. With the fix, they will appear checked.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Nope.

[Why is the change risky/not risky?]:
It's a very minor change to the filtering code in about:preferences and only affects these special-purpose links.


[String changes made/needed]:
None.
Attachment #8900478 - Flags: approval-mozilla-beta?
Hi Adrian,
Can you help check if this issue was fixed in the latest nightly?
Flags: needinfo?(adrian.florinescu)
Issue verified as fixed on Nightly 57.0a1 20170903220032 Ubuntu 16.04x64, Mac OsX 10.12, Windows 10 x64
status-firefox57: fixed → verified
Flags: needinfo?(adrian.florinescu)
Comment on attachment 8900478 [details]
Bug 1392960: Do not hide <preferences> elements for subcategories.

Fix a privacy preferences inconsistency. Beta56+.
Attachment #8900478 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: