1640897 - Update UI for Data Collection and Use to match Firefox's

Status: RESOLVED FIXED

(Thunderbird :: General, task, P2)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1597180 +++

Our preferences UI related to Telemetry would be better to update to match what Firefox has.

I have a patch for this, on top of bug 1615501.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1640897_telemetry_ui_port.patch

Mostly copy paste from Firefox.
I notice Firefox put all their strings into preferences.ftl but we're using the separate files. Was there a reason for that? I've now put the new ones into preferences.ftl but they could be elsewhere too.

Comment 2

[Alessandro Castellani [:aleca]]

I notice Firefox put all their strings into preferences.ftl but we're using the separate files. Was there a reason for that? I've now put the new ones into preferences.ftl but they could be elsewhere too.

I think we had all separated files to keep the various sections organized and kind of standalone when we had a more strict separation with multiple sections and tabs.
I'm now transitioning everything into 5 files, mostly to not have the single preferences.ftl grow too much, and to better handle fluent migrations, which don't play well when migrating onto the same file from different patches.

We could consider doing a final migration to move everything into a single FTL file, but I'd do that at the end in order to keep the patches small and easy to review/migrate.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9151865 [details] [diff] [review]
bug1640897_telemetry_ui_port.patch

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

Thanks for working on this.
There are some nits and other improvements we can do.

::: mail/app/profile/all-thunderbird.js
@@ +92,5 @@
>  pref("toolkit.crashreporter.infoURL",
>       "https://www.mozilla.org/thunderbird/legal/privacy/#crash-reporter");
>  
> +pref("datareporting.healthreport.uploadEnabled", true); // Required to enable telemetry pings.
> +pref("datareporting.healthreport.infoURL", "https://www.mozilla.org/thunderbird/legal/privacy/#health-report");

Should we put this on another like like we do above?

::: mail/components/preferences/privacy.inc.xhtml
@@ +204,5 @@
> +          <hbox align="top">
> +            <image class="info-icon-telemetry" flex="1"></image>
> +          </hbox>
> +          <hbox align="center" id="dataDescriptionBox" flex="1">
> +            <html:span id="telemetryDisabledDescription" class="tail-with-learn-more" data-l10n-id="collection-health-report-telemetry-disabled"/>

let's indent this.
Also, weren't we following the consistent approach of not self closing HTML tags but only XUL tags?

@@ +225,5 @@
> +        </description>
> +#ifndef MOZ_TELEMETRY_REPORTING
> +        <description id="TelemetryDisabledDesc"
> +          class="indent tip-caption" control="telemetryGroup"
> +          data-l10n-id="collection-health-report-disabled"/>

Wrong indentation

@@ +236,5 @@
> +                  preference="browser.crashReports.unsubmittedCheck.autoSubmit2"
> +                  data-l10n-id="collection-backlogged-crash-reports"
> +                  flex="1"/>
> +        <label id="crashReporterLearnMore"
> +               class="learnMore" is="text-link" data-l10n-id="collection-backlogged-crash-reports-link"/>

Let's also indent this data attribute and put the `is` inline for greppability.

@@ +241,5 @@
> +      </hbox>
> +#endif
> +      </vbox>
> +
> +      <hbox align="start" style="padding-bottom:2em;">

We shouldn't do this. Inline style make our UI inconsistent when updating via CSS.
It's very strange that the margin of the html:fieldset doesn't take care of this spacing.
Maybe Richard can look into it.

::: mail/components/preferences/privacy.js
@@ +513,5 @@
> +    if (
> +      Services.prefs.prefIsLocked(PREF_UPLOAD_ENABLED) ||
> +      !AppConstants.MOZ_TELEMETRY_REPORTING
> +    ) {
> +      checkbox.setAttribute("disabled", "true");

Should this be a boolean and not string?

@@ +535,5 @@
> +    if (!checkbox.checked) {
> +      telemetryContainer.hidden = checkbox.checked;
> +    } else {
> +      telemetryContainer.hidden = checkbox.checked;
> +    }

Is this condition necessary?

::: mail/themes/shared/mail/preferences/preferences.css
@@ +396,5 @@
> +  background-color: rgba(12,12,13,0.2);
> +  font-size: 85%;
> +  padding: 3px;
> +  margin-top: 4px;
> +  margin-bottom: 4px;

instead of margin top and bottom, we can use `margin-block: 4px;`

@@ +401,5 @@
> +  width: 100%;
> +}
> +
> +#dataDescriptionBox {
> +  line-height: 1.3;

1.3em;

@@ +410,1 @@
>  }

We need to also add the `.learnMore` class from m-c.

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) from comment #3)

Comment on attachment 9151865 [details] [diff] [review]

Many of the things pointed out are just from the copy pasted firefox code. While correct, I think it's preferable for some cases to use the same as they do to ease future porting work.

•  <hbox align="start" style="padding-bottom:2em;">
  

We shouldn't do this. Inline style make our UI inconsistent when updating

It's working around the rendering problem which is likely due to xul/html mixing here. Firefox has a xul:groupbox so not needed there. But we already removed those in favour of html:fieldset.

•  checkbox.setAttribute("disabled", "true");
  

Should this be a boolean and not string?

No, it's a xul:checkbox.

@@ +535,5 @@

• if (!checkbox.checked) {

•  telemetryContainer.hidden = checkbox.checked;
  

• } else {

•  telemetryContainer.hidden = checkbox.checked;
  

• }

Is this condition necessary?

Copy paste, but that does look unnecessary :)

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1640897_telemetry_ui_port.patch

Comment 6

[Richard Marti (:Paenglab)]

Comment on attachment 9153033 [details] [diff] [review]
bug1640897_telemetry_ui_port.patch

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

r+ with comments considered.

::: mail/locales/en-US/messenger/preferences/preferences.ftl
@@ +67,5 @@
> +
> +collection-description = We strive to provide you with choices and collect only what we need to provide and improve { -brand-short-name } for everyone. We always ask permission before receiving personal information.
> +collection-privacy-notice = Privacy Notice
> +
> +collection-health-report-telemetry-disabled = You’re no longer allowing { -vendor-short-name } to capture technical and interaction data. All past data will be deleted within 30 days.

-vendor-short-name is `mozilla.org`. Is this correct? The same for collection-health-report below.

::: mail/themes/shared/mail/preferences/preferences.css
@@ +398,5 @@
>  }
>  
> +#telemetry-container {
> +  border-radius: 4px;
> +  background-color: rgba(12,12,13,0.2);

Please add a space after the commas.

@@ +402,5 @@
> +  background-color: rgba(12,12,13,0.2);
> +  font-size: 85%;
> +  padding: 3px;
> +  margin-top: 4px;
> +  margin-bottom: 4px;

margin-top/-bottom can simpler be written with `margin-block: 4px;`

::: python/l10n/tb_fluent_migrations/bug_1615501_preferences.py
@@ -902,5 @@
>          "mail/messenger/preferences/preferences.ftl",
>          "mail/messenger/preferences/preferences.ftl",
>          transforms_from(
>  """
> -telemetry-legend = { COPY(from_path, "telemetrySection.label") }

I don't know if this script is executed on every build. But I haven't seen that this scripts are changed when a later patch removes, adds or changes strings.

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to Richard Marti (:Paenglab) from comment #6)

-vendor-short-name is mozilla.org. Is this correct?

It's mozilla.org for nightly/unofficial - depends on the build config. It's the same as for Firefox.

I don't know if this script is executed on every build. But I haven't seen

It's run manually. I'm removing the migration of those values since they will be gone before the script is run the first time.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/907599e03dc4
Update UI for Data Collection and Use to match Firefox's. r=Paenglab

Comment 9

[Richard Marti (:Paenglab)]

Magnus, I think we should scratch this <html:fieldset>. I saw that you have added this workaround (https://hg.mozilla.org/comm-central/rev/907599e03dc4#l2.74) to fix the overflow a bit. But making the window narrower let it still overflow. FX didn't use <html:fieldset> in the prefs and we should go back, at least for TB 78, to not get bug reports because of the bad overflow behaviour (like bug 1618616).

I have nothing said in the review to let it land before the string freeze. I'm also on a fix to make the Learn more align better.