1356020 - String Changes to about:preferences#privacy

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Justin [:JW_SoftvisionQA]]

Per Specs: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167680

Remove Colon:

History Section:
Firefox will

Location Bar Section:
When using the location bar, suggest

Certificates Section:
When a server requests your personal certificate

Update Description:

Tracking Protection Section:
Tracking is when companies collect information about you to build a profile and display content based on your browsing and personal data. (Learn More Link)

Block known tracking companies from displaying content

Update Radio Button Text:

Tracking Protection Section:
Only in Private Browsing mode

Add Check Box:

Tracking Protection Section:
Always apply Do Not Track (Learn More Link)
"Firefox will send a signal that you don't want to be tracked whenever
Tracking Protection is on."

Remove Radio Check Box:
Notifications Section:

Do not disturb me
Not notification will be shown until you restart Nightly

Remove Sections:
Container Tabs
Site Data

Add Section: (Waiting for final copy review)
Reports (previously in Advanced> Data Choices

Comment 1

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

Attachment: Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting.

Review commit: https://reviewboard.mozilla.org/r/130190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130190/

Comment 2

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

Spec for Privacy & Security pane: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167680

Comment 3

[:Gijs (he/him)]

It looks like "Reports" for telemetry etc. is still in the "Updates" section. Is there a separate bug to change that and update copy there? Just making sure that isn't falling through the cracks...

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs (away until Tuesday 18) from comment #3)
> It looks like "Reports" for telemetry etc. is still in the "Updates"
> section. Is there a separate bug to change that and update copy there? Just
> making sure that isn't falling through the cracks...

Oh, it seems this is somewhere else in the patchset you uploaded, but it's not marked as a dep of this bug and hasn't made nightly or central yet? So it's a bit tricky for me to review the markup changes you're making there...

Comment 5

[:Gijs (he/him)]

Comment on attachment 8858223 [details]
Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting.

https://reviewboard.mozilla.org/r/130190/#review132976

I reviewed the l10n changes, and though there's bits missing they look more or less OK otherwise, but I'm struggling to review the markup changes given that the patch doesn't apply on central and I don't see deps where the markup gets changed to the point that this patch modifies it further. In any case, there should be enough feedback in here for now. :-)

::: commit-message-dcf01:1
(Diff revision 1)
> +Bug 1356020 - Fix inconsistencies with strings in about:preferences#privacy. r?gijs

This is making significant markup changes and adding JS, so either those changes should be split out or the commit message should be expanded to cover them.

::: browser/components/preferences/in-content/privacy.js:91
(Diff revision 1)
>    /**
>     * Show the Containers UI depending on the privacy.userContext.ui.enabled pref.
>     */
>    _initBrowserContainers() {
>      if (!Services.prefs.getBoolPref("privacy.userContext.ui.enabled")) {
> +      document.getElementById("browserContainersGroup").setAttribute("data-hidden-from-search", "true");

This seems unrelated?

::: browser/components/preferences/in-content/privacy.xul:525
(Diff revision 1)
>  </groupbox>
>  
>  <!-- Certificates -->
> -<groupbox id="certSelection" align="start" data-category="panePrivacy" hidden="true">
> +<groupbox id="certSelection" data-category="panePrivacy" hidden="true">
>    <caption><label>&certificateTab.label;</label></caption>
> -  <description id="CertSelectionDesc" control="certSelection">&certPersonal.description;</description>
> +  <description id="CertSelectionDesc" control="certSelection">&certPersonal2.description;</description>

I wish we just hid this if we knew NSS had no personal certificates. Unrelated to this bug, of course, but if we're simplifying prefs anyway...

::: browser/components/preferences/in-content/privacy.xul:527
(Diff revision 1)
> +  <hbox align="start">
> +    <vbox flex="1">

I can't apply this patch, but the markup here surprises me. Specifically, the design in invision seems to suggest that the buttons are aligned only with the enableOCSP checkbox. Your markup will (I think?) top-align them to the cert selection radiogroup. Is this a change that was discussed elsewhere, or just a mistake, or...? :-)

::: browser/components/preferences/in-content/privacy.xul:705
(Diff revision 1)
> -  <vbox>
> +  <hbox align="center">
> -    <caption>
>      <checkbox id="submitHealthReportBox" label="&enableHealthReport.label;"
>                accesskey="&enableHealthReport.accesskey;"/>
> -  </caption>
> +    <label id="FHRLearnMore"
> -    <hbox class="indent" flex="1">
> -      <label flex="1">&healthReportDesc.label;</label>
> -      <label id="FHRLearnMore" flex="1"

Can you elaborate on what these changes are achieving?

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd:10
(Diff revision 1)
>  <!ENTITY  trackingProtectionHeader2.label      "Tracking Protection">
> +<!ENTITY  trackingProtection.description       "Tracking is when companies collect information about you to build a profile and display content based on your browsing and personal data.">
> +<!ENTITY  trackingProtection.radioGroupLabel   "Block known tracking companies from displaying content">
>  <!ENTITY  trackingProtectionAlways.label       "Always">
>  <!ENTITY  trackingProtectionAlways.accesskey   "y">
>  <!ENTITY  trackingProtectionPrivate.label      "Only in private windows">

The spec says this needs to be updated, too.

Comment 6

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

Comment on attachment 8858223 [details]
Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130190/diff/1-2/

Comment 7

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

(In reply to :Gijs (away until Tuesday 18) from comment #5)
> ::: browser/components/preferences/in-content/privacy.js:91
> (Diff revision 1)
> >    /**
> >     * Show the Containers UI depending on the privacy.userContext.ui.enabled pref.
> >     */
> >    _initBrowserContainers() {
> >      if (!Services.prefs.getBoolPref("privacy.userContext.ui.enabled")) {
> > +      document.getElementById("browserContainersGroup").setAttribute("data-hidden-from-search", "true");
> 
> This seems unrelated?

I added a comment about this in the code. This is causing a visual gap when viewing the Privacy pane.

> ::: browser/components/preferences/in-content/privacy.xul:525
> (Diff revision 1)
> >  </groupbox>
> >  
> >  <!-- Certificates -->
> > -<groupbox id="certSelection" align="start" data-category="panePrivacy" hidden="true">
> > +<groupbox id="certSelection" data-category="panePrivacy" hidden="true">
> >    <caption><label>&certificateTab.label;</label></caption>
> > -  <description id="CertSelectionDesc" control="certSelection">&certPersonal.description;</description>
> > +  <description id="CertSelectionDesc" control="certSelection">&certPersonal2.description;</description>
> 
> I wish we just hid this if we knew NSS had no personal certificates.
> Unrelated to this bug, of course, but if we're simplifying prefs anyway...

I'll file a bug about doing this.

> ::: browser/components/preferences/in-content/privacy.xul:527
> (Diff revision 1)
> > +  <hbox align="start">
> > +    <vbox flex="1">
> 
> I can't apply this patch, but the markup here surprises me. Specifically,
> the design in invision seems to suggest that the buttons are aligned only
> with the enableOCSP checkbox. Your markup will (I think?) top-align them to
> the cert selection radiogroup. Is this a change that was discussed
> elsewhere, or just a mistake, or...? :-)

You would have needed to apply the patches from bug 1355522. I moved the buttons now so that they are aligned with the enableOCSP checkbox.

> ::: browser/components/preferences/in-content/privacy.xul:705
> (Diff revision 1)
> > -  <vbox>
> > +  <hbox align="center">
> > -    <caption>
> >      <checkbox id="submitHealthReportBox" label="&enableHealthReport.label;"
> >                accesskey="&enableHealthReport.accesskey;"/>
> > -  </caption>
> > +    <label id="FHRLearnMore"
> > -    <hbox class="indent" flex="1">
> > -      <label flex="1">&healthReportDesc.label;</label>
> > -      <label id="FHRLearnMore" flex="1"
> 
> Can you elaborate on what these changes are achieving?

These changes are to match the text-styling that is shown in the spec. Before this patch each checkbox was shown as a <caption> and thus was bolded, which doesn't match any other checkbox in the preferences.

> ::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd:10
> (Diff revision 1)
> >  <!ENTITY  trackingProtectionHeader2.label      "Tracking Protection">
> > +<!ENTITY  trackingProtection.description       "Tracking is when companies collect information about you to build a profile and display content based on your browsing and personal data.">
> > +<!ENTITY  trackingProtection.radioGroupLabel   "Block known tracking companies from displaying content">
> >  <!ENTITY  trackingProtectionAlways.label       "Always">
> >  <!ENTITY  trackingProtectionAlways.accesskey   "y">
> >  <!ENTITY  trackingProtectionPrivate.label      "Only in private windows">
> 
> The spec says this needs to be updated, too.

I disagreed with the spec here. We only use "Private Browsing mode" in two places within our UI (Custom history settings "Always use Private Browsing mode" and in the quit dialog when in a private window. I'd like to discuss more with UX about their request here and in the meantime I'd rather not make this change.

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8858223 [details]
Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting.

https://reviewboard.mozilla.org/r/130190/#review133056

r=me to unblock this as I haven't noticed any issue, but my review is nowhere near as detailed as what Gijs did in comment 5, so feel free to re-review after the week-end Gijs.

Comment 9

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

Comment on attachment 8858223 [details]
Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting.

https://reviewboard.mozilla.org/r/130190/#review132976

> I wish we just hid this if we knew NSS had no personal certificates. Unrelated to this bug, of course, but if we're simplifying prefs anyway...

I filed bug 1356656 for this.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39a46a779d51
Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting. r=florian

Comment 11

[:Gijs (he/him)]

Comment on attachment 8858223 [details]
Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting.

https://reviewboard.mozilla.org/r/130190/#review133080

Sorry, I didn't realize we were in a rush, and technically I'm supposed to be on PTO today. Anyway.

::: browser/components/preferences/in-content/privacy.xul:713
(Diff revision 2)
> -          <checkbox id="submitTelemetryBox" preference="toolkit.telemetry.enabled"
> +      <checkbox id="submitTelemetryBox" preference="toolkit.telemetry.enabled"
> -                    label="&enableTelemetryData.label;"
> +                label="&enableTelemetryData.label;"
> -                    accesskey="&enableTelemetryData.accesskey;"/>
> +                accesskey="&enableTelemetryData.accesskey;"/>
> -        </caption>
> +      <label id="telemetryLearnMore"
> -        <hbox class="indent" flex="1">
> -          <label id="telemetryDataDesc" flex="1">&telemetryDesc.label;</label>
> -          <label id="telemetryLearnMore" flex="1"
> -                 class="learnMore text-link">&telemetryLearnMore.label;</label>
> +             class="learnMore text-link">&telemetryLearnMore.label;</label>

If the label string for the checkbox binding is long (in, say, German), does it ever wrap, or do we just end up with a horizontal scrollbar? And what happens with the learn more link in that case?

This to say that it would probably be more appropriate to have the label as a separate element that wraps the checkbox and includes the learn more link, or something like that, but I realize that that is more work as far as styling it correctly (making wrapped lines align at an indent). If I'm right that the behaviour here is suboptimal (and I might not be, it's almost midnight here and I'm tired), then perhaps that would be fodder for a followup.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/39a46a779d51

Comment 13

[Md. Majedul islam]

I have seen the previous strings in firefox nightly 55.0a1 (2017-04-12)on Windows 10(64Bit). 


I can verify that the strings has been fixed in latest nightly 55.0a1 .

Build ID 	20170420030346
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[Bugday-20170419]