Closed Bug 1446180 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1446180 - Migrate the "Privacy" section of Preferences to the new Localization API.
59 bytes, text/x-review-board-request
jaws
: review+
flod
: review+
Details 
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
Depends on: 1420761
Priority: -- → P1
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 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
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
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 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
Attachment #8959361 - Flags: review?(francesco.lodolo)
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 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 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.
Attachment #8959361 - Flags: review?(jaws) → review+
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.
Okay, that's fine. Please fix the indentation there at the least :)
Blocks: 1447978
(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?
Flags: needinfo?(jaws)
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
Attachment #8959361 - Flags: review?(francesco.lodolo)
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
Attachment #8959361 - Flags: review?(francesco.lodolo)
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 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:
…
```
Attachment #8959361 - Flags: review?(francesco.lodolo) → review+
(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?
Flags: needinfo?(jaws)
Flags: needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)
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.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f4319fc1bd2
Migrate the "Privacy" section of Preferences to the new Localization API. r=flod,jaws
https://hg.mozilla.org/mozilla-central/rev/6f4319fc1bd2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Depends on: 1455686
Depends on: 1514503
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: