Closed Bug 873054 Opened 7 years ago Closed 7 years ago

Firefox Health Report prefs pane does not reflect preference locking

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: mkaply, Assigned: gps)

References

Details

Attachments

(2 files, 1 obsolete file)

If you lock the preferences:

datareporting.healthreport.uploadEnabled
and
datareporting.healthreport.service.enabled

Then go to preferences, the health reporting checkbox is still enabled. It should be disabled.

As a side not, this preference is not correct. The preference says "Enable Firefox Health Report", but that's not what this preference does. It simply changes whether or not the report is sent to Mozilla. It should be more clear.
Component: Telemetry → Client: Desktop
Product: Toolkit → Firefox Health Report
This is just a UI issue, correct?  The backend still prevents us from changing the pref, IIRC.
Target Milestone: --- → Firefox 24
ACtual(In reply to Mike Connor [:mconnor] from comment #1)
> This is just a UI issue, correct?  The backend still prevents us from
> changing the pref, IIRC.

That's correct. Just a UI issue.
Duplicate of this bug: 874377
I'd like to fix this, but I need some info from the FHR team.

Should I check for the lock of the preference datareporting.healthreport.uploadEnabled in the pref dialog directly?

Or would you prefer I modify the data reporting service to send back it's lock status?
Has there been any status change on this issue?  We are planning a fairly large rollout of Firefox to users, so I was just wondering if this will be fixed soon or not (and whether we need to delay our roll out).

While the preference is indeed locked, when the option is not greyed-out, it can be very confusing to users.
There has been no progress on this issue.

It's probably trivial to implement. Although, I have no clue how we normally handle things like this.

UI: Can you weigh in here? If FHR is disabled via a locked preference, what should we do? I believe we mostly care about the checkbox in the Advanced -> Data Choices pane. I see our options as:

1) Hide the FHR section completely
2) Disable the checkbox
3) Disable the checkbox with additional UI indicating why
Keywords: uiwanted
I'm willing to implement this but no one answered my questions in Comment 4. 

And we should not hide the checkbox. Locking preferences disables them.
What part of "this is open source and a contributor who knows how to do this wants to fix it" do you guys not understand?

Please get someone from the FHR team to read and respond to my questions so I can fix this.

We need this for the Firefox 24 ESR. I'm already getting reports from enterprise users.
mkaply, I'm not sure what information you're looking for. You already said that the lock should disable the checkbox in comment 7, so why not just implement that?

datareporting.healthreport.service.enabled is not exposed in the UI at all, so I don't think we need to worry about it at all in terms of UI design.
(In reply to Michael Kaply (mkaply) from comment #4)
> I'd like to fix this, but I need some info from the FHR team.
> 
> Should I check for the lock of the preference
> datareporting.healthreport.uploadEnabled in the pref dialog directly?
> 
> Or would you prefer I modify the data reporting service to send back it's
> lock status?
Repeating the question doesn't make it make any more sense... If you're trying to solve the UI problem of the checkbox doing nothing because the pref is locked, then disable the checkbox like you said.

The data reporting service wouldn't send any data if the pref was locked "off", and I don't think we care about the locked status in the FHR payload if the pref is locked "on".
If you look at the code for the Firefox Health Report preferences:

https://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/advanced.js#258

You'll see that it doesn't ever query the preference. It asks the health service:

checkbox.checked = policy.healthReportUploadEnabled;

So my question was, should health report service be the one that knows whether the preference is locked or not? Or should I just add code to the preference module that uses the preference directly.

The fact that FHR didn't put any reference to the preference in the preference code seems to imply they wanted the flexibility of not depending on the preference names in the preference code (or maybe I'm overthinking it).

Otherwise they would have just created a standard preference and we wouldn't be having this discussion.
Interestingly enough, the checkbox is disabled if there is no policy:

    let policy = Components.classes["@mozilla.org/datareporting/service;1"]
                                   .getService(Components.interfaces.nsISupports)
                                   .wrappedJSObject
                                   .policy;

    let checkbox = document.getElementById("submitHealthReportBox");

    if (!policy) {
      checkbox.setAttribute("disabled", "true");
      return;
    }

How does that happen?
shorlander: Could you please answer simple desktop UI question in comment #6 and clear uineeded?
Flags: needinfo?(shorlander)
gps, I don't think there is any UI question, the checkbox should be disabled. mkaply has a technical question about whether the pref code should check for locked preferences or whether the healthreport code should do that.
Flags: needinfo?(shorlander) → needinfo?(gps)
Keywords: uiwanted
I posed a UI question because I thought a property of good UI design is that controls are usable. A disabled control is not usable, so why display it at all?
(In reply to Gregory Szorc [:gps] from comment #16)
> I posed a UI question because I thought a property of good UI design is that
> controls are usable. A disabled control is not usable, so why display it at
> all?

Disabling locked prefs is how it has been since the beginning of time. That's not something we should change right now.
(In reply to Gregory Szorc [:gps] from comment #16)
> I posed a UI question because I thought a property of good UI design is that
> controls are usable. A disabled control is not usable, so why display it at
> all?

To be fair, some of the purpose of controls is to convey a state -- this is the "green light" or "fake padlock" principle.


(In reply to Michael Kaply (mkaply) from comment #13)
> Interestingly enough, the checkbox is disabled if there is no policy:
> How does that happen?

IIRC, it's when Firefox is built without MOZ_SERVICES_HEALTHREPORTER. I'm not inclined to spend the 30 minutes to double-check this, though!


> So my question was, should health report service be the one that knows
> whether the preference is locked or not? Or should I just add code to the
> preference module that uses the preference directly.

I'd opt for a patch like this:

  checkbox.checked = policy.healthReportUploadEnabled;
  if (policy.healthReportUploadLocked) {
    checkbox.setAttribute("disabled", "true");
  }

with a defined getter on the policy object. Here's an untested patch that adds this getter. Please go ahead and build on top of this.


> The fact that FHR didn't put any reference to the preference in the
> preference code seems to imply they wanted the flexibility of not depending
> on the preference names in the preference code (or maybe I'm overthinking
> it).

Correct. At the time this code was written, it was expected that on Android we would not be using Gecko prefs storage, and/or that some of these accessors would actually be compound.

This might still be true at some point in the future.

Also, directly accessing prefs is kinda awful: it spreads validation logic and observers around the tree, and removes the ability to sanely handle errors.
One other note on this. Even if you set upload to false, you still end up getting the ask down at the bottom if you want to report data.

If you set datareporting.healthreport.service.enabled to false, do you not get the ask anymore?

We really need to make it easier to turn this off.
Just to weigh in on the "locked control versus not displaying a control" debate: I personally would prefer that Firefox continue simply locking preferences rather than hiding them.

And the main reason I prefer that has nothing to do with anything ideological.  It's just a simple matter of being less confusing to our users.  When Firefox settings at work look identical to how they look at home -- except certain preferences are locked -- users are fully aware that they may not change them.  I'd imagine that, if we just hid the preferences, our department would get a ton of calls asking why things are missing or why they have a "special" or "messed up" version of Firefox.

If I could make one change, I'd have a preference that allows administrators to display a message in the preferences window that reads something like this: "Some settings are managed by your system administrator or information-technology department."  That would drive home the fact that the locked controls as done so purposefully, not by chance or accident.
(In reply to Jason from comment #20)

> If I could make one change, I'd have a preference that allows administrators
> to display a message in the preferences window that reads something like
> this: "Some settings are managed by your system administrator or
> information-technology department."  That would drive home the fact that the
> locked controls as done so purposefully, not by chance or accident.

Sounds like a reasonable request; file a bug in Firefox :: Preferences?
Clearing gps's needinfo (see Comment 18).
Flags: needinfo?(gps)
Target Milestone: Firefox 24 → ---
(In reply to Michael Kaply (mkaply) from comment #19)
> One other note on this. Even if you set upload to false, you still end up
> getting the ask down at the bottom if you want to report data.

Note that the data reporting notification covers multiple components -- currently FHR, telemetry, and crash reporter -- and that it's a legal and policy requirement that we proactively inform the user about the information Firefox might collect.

If you *build* Firefox without any of the flags that contribute to MOZ_DATA_REPORTING (that is, you disable crash reporter, telemetry, and FHR at build time), you won't get the info bar.

See browser/base/content/browser.js:

#ifdef MOZ_DATA_REPORTING
#include browser-data-submission-info-bar.js
#endif
> Note that the data reporting notification covers multiple components -- currently FHR, telemetry, and crash reporter -- and that it's a legal and policy requirement that we proactively inform the user about the information Firefox might collect.

I understand, but it's quite difficult to figure out how to make all this stuff not appear. In a kiosk or enterprise environment, you don't want to give user's these kind of choices.
(In reply to Michael Kaply (mkaply) from comment #24)

> I understand, but it's quite difficult to figure out how to make all this
> stuff not appear. In a kiosk or enterprise environment, you don't want to
> give user's these kind of choices.

Speaking technically, there are only two ways to prevent that notification:

* Build without any data reporting features.
* Ship with a configuration that tricks Firefox into thinking it's already notified.

If you do the latter, you are very much on your own from a privacy and legal standpoint, and you should make sure that you have turned off every data reporting aspect of the browser; I can make no claims that this will pass muster for any jurisdiction's privacy law, and I'm not a lawyer.

This is getting into legal and privacy territory, which is the point at which I stop having discussions on bugs, so I'll stop there!
rnewman, the point of the request here is to add a check to this code so that the notification does not display if all of the data-sending prefs are locked off as they would be in mkaply's enterprise configuration. Enterprises use stock Firefox with configuration, so discussing this in terms of build flags doesn't make much sense.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #26)

> Enterprises use stock Firefox with configuration, so discussing this in
> terms of build flags doesn't make much sense.

I understood the thrust of his question. My reference to build flags was an attempt to provide the background as to why things are the way they are.

Comment 25 suggests a configuration approach to disabling the notification.

As to whether a set of locked, disabled prefs are equivalent to not shipping a data reporting feature in the context of, say, German privacy law, and thus we should reflect that logic into whether to show the notification -- well, Jishnu should answer that question!
I think you're assuming somehow people want to turn it on and not tell user's. It's quite the opposite.

Enterprises want to turn EVERYTHING off and the way it is designed right now makes it quite difficult.

You have to turn off at least three preferences (maybe more) to get rid off the stuff. Crash reporting can't even be turned off with a pref.
I understand what you want to do, and no, that's not my assumption. The issue is that it's in tension with some of the things we have to do in shipping to a general audience which require being thorough in the opposite direction!

You're right that things are not designed up-front to make it easy for you to reconfigure things the way you want them to be. From my personal experience, enterprise needs have not factored into the requirements we've been given.
It is not difficult to disable FHR!

To turn off FHR data upload:

datareporting.healthreport.uploadEnabled -> false

IMO this is the preferred mechanism enterprises should use to "disable FHR."

To turn off FHR data collection completely:

datareporting.healthreport.service.enabled -> false

(This would deprive users of the ability to use about:healthreport and view the general health of their browser. One of the goals of FHR is to allow users who don't submit data to still compare their browser performance/behavior against the masses. By disabling *local* data collection, you would deprive users of this benefit.)

To disable the notification bar:

datareporting.policy.dataSubmissionPolicyBypassAcceptance -> true

(This may imply consent in a legal sense - I'm not a lawyer.)

OR

datareporting.policy.dataSubmissionPolicyAccepted -> true

(We'll likely change this in bug 862563.)

I have adjusted the bug summary to reflect this is merely a cosmetic issue in the prefs pane.
Summary: Firefox Health Report does not honor the locking of health report preferences → Firefox Health Report prefs pane does not reflect preference locking
This is the simplest implementation possible. I didn't include a
mochitest because I was not sure how to force a prefs pane reload from
the same test and was too tired at the time I coded this to write a new
test.
Attachment #769998 - Flags: review?(rnewman)
Assignee: nobody → gps
I think people would be fine leaving about:healthreport enabled if there wasn't a big "enable upload" button on the right of the page.

If uploadEnabled is locked to false, that icon in the upper right should disappear.

You guys are trying to put bandaids on the larger issue here which is that you designed this feature without giving any thought to how to easily turn it off.

That fact that you had to mention four preferences kind of gives that away.
The reason there are multiple preferences controlling behavior is because of me.

I argued that end-users and enterprise deployments should have as much control over FHR as possible without having to patch Firefox itself. I wanted to ensure people had the control they desired from day 0. The alternative could be construed as Mozilla trying to "trap" people into using FHR. I wanted to avoid this charge entirely and thus was liberal in the number of controls/preferences implemented.

While the backend implementation is solid and respects locked preferences, unfortunately some of the UI isn't as polished. The cosmetic bugs will get fixed. Until then, there's no need to sound the alarm that we didn't give thought to how to turn FHR off because those claims are patently false.
Attachment #769998 - Flags: review?(rnewman) → review+
(In reply to Gregory Szorc [:gps] from comment #33)
> The reason there are multiple preferences controlling behavior is because of
> me.
> 
> I argued that end-users and enterprise deployments should have as much
> control over FHR as possible without having to patch Firefox itself. I
> wanted to ensure people had the control they desired from day 0. The
> alternative could be construed as Mozilla trying to "trap" people into using
> FHR. I wanted to avoid this charge entirely and thus was liberal in the
> number of controls/preferences implemented.

That's great, and I really appreciate it, but where was all of this documented for end-users and enterprise?
I slept and coded tests.
Attachment #770238 - Flags: review?(rnewman)
Attachment #769998 - Attachment is obsolete: true
Can we also remove the button on about:healthreport if it locked?

This will allow me to do a better job of convincing people to leave health report on and just disable uploading.
Attachment #770238 - Flags: review?(rnewman) → review+
(In reply to Michael Kaply (mkaply) from comment #36)
> Can we also remove the button on about:healthreport if it locked?

That's a separate bug for the "Web: Health Report" component since about:healthreport is an iframe. Although, it will require some additional state be passed to the iframe. But that's easy enough to implement.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5667b9e736
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 25
Can we shoot for FF24 please? That's the next ESR.
https://hg.mozilla.org/mozilla-central/rev/cc5667b9e736
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 889896
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.