1075055 - Make it impossible to turn on telemetry unless FHR is also turned on.

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Benjamin Smedberg]

Short summary: make it impossible to turn on telemetry unless FHR is also turned on.

Currently FHR and telemetry have completely independent checkboxes in the prefs. For various technical reasons, this is no longer going to be possible: there should only be three possible states:

* FHR+telemetry (default for prerelease channels)
* FHR (default for release channel)
* No data

I don't have an opinion about exactly how the UI should work: we could grey out the telemetry checkbox if FHR is not checked, or we could replace the current setup with a radio or select box. I'm going to file a UX bug specifically to get the design settled, blocking this one.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Note that we will ship in-content prefs in 38, but will keep the normal pref UI around as fallback.
That means that we have to do this in both in-content and classic prefs.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Benjamin, do you think you can take this or should we find someone else?

Comment 3

[Benjamin Smedberg]

I can't commit to this, no.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Change the strings.

This patch removes a string which is no longer used on desktop and changes another string.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Changes the options dialog (in content and classic).

This patch changes both the in-content and classic Telemetry prefs, making it impossible to turn Telemetry ON if FHR is OFF.

The classic options are shown a little differently from the UX sketch in bug 1075056. Is this ok or should I get rid of the <groupbox>? (see the attached pictue)

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: classic.PNG

This is different from the UX design, as there's a box border visible around the FHR group of options.

This border is still there because every other options group in the classic version has it. Getting rid of it would make this section look differently from the other sections (e.g. "General").

If we decide to keep the box border in this section (for the classic version only), there should be no need for the line separator.

Also, due to the

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: incontent.PNG

In-content updated FHR preferences.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566005 [details] [diff] [review]
part 1 - Change the strings.

Review of attachment 8566005 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd
@@ +36,2 @@
>  <!ENTITY telemetryDesc.label             "Shares performance, usage, hardware and customization data about your browser with &vendorShortName; to help us make &brandShortName; better">
> +<!ENTITY enableTelemetryData.label       "Share additional data (e.g. Telemetry)">

e.g. is used to describe one or more examples. In this case, I'm not aware of any other data collection outside of Telemetry, but "e.g." may imply that there are other systems also in place.

I think you want "i.e." here, which roughly translate from Latin to English as "in essence", so this is saying "Share additional data (i.e., Telemetry)", which expanded would be "Share additional data (in essence, Telemetry)".

Note also that in US-English, a comma always should follow "e.g." and "i.e.", while in British English the comma is not used.

* These rules and recommendations are from http://www.wikihow.com/Use-%22i.e.%22-Versus-%22e.g.%22

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566006 [details] [diff] [review]
part 2 - Changes the options dialog (in content and classic).

Review of attachment 8566006 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/advanced.js
@@ +237,5 @@
> +  /**
> +   * Set the disabled status of the telemetry controls based on the input argument.
> +   * @param {Boolean} aDisabled True disables the controls, false enables them.
> +   */
> +  setTelemetryControlsStatus: function (aDisabled)

Functions that take a negated argument can be confusing to use. For example, if I call `setTelemetryControlsStatus(true)`, this will actually disable the controls even though there is nothing within the function name or argument that implies a "negative" connotation.

We should change this function signature to `function setTelemetrySectionEnabled (aEnabled)`.

::: toolkit/themes/shared/in-content/common.inc.css
@@ +46,5 @@
>    line-height: 22px;
>    margin: 0 !important;
>  }
>  
> +xul|caption > xul|checkbox {

Please move this selector up to be in the same ruleset as `xul|caption > xul|label`.

@@ +418,5 @@
>  
> +html|a[disabled="true"],
> +.text-link[disabled="true"],
> +.inline-link[disabled="true"] {
> +  line-height: 22px;

Why does the line-height need to be declared when they are disabled?

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> > +<!ENTITY enableTelemetryData.label       "Share additional data (e.g. Telemetry)">
> 
> e.g. is used to describe one or more examples. In this case, I'm not aware
> of any other data collection outside of Telemetry, but "e.g." may imply that
> there are other systems also in place.
> 
> I think you want "i.e." here, which roughly translate from Latin to English
> as "in essence", so this is saying "Share additional data (i.e.,
> Telemetry)", which expanded would be "Share additional data (in essence,
> Telemetry)".

This setting would cover more of the data collection than the "classic telemetry data".
I think it would also entail additional ping types (like loop data & other potential implementations).

Benjamin, can you confirm?
Would it be wrong from the privacy/datacollection policy perspective to say this enables Telemetry?
Or is the term "Telemetry" fine here and covering the other data collections too?

Comment 11

[Benjamin Smedberg]

This checkbox is specifically about "telemetry", which is a term that may change in scope over time, but in terms of user expectations is a single thing. It's not "telemetry and other stuff".

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Change the strings.

Updates the string and removes crashReportSection.label (no longer used).

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Changes the options dialog (in content and classic) - v2

Thank you for your time! This patch:

- Renames |setTelemetryControlsStatus| to |setTelemetrySectionEnabled|
- Moves the |xul|caption > xul|checkbox| selector
- Removes the line height.

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Add browser mochitests

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Allow the use of !telemetry in tests.

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Change the strings. v2

Thank you Jared for your review!

Comment 17

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Changes the options dialog (in content and classic) - v2

Updated the commit message.

Comment 18

[Alessio Placitelli [:Dexter]]

A try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a5a15c527d7

Comment 19

[Alessio Placitelli [:Dexter]]

Whoops, wrong link posted. Correct try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=724477eee476

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566466 [details] [diff] [review]
part 3 - Add browser mochitests

Review of attachment 8566466 [details] [diff] [review]:
-----------------------------------------------------------------

Holding back r+ due to the question about "policy" in browser/components/preferences/tests/browser_telemetry.js.

::: browser/components/preferences/in-content/tests/browser_telemetry.js
@@ +30,5 @@
> +            "Telemetry checkbox must be enabled if FHR is checked.");
> +
> +  // Uncheck the FHR checkbox and make sure that Telemetry checkbox gets disabled.
> +  fhrCheckbox.checked = false;
> +  fhrCheckbox.doCommand();

These two lines appear to be trying to do the same thing. Does fhrCheckbox.click() get you the same result in only one line?

::: browser/components/preferences/tests/browser_telemetry.js
@@ +56,5 @@
> +  let service = Cc["@mozilla.org/datareporting/service;1"]
> +                  .getService(Ci.nsISupports)
> +                  .wrappedJSObject;
> +  service.policy._prefs.resetBranch("datareporting.policy.");
> +  service.policy.dataSubmissionPolicyBypassNotification = true;

This policy-related changes aren't in the in-content tests. Should they be added to the in-content tests?

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566475 [details] [diff] [review]
part 1 - Change the strings. v2

Review of attachment 8566475 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd
@@ +38,1 @@
>  <!ENTITY enableTelemetry.accesskey       "T">

Sorry, I missed this on the first review pass, but the entity name for enableTelemetry.accesskey will need to be updated to enableTelemetyData.accesskey so that the prefix remains consistent between the two strings. This helps localizers keep the accesskey up to date when the string being translated changes.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

What are the plans for migrating users who may have Telemetry enabled but FHR disabled?

Comment 23

[Benjamin Smedberg]

My understanding of the code is that they will not be sending anything, and it's enough of a minority case that I don't care about it beyond that. Georg can you confirm that's what will happen?

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566476 [details] [diff] [review]
part 2 - Changes the options dialog (in content and classic) - v2

Review of attachment 8566476 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/advanced.xul
@@ +202,3 @@
>            </groupbox>
> +          <separator/>
> +          <separator class="groove"/>

This shouldn't be necessary for the windowed preferences, as the groupbox there already draws its own borders.

::: browser/components/preferences/in-content/advanced.js
@@ +277,5 @@
> +      // If we disable FHR, untick the telemetry checkbox.
> +      document.getElementById("submitTelemetryBox").checked = false;
> +    }
> +    document.getElementById("telemetryDataDesc").disabled = disabled;
> +    document.getElementById("telemetryLearnMore").disabled = disabled;

Looking at this more, I don't think we should disable the "Learn more" link. It removes the ability for the user to learn more about the feature and give them a reason to enable FHR as well as Telemetry.

::: browser/components/preferences/in-content/advanced.xul
@@ +226,3 @@
>        </groupbox>
> +      <separator/>
> +      <separator class="groove"/>

These separators look out of place (and the above separator seems unnecessary [just adding whitespace]). Let's ditch the separators for both in-content and windowed.

Throughout the in-content preferences we use the pre-existing groupbox captions to draw distinction between the groups. I think this approach is sufficient and we don't need to add these extra separators.

::: toolkit/themes/shared/in-content/common.inc.css
@@ +412,5 @@
>  
> +html|a[disabled="true"],
> +.text-link[disabled="true"],
> +.inline-link[disabled="true"] {
> +  color: #addcf4;

I'm a bit worried about the contrast of this text, as well as it working with high-contrast themes.

Making my recommended change of not disabling the Learn More link will make these changes unnecessary, so that provides a simple answer to my worries :)

Comment 25

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #23)
> My understanding of the code is that they will not be sending anything, and
> it's enough of a minority case that I don't care about it beyond that. Georg
> can you confirm that's what will happen?

This will be the behavior. I filed bug 1134775 to track the migration and a corner-case explicitly.

Comment 26

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Change the strings. v3

Fixes the accesskey entry for telemetry data.

Comment 27

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Changes the options dialog (in content and classic) - v3

Thank you for your help :jaws! This patch implements all of your suggestions.

Comment 28

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Add browser mochitests (v2)

This patch uses |click()| instead of manually triggering the changes in the tests.

> This policy-related changes aren't in the in-content tests. Should they be added to the in-content tests?

No, as they were not in the in-content healthreport test as well [1].

[1] - https://hg.mozilla.org/mozilla-central/annotate/9696d1c4b3ba/browser/components/preferences/in-content/tests/browser_healthreport.js

Comment 29

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Allow the use of !telemetry in tests.

Updated the commit message.

Comment 30

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Add browser mochitests (v2)

Updated the commit message.

Comment 31

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/9e4ee18f745a
https://hg.mozilla.org/integration/fx-team/rev/460076a2e344
https://hg.mozilla.org/integration/fx-team/rev/ac7c99989aa5
https://hg.mozilla.org/integration/fx-team/rev/ee9a8d36a96a

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e4ee18f745a
https://hg.mozilla.org/mozilla-central/rev/460076a2e344
https://hg.mozilla.org/mozilla-central/rev/ac7c99989aa5
https://hg.mozilla.org/mozilla-central/rev/ee9a8d36a96a

Comment 33

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Firefox 38 Beta 8 (buildID: 20150427090451).