1446180 - Migrate the "Privacy" section of Preferences to the new Localization API

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

This bug will cover the migration work required to move the "Sync" section of Preferences to Fluent.

URL: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/privacy.xul

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

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

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

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

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

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

Comment 4

[Francesco Lodolo [:flod]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review235600

::: browser/locales/en-US/browser/preferences/preferences.ftl:455
(Diff revision 3)
> +
> +## Privacy Section
> +
> +privacy-header = Browser Privacy
> +
> +## Privacy Section - Passwords 

Remove the trailing whitespace

::: browser/locales/en-US/browser/preferences/preferences.ftl:455
(Diff revision 3)
> +
> +## Privacy Section
> +
> +privacy-header = Browser Privacy
> +
> +## Privacy Section - Passwords 

If we have to pick one work for the IDs of this group, I'd prefer "forms" over "passwords". Only one of the items apply to passwords, all the rest applies to forms.

::: browser/locales/en-US/browser/preferences/preferences.ftl:467
(Diff revision 3)
> +    .label = Exceptions…
> +    .accesskey = x
> +passwords-saved-logins =
> +    .label = Saved Logins…
> +    .accesskey = L
> +    .searchkeywords = { passwords-saved-logins.label }

Is it technically necessary to expose this attribute or a choice?

::: browser/locales/en-US/browser/preferences/preferences.ftl:591
(Diff revision 3)
> +# This string is displayed if privacy.trackingprotection.ui.enabled is set to false.
> +# This currently happens on the release and beta channel.
> +tracking-pbm-label = Use Tracking Protection in Private Browsing to block known trackers
> +
> +tracking-pbm-checkbox =
> +    .accesskey = v

What's this checkbox without a label?

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228200/diff/3-4/

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review235600

> Is it technically necessary to expose this attribute or a choice?

Nope, removed!

> What's this checkbox without a label?

Idk, but that's what the original code does, so I kept it! https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/browser/components/preferences/in-content/privacy.xul#325

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228200/diff/4-5/

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

This seems to be the last large patch in Preferences! It's big, but fairly simple, very heavily focused on 1-1 migration from XUL+DTD to Fluent, very little JS logic.

I'm leaving:

1) One DTD string combo in privacy.dtd which uses concatenation of complex widgets to build a sentence. I'd like to resolve it in context of Fluent with my teammates and then migrate.
2) A bunch of searchkeywords (both in JS and XUL) that use the strings needsd by the sub-dialogs. I'll migrate that when I'll be migrating the dialogs.
3) A couple of the same strings used to set the title of the subdialog. I'll also migrate that when I'll be migrating the dialogs

The rest looks fairly straightforward, so I hope we can land it early in the cycle. I tested migrations and opening the dialogs and couldn't find any regression so far.

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review235728

::: browser/components/preferences/in-content/privacy.xul:318
(Diff revision 5)
>        <vbox id="trackingProtectionPBMBox" flex="1">
>          <hbox id="trackingProtectionPBMExtensionContentLabel" align="center" hidden="true">
>            <description control="disableTrackingProtectionExtension" flex="1"/>
>          </hbox>
>          <hbox align="start">
>            <checkbox id="trackingProtectionPBM"

Here there are 2 different elements: a checkbox with an accesskey, and a label
https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/browser/components/preferences/in-content/privacy.xul#323-326

In a DTD world this generates something that makes sense
https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/browser/locales/en-US/chrome/browser/preferences/privacy.dtd#19-20

I understand that you don't want to add more code than necessary, but this 
doesn't make any sense in Fluent. We have a stray access key, that you don't know how to localize because the label is defined elsewhere.

This should be a checkbox with label and accesskey attributes.

::: browser/locales/en-US/browser/preferences/preferences.ftl:585
(Diff revision 5)
> +
> +addressbar-suggest = When using the address bar, suggest
> +
> +addressbar-locbar-history-option =
> +    .label = Browsing history
> +    .accesskey = H

h (lowercase)

::: browser/locales/en-US/browser/preferences/preferences.ftl:611
(Diff revision 5)
> +tracking-mode-private =
> +    .label = Only in private windows
> +    .accesskey = l
> +tracking-mode-never =
> +    .label = Never
> +    .accesskey = n

N (uppercase)

::: browser/locales/en-US/browser/preferences/preferences.ftl:647
(Diff revision 5)
> +permissions-microphone = Microphone
> +permissions-microphone-settings =
> +    .label = Settings…
> +    .accesskey = t
> +
> +permissions-notification = Notofications

Notifications

::: browser/locales/en-US/browser/preferences/preferences.ftl:721
(Diff revision 5)
> +    .accesskey = B
> +security-enable-safe-browsing-link = Learn more
> +
> +security-block-downloads =
> +    .label = Block dangerous downloads
> +    .accesskey = D

d (lowercase)

::: browser/locales/en-US/browser/preferences/preferences.ftl:725
(Diff revision 5)
> +    .label = Block dangerous downloads
> +    .accesskey = D
> +
> +security-block-uncommon-software =
> +    .label = Warn you about unwanted and uncommon software
> +    .accesskey = C

c (lowercase)

::: python/l10n/fluent_migrations/bug_1446180_preferences_privacy.py:391
(Diff revision 5)
> +                attributes=[
> +                    FTL.Attribute(
> +                        FTL.Identifier('label'),
> +                        COPY(
> +                            'browser/chrome/browser/preferences/privacy.dtd',
> +                            'acceptThirdParty3.always.label',

acceptThirdParty.always.label

::: python/l10n/fluent_migrations/bug_1446180_preferences_privacy.py:403
(Diff revision 5)
> +                attributes=[
> +                    FTL.Attribute(
> +                        FTL.Identifier('label'),
> +                        COPY(
> +                            'browser/chrome/browser/preferences/privacy.dtd',
> +                            'acceptThirdParty3.visited.label',

acceptThirdParty.visited.label

::: python/l10n/fluent_migrations/bug_1446180_preferences_privacy.py:415
(Diff revision 5)
> +                attributes=[
> +                    FTL.Attribute(
> +                        FTL.Identifier('label'),
> +                        COPY(
> +                            'browser/chrome/browser/preferences/privacy.dtd',
> +                            'acceptThirdParty3.never.label',

acceptThirdParty.never.label

::: python/l10n/fluent_migrations/bug_1446180_preferences_privacy.py:1186
(Diff revision 5)
> +                    ),
> +                    FTL.Attribute(
> +                        FTL.Identifier('accesskey'),
> +                        COPY(
> +                            'browser/chrome/browser/preferences/advanced.dtd',
> +                            'selectCerts.ask,accesskey',

selectCerts.ask.accesskey

Comment 10

[Francesco Lodolo [:flod]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review235864

::: python/l10n/fluent_migrations/bug_1446180_preferences_privacy.py:18
(Diff revision 5)
> +def migrate(ctx):
> +    """Bug 1446180 - Migrate Preferences::Privacy to Fluent, part {index}."""
> +
> +    ctx.add_transforms(
> +        'browser/browser/preferences/preferences.ftl',
> +        'browser/locales/en-US/browser/preferences/preferences.ftl',

At this point this should be `browser/browser/preferences/preferences.ftl`, given bug 1448068

Comment 11

[Francesco Lodolo [:flod]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review235866

::: browser/locales/en-US/browser/preferences/preferences.ftl:551
(Diff revision 5)
> +
> +sitedata-keep-until = Keep until
> +    .accesskey = u
> +
> +sitedata-keep-until-expire =
> +    .label = they expire

`They expire`, uppercase T (per bug 1447975)

::: browser/locales/en-US/browser/preferences/preferences.ftl:553
(Diff revision 5)
> +    .accesskey = u
> +
> +sitedata-keep-until-expire =
> +    .label = they expire
> +sitedata-keep-until-close =
> +    .label = I close { -brand-short-name }

Per bug 1447975, this should be come `{ -brand-short-name } is closed`.

::: python/l10n/fluent_migrations/bug_1446180_preferences_privacy.py:354
(Diff revision 5)
> +                        ),
> +                    ),
> +                ],
> +            ),
> +            FTL.Message(
> +                id=FTL.Identifier('sitedata-keep-until-close'),

We need to remove this migration, since the string changes from bug 1447975

Comment 12

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

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review236798

::: browser/components/preferences/in-content/privacy.xul:76
(Diff revision 5)
>      <!-- Please don't remove the wrapping hbox/vbox/box for these elements. It's used to properly compute the search tooltip position. -->
>      <hbox>
>        <menulist id="historyMode">
>          <menupopup>
> -          <menuitem label="&historyHeader.remember.label;" value="remember" searchkeywords="&rememberDescription1.label;"/>
> -          <menuitem label="&historyHeader.dontremember.label;" value="dontremember" searchkeywords="&dontrememberDescription.label;"/>
> +          <menuitem
> +            label="&historyHeader.remember.label;"

Are you going to replace these label attributes? Here and immediately below.

::: browser/components/preferences/in-content/privacy.xul:320
(Diff revision 5)
> -                    accesskey="&trackingProtectionPBM6.accesskey;"/>
> -          <label id="trackingProtectionPBMLabel" flex="1">&trackingProtectionPBM6.label;</label>
> +                    data-l10n-id="tracking-pbm-checkbox"/>
> +            <label id="trackingProtectionPBMLabel" flex="1" data-l10n-id="tracking-pbm-label"/>

The accesskey should be moved from the checkbox to the label, and the label needs to set the `control` attribute to the checkbox ID.

::: browser/locales/en-US/browser/preferences/preferences.ftl:687
(Diff revision 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 =
> +    .label = Allow { -brand-short-name } to send technical and interaction data to Mozilla

I believe we should be using vendorShortName here and a few other places instead of Mozilla.

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review236798

> Are you going to replace these label attributes? Here and immediately below.

Not in this patch. This is a fairly tricky case since the DOM Overlay here won't be trivial. My plan is to leave this single use of DTD here and fix it in a couple weeks together with several other remaining DTD uses once we finalize the new DOM Overlays API.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228200/diff/5-6/

Comment 15

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

Okay, that's fine. Please fix the indentation there at the least :)

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228200/diff/6-7/

Comment 17

[Francesco Lodolo [:flod]]

(In reply to [slow to respond 3/26-3/30] Jared Wein [:jaws] (please needinfo? me) from comment #12)
> ::: browser/locales/en-US/browser/preferences/preferences.ftl:687
> (Diff revision 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 =
> > +    .label = Allow { -brand-short-name } to send technical and interaction data to Mozilla
> 
> I believe we should be using vendorShortName here and a few other places
> instead of Mozilla.

I think this was done on purpose, because telemetry can only be used to send data to Mozilla?

Comment 18

[Francesco Lodolo [:flod]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review237086

::: python/l10n/fluent_migrations/bug_1446180_preferences_privacy.py:356
(Diff revisions 5 - 7)
>                          ),
>                      ),
>                  ],
>              ),
>              FTL.Message(
>                  id=FTL.Identifier('sitedata-keep-until-expire'),

We should remove this migration, to make sure the string is "retranslated" uppercase like the new one

Comment 19

[Francesco Lodolo [:flod]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review237094

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228200/diff/7-8/

Comment 21

[Francesco Lodolo [:flod]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review237096

::: browser/locales/en-US/browser/preferences/preferences.ftl:547
(Diff revisions 5 - 7)
>  sitedata-keep-until = Keep until
>      .accesskey = u
>  
>  sitedata-keep-until-expire =
> -    .label = they expire
> +    .label = They expire
>  sitedata-keep-until-close =

mozreview ate my comment because of the flaky connection: should we rename this to sitedata-keep-until-closed?

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228200/diff/8-9/

Comment 23

[Francesco Lodolo [:flod]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/228200/#review237202

Pending answer about Mozilla -> -vendor-short-name. I feel we need an hard-coded Mozilla for those, in particular for the one about sending browser errors in Nightly.

Even if we end up not using the new term, let's keep it in brand.ftl with the migration, it's going to come useful anyway in the near future.

::: browser/branding/aurora/locales/en-US/brand.ftl:5
(Diff revision 9)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  ## Firefox Brand

I think we should update this comment to mention Mozilla.

```
Firefox and Mozilla Brand

Firefox and Mozilla must be…
They cannot be:
…
```

Comment 24

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

(In reply to Francesco Lodolo [:flod] (workweek until Mar 30, slower than usual) from comment #17)
> (In reply to [slow to respond 3/26-3/30] Jared Wein [:jaws] (please needinfo? me) from comment #12)
> > I believe we should be using vendorShortName here and a few other places
> > instead of Mozilla.
> 
> I think this was done on purpose, because telemetry can only be used to send
> data to Mozilla?

This was changed by https://bugzilla.mozilla.org/show_bug.cgi?id=1365133#c241 (comment 241 shows it clearly) but there is no mention that it was intentional in that bug and it could have just been an oversight. Are you saying that a redistributed and rebranded version of Firefox cannot possibly send telemetry to another organization?

Comment 25

[Francesco Lodolo [:flod]]

(In reply to [slow to respond 3/26-3/30] Jared Wein [:jaws] (please needinfo? me) from comment #24)
> (In reply to Francesco Lodolo [:flod] (workweek until Mar 30, slower than
> usual) from comment #17)
> > (In reply to [slow to respond 3/26-3/30] Jared Wein [:jaws] (please needinfo? me) from comment #12)
> > > I believe we should be using vendorShortName here and a few other places
> > > instead of Mozilla.
> > 
> > I think this was done on purpose, because telemetry can only be used to send
> > data to Mozilla?
> 
> This was changed by
> https://bugzilla.mozilla.org/show_bug.cgi?id=1365133#c241 (comment 241 shows
> it clearly) but there is no mention that it was intentional in that bug and
> it could have just been an oversight. Are you saying that a redistributed
> and rebranded version of Firefox cannot possibly send telemetry to another
> organization?

Mine was more a question, I don't really know the answer to that.

If it was &vendorShortName; before (from bug 809094), I assume that's not the case, and we should keep the patch as it is, replacing Mozilla with -vendor-short-name.

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228200/diff/9-10/

Comment 27

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8959361 [details]
Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228200/diff/10-11/

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

Unfortunately, I had to bring back security.dtd with a single entity used in toolkit's aboutRights.xhtml. Will have to tackle that when we'll be redoing aboutRights.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f4319fc1bd2
Migrate the "Privacy" section of Preferences to the new Localization API. r=flod,jaws

Comment 30

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/6f4319fc1bd2