1424682 - Migrate the chrome of Preferences to the new Localization API

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

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

Part of bug 1415733 focused on the outer chrome of Preferences UI.

Comment 1

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

Ugh, mean bug 1415730.

Comment 2

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

Attachment: Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 3

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 4

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 5

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Stas, Flod - can I get your feedback on the use of Fluent in this patch?

Comment 6

[Francesco Lodolo [:flod]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review216424

This is a first quick pass exclusively on the FTL file, I'll probably need to take another look with a fresh brain.

focusSearch1.key is still in the DTD. Expected?

::: browser/locales/en-US/browser/preferences/preferences.ftl:19
(Diff revision 3)
> +        [win] Options
> +       *[other] Preferences
> +    }
> +
> +# This is used to determine the width of the search field in about:preferences,
> +# in order to make entire placeholder string visible

nit: the entire placeholder…

::: browser/locales/en-US/browser/preferences/preferences.ftl:28
(Diff revision 3)
> +pane-search-results
> +    .title = Search Results
> +
> +pane-general-title = General
> +category-general
> +    .tooltiptext = { pane-general-title }

I wonder if this is going to bite us, as in locales translating the reference (I've seen people translating $num, here we don't even have the $ to make it more evident it's a placeholder).

We used to use this approach in Firefox OS, so I hope this will be OK. We'll need to educate a lot of localizers.

::: browser/locales/en-US/browser/preferences/preferences.ftl:41
(Diff revision 3)
> +    .tooltiptext = { pane-privacy-title }
> +
> +pane-containers
> +    .title = Container Tabs
> +
> +# This should use the formal brand name of Firefox Account

I don't think this comment is helping, we should drop it.

"Firefox Account" is likely not going to end up in a common branding file either, since the "account" part can be localized, and it becomes a nightmare to manage. 

E.g. "Account Firefox" in Italian at the beginning of a sentence, but "account Firefox" in the middle, with an apostrophe if there's an article before ("l’account Firefox").

Maybe we should just add a localization note like:

# The word "account" can be translated, do not translate or transliterate "Firefox".

::: browser/locales/en-US/browser/preferences/preferences.ftl:65
(Diff revision 3)
> +revert-no-restart-button = Revert
> +restart-later = Restart Later
> +
> +## Extension Impact Notifications
> +##
> +## Those strings are used to inform the user about the changes extensions make to

nit: these strings

I think passive form is more straightforward to understand for a non native speaker. Maybe better:

These strings are used to inform the user about changes made by extensions to browser settings.

::: browser/locales/en-US/browser/preferences/preferences.ftl:68
(Diff revision 3)
> +## Extension Impact Notifications
> +##
> +## Those strings are used to inform the user about the changes extensions make to
> +## the user selected settings.
> +##
> +## The <image/> is going to be an icon of the extension

<image/> is going to be replaced by the extension icon

::: browser/locales/en-US/browser/preferences/preferences.ftl:73
(Diff revision 3)
> +## The <image/> is going to be an icon of the extension
> +##
> +## Variables:
> +##   $name (String): name of the extension
> +
> +# This string is shown to notify the user that their home page is being controlled by an extension.

I think we can drop individual comments on these strings, now that we have section comments (well, we need Pontoon's support)

::: browser/locales/en-US/browser/preferences/preferences.ftl:74
(Diff revision 3)
> +##
> +## Variables:
> +##   $name (String): name of the extension
> +
> +# This string is shown to notify the user that their home page is being controlled by an extension.
> +extension-controlled-homepage_override = An extension, <image/> { $name }, controls your home page.

I think we should avoid mixing dashes and underscores, and use only dashes.

::: browser/locales/en-US/browser/preferences/preferences.ftl:77
(Diff revision 3)
> +
> +# This string is shown to notify the user that their home page is being controlled by an extension.
> +extension-controlled-homepage_override = An extension, <image/> { $name }, controls your home page.
> +
> +# This string is shown to notify the user that their new tab page is being controlled by an extension.
> +extension-controlled-newTabURL = An extension, <image/>{ $name }, controls your New Tab page.

Missing space after placeholder.

I think we agreed on using dashes instead of camel case
https://bugzilla.mozilla.org/show_bug.cgi?id=1411012#c45

::: browser/locales/en-US/browser/preferences/preferences.ftl:85
(Diff revision 3)
> +# by an extension.
> +extension-controlled-defaultSearch = An extension, <image/> { $name }, has set your default search engine.
> +extension-controlled-privacy-containers = An extension, <image/> { $name }, requires Container Tabs.
> +
> +# This string is shown to notify the user how to enable an extension that they disabled.
> +extension-controlled-enable = To enable the extension go to <image/> Add-ons in the <image/> menu.

There are two <image/> here, how do you deal with languages inverting the order. Also, the section comment doesn't apply anymore, and the individual localization comment doesn't help.

Comment 7

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

> I think we should avoid mixing dashes and underscores, and use only dashes.

I can't do that here. The JS code that creates those message IDs uses underscore here, and the common part uses dashes. Changing that would require a code refactor and I'm trying to keep any code changes to the minimum :(

> There are two <image/> here, how do you deal with languages inverting the order.

As far as I understand the order will be preserved so no way to invert it using the current DOM Overlays. :stas may know more.

Comment 8

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Axel - can you also skim through the patch? General feedback at this point, any ideas on things we should make sure we're ready for or things we should localize differently.

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review218170

I have a couple of comments, in addition to what flod said. Based on his comments, I'll go with an r- for now.

Given the state of the PLATFORM() selector in pontoon, we'll need a bug on that, or avoid using PLATFORM for now.

::: browser/components/preferences/in-content/preferences.js:486
(Diff revision 3)
>    let image = document.createElement("image");
>    image.setAttribute("src", addon.iconURL || defaultIcon);
>    image.classList.add("extension-controlled-icon");
>    description.appendChild(image);
> -  description.appendChild(document.createTextNode(` ${addon.name}`));
> -  description.appendChild(document.createTextNode(stringParts[1]));
> +  document.l10n.setAttributes(description,
> +    `extension-controlled-${settingName.replace(".", "-")}`, { name: addon.name });

just add `/[\._]/` here instead "." to get rid of the underscore in the msg id?

::: browser/locales/en-US/browser/preferences/preferences.ftl:28
(Diff revision 3)
> +pane-search-results
> +    .title = Search Results
> +
> +pane-general-title = General
> +category-general
> +    .tooltiptext = { pane-general-title }

Should bug 1376747 block this?

Comment 10

[Francesco Lodolo [:flod]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> > There are two <image/> here, how do you deal with languages inverting the order.
> 
> As far as I understand the order will be preserved so no way to invert it
> using the current DOM Overlays. :stas may know more.

Stas confirmed it over IRC, but I'd still like to understand if we already know when that's going to be possible (i.e. we know what's required to make it happen, in terms of time and resources).

(In reply to Axel Hecht [:Pike] from comment #9)
> Should bug 1376747 block this?

Probably yes.

Comment 11

[Staś Małolepszy :stas]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review217750

LGTM. I created https://wiki.mozilla.org/L20n/Localizable_XUL_Attributes to start an inventory of data-l10n-attrs used during the porting effort.  Once we've collected more occurences we can consider changes to the default whitelist.

::: browser/components/preferences/in-content/preferences.js:346
(Diff revision 3)
>  const CONFIRM_RESTART_PROMPT_RESTART_LATER = 2;
> -function confirmRestartPrompt(aRestartToEnable, aDefaultButtonIndex,
> +async function confirmRestartPrompt(aRestartToEnable, aDefaultButtonIndex,
>                                aWantRevertAsCancelButton,
>                                aWantRestartLaterButton) {
> -  let brandName = document.getElementById("bundleBrand").getString("brandShortName");
> -  let bundle = document.getElementById("bundlePreferences");
> +  let [
> +    msg, title, button0Text, button1Text, button2Text

How about using more descriptive names for the button variables: `restartButtonText`, `noRestartButtonText` and `restartLaterButtonText` perhaps?

::: browser/locales/en-US/browser/preferences/preferences.ftl:14
(Diff revision 3)
>  do-not-track-option-always
>      .label = Always
> +
> +pref-page
> +    .title = { PLATFORM() ->
> +        [win] Options

I would prefer if PLATFORM() returned longer names: windows, macos, linux, android, ios, etc,

::: browser/locales/en-US/browser/preferences/preferences.ftl:21
(Diff revision 3)
> +    }
> +
> +# This is used to determine the width of the search field in about:preferences,
> +# in order to make entire placeholder string visible
> +search-field
> +    .style = min-width: 15.4em

Should we mention that this is CSS and that `min-width` should not be translated verbatim?

::: browser/locales/en-US/browser/preferences/preferences.ftl:85
(Diff revision 3)
> +# by an extension.
> +extension-controlled-defaultSearch = An extension, <image/> { $name }, has set your default search engine.
> +extension-controlled-privacy-containers = An extension, <image/> { $name }, requires Container Tabs.
> +
> +# This string is shown to notify the user how to enable an extension that they disabled.
> +extension-controlled-enable = To enable the extension go to <image/> Add-ons in the <image/> menu.

I filed https://github.com/projectfluent/fluent.js/issues/128 to track the reordering edge-case in fluent-dom.

If reordering of images is desired right now, we could pass `addonIcon` and `toolbarIcon` as string-type arguments to the translation:

    extension-controlled-enable = To enable the extension go to { $addonIcon } Add-ons in the { $toolbarIcon } menu.
    
And then, in preferences.js:

    let addonIcon = icon("chrome://mozapps/skin/extensions/extensionGeneric-16.svg");
    let toolbarIcon = icon("chrome://browser/skin/menu.svg");
    document.l10n.setAttributes(
        description,
        "extension-controlled-enable",
        {addonIcon, toolbarIcon});

This is a work-around with the limitation of not allowing the localizer to translate any attributes of the inlined element (like the alt attribute). This limitation should be OK for this use-case however.

Comment 12

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 13

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

(In reply to Francesco Lodolo [:flod] from comment #6)
> focusSearch1.key is still in the DTD. Expected?

yes, this is used in a few other places still. :(

> I think we agreed on using dashes instead of camel case
> https://bugzilla.mozilla.org/show_bug.cgi?id=1411012#c45

I took Pike's advice and removed the dash, but removing the camel case is going to require a bit more changing around in JS. I'd prefer not to do this unless :jaws will see it fit.
 
> There are two <image/> here, how do you deal with languages inverting the
> order. Also, the section comment doesn't apply anymore, and the individual
> localization comment doesn't help.

I applied :stas' workaround.

Comment 14

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Thank you for all the feedback Stas, Flod, Pike.

I applied most of it, and now I'd like to get feedback from :jaws.

Jared - this is the first major patch transitioning the chrome of Preferences to use Fluent.

It depends on a couple of matches including an update to Fluent 0.5 which changes a few minor things in the syntax (introduces private messages and changes comment sygil).

I'll be testing it much more once we have the dependencies landed, and I'll ask you for a full review then, so for now I'd just like to get your feedback on the changes I'm introducing.

Since it's the first patch we're targeting, I'd also like to ask you to pay attention to your experience of reading the Fluent backed code and whether you find it an improvement over the DTD/.properties API/UX or not.
It's a good opportunity to identify things we can bring to Fluent to make your future work on Preferences easier :)

Comment 15

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 16

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Updated the patch to contain the latest change to the syntax - required `=` sign at the end of Message ID even when there's no value.

Re-requesting f?jaws

Comment 17

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #10)
> (In reply to Axel Hecht [:Pike] from comment #9)
> > Should bug 1376747 block this?
> 
> Probably yes.

For sake of moving this forward in a timely manner, I can also live with this being added to my personal QA scripts for a while.

Comment 18

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

I did the initial performance measurements using the mochitest designed by Myk for the de-XBL patch: https://github.com/mozilla/gecko/compare/central...mykmelez:about-preferences-load-perf

I ran it 4 times for central and 4 times for pref-chrome branch. Results:

mozilla-central:

Initial time to initialize prefs page: 328.3 (378.6 | 287.3 | 276.4 | 370.9)
Mean time to initialize prefs page: 209.0 (200.0 | 210.8 | 208.0 | 217.4)

pref-chrome:

Initial time to initialize prefs page: 334.25 (307.3 | 357.1 | 385.5 | 287.1)
Mean time to initialize prefs page: 213.2 (202.0 | 200.0 | 211.0 | 240.1)


This is very preliminary, since we have to wait for Fluent update to parse all the messages properly, but I think it's a good indication that at least according to the test we have (and we used to evaluate other Preferences refactor), we're in a good space.

Comment 19

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review220740

f+, thanks for working on this. Looks like it's off to a good start :)

::: browser/components/preferences/in-content/preferences.js:346
(Diff revision 5)
>                                aWantRevertAsCancelButton,
>                                aWantRestartLaterButton) {

nit, please fix the indentation here

::: browser/components/preferences/in-content/preferences.js:513
(Diff revision 5)
>    let image = document.createElement("image");
>    image.setAttribute("src", addon.iconURL || defaultIcon);
>    image.classList.add("extension-controlled-icon");
>    description.appendChild(image);
> -  description.appendChild(document.createTextNode(` ${addon.name}`));
> -  description.appendChild(document.createTextNode(stringParts[1]));
> +  document.l10n.setAttributes(description,
> +    `extension-controlled-${settingName.replace(/[\._]/, "-")}`, { name: addon.name });

Can we file a bug to clean up the naming schema here of the extension-controlled-* so we don't have to do this ugly conversion of periods and underscores to hyphens?

::: browser/components/preferences/in-content/preferences.xul:230
(Diff revision 5)
>                role="dialog"
>                aria-labelledby="dialogTitle">
>        <caption flex="1" align="center">
>          <label class="dialogTitle" flex="1"></label>
>          <button class="dialogClose close-icon"
> -                aria-label="&preferencesCloseButton.label;"/>
> +                data-l10n-id="close-button"/>

Is this missing a `data-l10n-attrs="aria-label"` attribute?

::: browser/locales/en-US/browser/preferences/preferences.ftl:12
(Diff revision 5)
> +pref-page =
> +    .title = { PLATFORM() ->
> +        [win] Options
> +       *[other] Preferences
> +    }

Wow this is crazy lol, up to this point we haven't had logic branches in our localization files.

How worried are we about people making a mistake and breaking the logic here?

Is there some documentation that lists all the macros such as PLATFORM() ?

::: browser/locales/en-US/browser/preferences/preferences.ftl:22
(Diff revision 5)
> +
> +# This is used to determine the width of the search field in about:preferences,
> +# in order to make the entire placeholder string visible
> +#
> +# Notice: The value of the `.style` attribute is a CSS string, and the `min-width`
> +# is the name of the CSS variable. It is intended only to adjust the element's width.

While you're here, please change "variable" to "property".

::: browser/locales/en-US/browser/preferences/preferences.ftl:42
(Diff revision 5)
> +pane-containers =
> +    .title = Container Tabs

I don't see where pane-containers is referenced.

::: browser/locales/en-US/browser/preferences/preferences.ftl:50
(Diff revision 5)
> +# The word "account" can be translated, do not translate or transliterate "Firefox".
> +pane-sync-title = Firefox Account
> +category-sync =
> +    .tooltiptext = { pane-sync-title }
> +
> +help-button-label = { -brand-short-name } Support

Where is `brand-short-name` defined? I don't see it on searchfox.

Comment 20

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

Thanks Jared!

> Can we file a bug to clean up the naming schema here of the extension-controlled-* so we don't have to do this ugly conversion of periods and underscores to hyphens?

Sure! Will do! Do you want me also to transform camel case to underscores in FTL and use a camelCaseToSnakeCase converter when calling for strings?

> Wow this is crazy lol, up to this point we haven't had logic branches in our localization files.

Hahaha, right? No more per-platform localization strings, no more need for ja-JP-mac, no more pre-processor branches, and also, for localizers who don't need per-platform words here, they can just provide an unbranched translation. :)

Welcome to XXI century!

> How worried are we about people making a mistake and breaking the logic here?

Not much. We're working on getting our GUI localization tool to handle PLATFORM and it's blocking this bug - bug 1431436.
We also have linters and solid error recovery (so, if someone would place wrong variant name, we'll just fallback on the default variant denoted with an asterisk. That also helps us in case we'll want to add more variants in the future!

> Is there some documentation that lists all the macros such as PLATFORM() ?

Here's a documentation request for our L10n Docs which also blocks this bug - https://github.com/mozilla-l10n/localizer-documentation/issues/103

> Where is `brand-short-name` defined? I don't see it on searchfox.

Being added in the dependency bug - bug 1424683.

I'll apply the rest of your feedback and run another round of feedback from Pike and Flod once we have this patch testable against m-c!

Comment 21

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 22

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

I added the migration code and would like to get :stas' feedback on its use.

There's also one message I'm not sure how to migrate - the `PLATFORM` selector. I only found `PLURAL` helper in the migrate code and I'm not sure how to add a general selector and how to assign two values from two DTD entities to its variants.

:flod, :pike - I'm also marking you for f? here because I think it's the final period to evaluate little details like message names etc.

Comment 23

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 24

[Francesco Lodolo [:flod]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review218182

I went through the migration code, but at this point I have no clue how to test it, given how many moving pieces there are (Fluent and compare-locales updates, Brand patch).

::: browser/locales/en-US/browser/preferences/preferences.ftl:75
(Diff revision 7)
> +##
> +## Variables:
> +##   $name (String): name of the extension
> +
> +extension-controlled-homepage-override = An extension, <image/> { $name }, controls your home page.
> +extension-controlled-newTabURL = An extension, <image/> { $name }, controls your New Tab page.

Not sure where we ended up on this.

Comment 20 talks about filing a bug to clean up the naming schema. The other strings will be fine if we do that, but this one (and -defaultSearch) won't, we'll need new strings.

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:230
(Diff revision 7)
>  # LOCALIZATION NOTE (featureEnableRequiresRestart, featureDisableRequiresRestart, restartTitle): %S = brandShortName
> -featureEnableRequiresRestart=%S must restart to enable this feature.
> -featureDisableRequiresRestart=%S must restart to disable this feature.
>  shouldRestartTitle=Restart %S
>  okToRestartButton=Restart %S now
>  revertNoRestartButton=Revert

Should these 3 strings be removed? I can't see other usage besides the code already removed.

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:211
(Diff revision 7)
> +                id=FTL.Identifier('extension-controlled-homepage-override'),
> +                value=REPLACE(
> +					'browser/chrome/browser/preferences/preferences.properties',
> +					'extensionControlled.homepage_override',
> +                    {
> +                        '%S': CONCAT(

I don't believe CONCAT() adds spaces, and the strings in preferences.ftl have a space between <image/> and \{ $name \}

Comment 25

[Francesco Lodolo [:flod]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

The only real doubt is about the 2 strings with camel case.

Comment 26

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #22)
> Comment on attachment 8937653 [details]
> Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.
> 
> I added the migration code and would like to get :stas' feedback on its use.
> 
> There's also one message I'm not sure how to migrate - the `PLATFORM`
> selector. I only found `PLURAL` helper in the migrate code and I'm not sure
> how to add a general selector and how to assign two values from two DTD
> entities to its variants.

Great question! The PLURAL transform is specifically designed to migrate old pluralized translations stored in a properties file. You give it a single legacy string to migrate from and it splits it and builds a valid select expression using the current locales plural categories as variant keys.

Here, you simply want to combine two strings into a single FTL message. The PLATFORM selector is new (it's not being migrated from legacy translations), so you'll need to recreate the complete AST for it. It's a bit verbose but the benfit of this approach is that you can simply refer to https://github.com/projectfluent/python-fluent/blob/master/fluent/syntax/ast.py to know what's possible.

I didn't test it, but something like the following should do the job:

    FTL.Message(
        id=FTL.Identifier("pref-pane"),
        value=FTL.Pattern(
            elements=[
                FTL.Placeable(
                    expression=FTL.SelectExpression(
                        expression=FTL.CallExpression(
                            callee=FTL.Identifier("PLATFORM")
                        ),
                        variants=[
                            FTL.Variant(
                                key=FTL.VariantName("windows"),
                                default=False,
                                value=COPY(
                                    'browser/chrome/browser/preferences/preferences.dtd',
                                    'prefWindow.titleWin'
                                )
                            ),
                            FTL.Variant(
                                key=FTL.VariantName("other"),
                                default=True,
                                value=COPY(
                                    'browser/chrome/browser/preferences/preferences.dtd',
                                    'prefWindow.title'
                                )
                            )
                        ]
                    )
                )
            ]
        )
    )

Comment 27

[Staś Małolepszy :stas]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review222644

::: browser/components/preferences/in-content/preferences.xul:196
(Diff revisions 5 - 6)
>          </label>
>        </hbox>
>      </vbox>
>  
>      <keyset>
> -      <key data-l10n-id="focus-search" modifiers="accel" id="focusSearch1" oncommand="gSearchResultsPane.searchInput.focus();"/>
> +      <key data-l10n-id="focus-search" data-l10n-attrs="key" modifiers="accel" id="focusSearch1" oncommand="gSearchResultsPane.searchInput.focus();"/>

This shouldn't be required; the key attribute is allowed on <key> elements by default: https://github.com/projectfluent/fluent.js/blob/24585e681edb28c04f0d43e8ceb6621205803def/fluent-dom/src/overlay.js#L38

::: browser/components/preferences/in-content/preferences.xul:230
(Diff revisions 5 - 6)
>                role="dialog"
>                aria-labelledby="dialogTitle">
>        <caption flex="1" align="center">
>          <label class="dialogTitle" flex="1"></label>
>          <button class="dialogClose close-icon"
> +                data-l10n-attrs="aria-label"

This also shouldn't be required because aria-label is allowed on all elements by default: https://github.com/projectfluent/fluent.js/blob/24585e681edb28c04f0d43e8ceb6621205803def/fluent-dom/src/overlay.js#L36

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:65
(Diff revision 6)
> +            FTL.Message(
> +                id=FTL.Identifier('category-general'),
> +                attributes=[
> +                    FTL.Attribute(
> +                        FTL.Identifier('tooltiptext'),
> +                        MESSAGE_REFERENCE('pane-general-title'),

This needs to be a valid attribute value:

    FTL.Pattern(
        elements=[
            FTL.Placeable(
                expression=FTL.MessageReference(
                    id=FTL.Identifier(
                        'pane-general-title'
                    )
                )
            )
        ]
    )
    
You can also choose to use the MESSAGE_REFERENCE helper defined in https://github.com/projectfluent/python-fluent/blob/7b80850df59d969742672df9373e2ccf58aec4c7/fluent/migrate/helpers.py#L24.

    FTL.Pattern(
        elements=[
            FTL.Placeable(
                expression=MESSAGE_REFERENCE(
                    'pane-general-title'
                )
            )
        ]
    )

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:175
(Diff revision 6)
> +            ),
> +            FTL.Message(
> +                id=FTL.Identifier('should-restart-title'),
> +                value=REPLACE(
> +					'browser/chrome/browser/preferences/preferences.properties',
> +					'shouldRestartTitle',

Some locales (mai, ne-NP) put a trailing space in this string for some reason. Is this intentional?

If so, we might need to make bug 1374246 a dependency. I actually don't know what would happen with those trailing spaces right now; the tests for this are currently skipped:

https://github.com/projectfluent/python-fluent/blob/7b80850df59d969742672df9373e2ccf58aec4c7/tests/migrate/test_copy.py#L45-L71

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:185
(Diff revision 6)
> +            ),
> +            FTL.Message(
> +                id=FTL.Identifier('should-restart-ok'),
> +                value=REPLACE(
> +					'browser/chrome/browser/preferences/preferences.properties',
> +					'featureDisableRequiresRestart',

Should this read `okToRestartButton`?

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:211
(Diff revision 6)
> +                id=FTL.Identifier('extension-controlled-homepage-override'),
> +                value=REPLACE(
> +					'browser/chrome/browser/preferences/preferences.properties',
> +					'extensionControlled.homepage_override',
> +                    {
> +                        '%S' : '<image/> { $name }'

Interesting. I'm afraid this isn't supported right now. The target value which will replace `%S` can currently only be an FTL Expression and it will be wrapped in an FTL.Placeable: https://github.com/projectfluent/python-fluent/blob/7b80850df59d969742672df9373e2ccf58aec4c7/fluent/migrate/transforms.py#L171

Here, we'd rather need an tuple of a TextElement and a Placeable which will be concatented to the elements of the Pattern.

Maybe we should use the `$icon` workaround here as well?

If not, please file a bug in L20n > Python Library blocking bug 1367803 and this bug.

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:262
(Diff revision 6)
> +                value=REPLACE(
> +					'browser/chrome/browser/preferences/preferences.properties',
> +					'extensionControlled.enable',
> +                    {
> +                        '%1$S' : EXTERNAL_ARGUMENT('addonicon'),
> +                        '%2$S' : EXTERNAL_ARGUMENT('toolbarIcon'),

There's bug 1317336 about supporting different formats of Gecko placeables when translations don't use the exact same specifier as en-US. Looking at Transvision none of the translations migrated in this bug will require this feature, so we might not need to block on it.

Comment 28

[Staś Małolepszy :stas]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review222652

Comment 29

[Staś Małolepszy :stas]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review222654

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:211
(Diff revisions 6 - 7)
>                  id=FTL.Identifier('extension-controlled-homepage-override'),
>                  value=REPLACE(
>  					'browser/chrome/browser/preferences/preferences.properties',
>  					'extensionControlled.homepage_override',
>                      {
> -                        '%S' : '<image/> { $name }'
> +                        '%S': CONCAT(

Ah, it looks like I reviewed an outdated version of the patch.

This also won't work because CONCAT returns a Pattern while REPLACE expects the replacement values to be Expressions and wraps them in Placeables.

Comment 30

[Staś Małolepszy :stas]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

It looks like the REPLACE transform may be too limited to fully support what you want to achieve here. Removing the feedback request on me because this isn't really the fault of the patch.

Comment 31

[Francesco Lodolo [:flod]]

(In reply to Staś Małolepszy :stas from comment #27)
> Some locales (mai, ne-NP) put a trailing space in this string for some
> reason. Is this intentional?

Not really, that looks like a mistake, inherited from Pootle. Fixing both of them.

Comment 32

[Axel Hecht [:Pike]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review222666

Yeah to what flod and stas already said, but more importantly ...

::: browser/components/preferences/in-content/preferences.js:550
(Diff revision 7)
>    let toolbarIcon = icon("chrome://browser/skin/menu.svg");
> -  let message = document.getElementById("bundlePreferences")
> -                        .getString("extensionControlled.enable");
> -  let frag = BrowserUtils.getLocalizedFragment(document, message, addonIcon, toolbarIcon);
> -  elements.description.innerHTML = "";
> -  elements.description.appendChild(frag);
> +  let description = elements.description;
> +  document.l10n.setAttributes(
> +    description,
> +    "extension-controlled-enable",
> +    {addonIcon, toolbarIcon});

I chatted a bit with stas, and this doesn't work.

Even if you outerHTML this, it won't work, https://github.com/projectfluent/fluent.js/blob/master/fluent-dom/test/overlay_test.js#L24. Whatever we do here should probably start with a test-case in fluent-dom.

We reasoned back and forth a bit, and didn't come up with a clear yes.

Comment 33

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 34

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

(In reply to Francesco Lodolo [:flod] from comment #24)
> Not sure where we ended up on this.
> 
> Comment 20 talks about filing a bug to clean up the naming schema. The other
> strings will be fine if we do that, but this one (and -defaultSearch) won't,
> we'll need new strings.

Comment 20 talks about filing a bug to clean the "_" vs "-". Not about cleaning camelCase.
I asked Jared if he'd like me to add a code that converts things like "NewTabURL" to "new-tab-url". Let's wait for his call.


I updated the patch to your feedback.

If I understand correctly we have two issues at the moment without a clear resolution:

1) The `'%S' : '<image/> { $name }'` replacement doesn't work in the migration code
2) The `doc.l10n.setAttributes(..., {addonIcon, toolbarIcon});` doesn't work in the runtime code

If there's anything else I didn't list here, please, flag me ASAP.

Comment 35

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

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20)
> Thanks Jared!

Sorry, I didn't see these questions because I wasn't flagged for needinfo. Please flag me in the future if you have a question. It was luck that I saw you mention my name in comment #34 ;)

> > Can we file a bug to clean up the naming schema here of the extension-controlled-* so we don't have to do this ugly conversion of periods and underscores to hyphens?
> 
> Sure! Will do! Do you want me also to transform camel case to underscores in
> FTL and use a camelCaseToSnakeCase converter when calling for strings?

It's OK with me, but let's make sure that we have buy-in from the add-ons people who have been adding these strings. @bsilverberg, are you OK with the naming convention changes?

Comment 36

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

(sorry didn't mean to flag you for needinfo on this)

Comment 37

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 38

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 39

[Code Review Bot [:reviewbot]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review223166


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/preferences.js:499
(Diff revision 10)
>  }
>  
> +
> +function settingNameToL10nID(settingName) {
> +  // 1) We replace NewTabURL to new-tab-uRL
> +  let name = settingName.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase();

Error: Strings must use doublequote. [eslint: quotes]

Comment 40

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review220740

> Can we file a bug to clean up the naming schema here of the extension-controlled-* so we don't have to do this ugly conversion of periods and underscores to hyphens?

Filed bug 1435168

Comment 41

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 42

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

Ok, I think I fixed the problem (2) from comment 34, and I filed bug 1434998 to tackle issue (1).

I believe the patch is now ready for review.

Comment 43

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

:jaws - in order to review it, you need to apply patches from bug 
1424683 and bug 1426063 before this one.

If you want I can build a single combined patch for your testing.

We'd like to target this patch for landing by the end of the next week, so if you prefer to wait for the two patches to land, I hope to land them early next week.

Comment 44

[Staś Małolepszy :stas]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review223218

::: browser/locales/en-US/browser/preferences/preferences.ftl:79
(Diff revision 11)
> +extension-controlled-privacy-containers = An extension, <image/> { $name }, requires Container Tabs.
> +extension-controlled-tracking-protection-mode = An extension, <image/> { $name }, is controlling tracking protection.
> +
> +# The first <image/> is an icon for the Add-ons menu, the second is an icon
> +# for the toolbar menu.
> +extension-controlled-enable = To enable the extension go to <image/> Add-ons in the <image/> menu.

This brings us back to a situation where the two icons can't be reordered. Looking at https://transvision.mozfr.org/string/?entity=browser/chrome/browser/preferences/preferences.properties:extensionControlled.enable&repo=gecko_strings, there are a few locales which need to switch the order of the images in this sentence (I counted 7).

This also has another problem: fluent-dom doesn't recognize the `image` element as localizable as per https://github.com/projectfluent/fluent.js/blob/master/fluent-dom/src/overlay.js. Actually, it currently doesn't allow any XUL elements at all. Consequently, during sanitization it will replace any child XUL elements found in the translation with their textContent.

Pike suggested another workaround: we could rephrase the message to:

    extension-controlled-enable = To enable the extension go to <span title="1"/> Add-ons in the <span title="2"/> menu.
    
Spans are HTML elements which are always allowed in translations and title is a safe global attribute which can be localized as well.

Then, in the JS code we could replace the spans with the corresponding icons via something like `element.querySelector("span[title=1]")` and `replaceChild`. The challenge with this approach, however, is that it's not clear _when_ to run this JS. `setAttributes` relies on the MutationObserver and the translation happens asynchronously.

Another approach might be to add `image` as an allowed XUL element in https://github.com/projectfluent/fluent.js/blob/master/fluent-dom/src/overlay.js#L10 but not allow the `src` attribute on it. It would make it pretty safe, although it would mean that any translation anywhere in XUL can insert a random `image` element.

This approach would also help with all the other `extension-controlled-*` messages.

Comment 45

[Axel Hecht [:Pike]]

Another though I hadn't shared with stas yet, I wonder if we need to replace the spans, or if we can use CSS to have them get a background image and size.

I'm a bit concerned about the retranslation part of the approaches here, too.

Comment 46

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

A quick update on comment 34:

> 1) The `'%S' : '<image/> { $name }'` replacement doesn't work in the migration code

:stas landed bug 1434998 which extends migration code to handle this case. We hope to release the new python-fluent on Mon/Tue and land it in tree by Wed.

> 2) The `doc.l10n.setAttributes(..., {addonIcon, toolbarIcon});` doesn't work in the runtime code

There are two pieces to fix this:

a) We'd like to enable <image/> to be an allowed XUL element in DOM Overlays. I filed bug 1435464 to track this.
b) We'd like to be able to reorder DOM Overlay elements. I wrote a PR in https://github.com/projectfluent/fluent.js/pull/143

Comment 47

[Francesco Lodolo [:flod]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review223476

Side question: migration is completely ignoring empty blank lines (the source has them, migrated file doesn't). Is that expected? That's going to create a lot of diff as soon as Pontoon writes to them.

::: browser/locales/en-US/browser/preferences/preferences.ftl:7
(Diff revision 11)
> -// 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/.
> +# 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/.
>  
>  do-not-track-description = Send websites a “Do Not Track” signal that you don’t want to be tracked
>  do-not-track-learn-more = Learn more
>  do-not-track-option-default

Add = on empty value for new syntax.

::: browser/locales/en-US/browser/preferences/preferences.ftl:9
(Diff revision 11)
>  
>  do-not-track-description = Send websites a “Do Not Track” signal that you don’t want to be tracked
>  do-not-track-learn-more = Learn more
>  do-not-track-option-default
>      .label = Only when using Tracking Protection
>  do-not-track-option-always

Add = on empty value for new syntax.

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:27
(Diff revision 11)
> +                value=FTL.Pattern(
> +                    elements=[
> +                        FTL.Placeable(
> +                            expression=FTL.SelectExpression(
> +                                expression=FTL.CallExpression(
> +                                    callee=FTL.Identifier("PLATFORM")

This doesn't work. Unfortunately the stack trace is pretty useless, besides indicating this line

TypeError: __init__() takes exactly 3 arguments (2 given)

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:57
(Diff revision 11)
> +            FTL.Message(
> +                id=FTL.Identifier('search-field'),
> +                attributes=[
> +                    FTL.Attribute(
> +                        FTL.Identifier('style'),
> +                        COPY(

This results in English having 

min-width: 15.4em

While localization only has

15.4em

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:243
(Diff revision 11)
> +                        '%S' : MESSAGE_REFERENCE('-brand-short-name')
> +                    }
> +                )
> +            ),
> +            FTL.Message(
> +                id=FTL.Identifier('revert-no-restart'),

Bug 1435181 FTW!

WARNING:migrate:Message "revert-no-restart" was not found in browser/locales/en-US/browser/preferences/preferences.ftl

This should be revert-no-restart-button

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:263
(Diff revision 11)
> +                value=REPLACE(
> +                    'browser/chrome/browser/preferences/preferences.properties',
> +                    'extensionControlled.homepage_override',
> +                    {
> +                        '%S': CONCAT(
> +                            FTL.TextElement('<image/>'),

All these patterns are failing with

RuntimeError: CONCAT accepts FTL Patterns, TextElements and Placeables.

I tried replacing the second element in CONCAT() with

    FTL.Placeable(EXTERNAL_ARGUMENT('name'))

But then I get a different error:

TypeError: sequence item 1: expected string or Unicode, NoneType found

I get the same error with something as simple as, so I assume the problem is with assigning CONCAT() to %S?

    FTL.Message(
        id=FTL.Identifier('extension-controlled-homepage-override'),
        value=REPLACE(
            'browser/chrome/browser/preferences/preferences.properties',
            'extensionControlled.homepage_override',
            {
                '%S': CONCAT(
                    FTL.TextElement('<a>'),
                    FTL.TextElement('</a>'),
                )
            }
        )
    ),

Comment 48

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

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

Comment 49

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

Thanks Flod! I was able to perform the migration and updated the patch to fix things you pointed out.

Two things left wrt. migration:

1) The error you noticed was because we don't support calling `FTL.CallExpression` without arguments. Since it's going to be fairly common, I filed a PR to add it - https://github.com/projectfluent/python-fluent/pull/42 - if :stas agrees, he'll take it for 0.6.1. If not, I'll add it to the patch.

2) Your general point stands - we don't add empty lines between messages.

I'm taking off r? for :jaws, to give us a chance to solve the <image/> and ordering problem.

Comment 50

[Francesco Lodolo [:flod]]

I can confirm that migration works correctly now (tried Italian with the patched Fluent). 

As said, the only point is about the missing empty lines compared to the source file. Personally, I'm fine with it, but Pontoon won't.

Comment 51

[Staś Małolepszy :stas]

(In reply to Francesco Lodolo [:flod] from comment #47)

> Side question: migration is completely ignoring empty blank lines (the
> source has them, migrated file doesn't). Is that expected? That's going to
> create a lot of diff as soon as Pontoon writes to them.

This is because the result of the migration is serialized by FluentSerializer which doesn't know anything about the whitespace between messages. Some time in the future there might be a way to used compare-locales merging algorithm here if preserving whitespace becomes an important requirement. I don't see us having time for this right now.

> All these patterns are failing with
> 
> RuntimeError: CONCAT accepts FTL Patterns, TextElements and Placeables.

I landed a small fix this morning to make the API of CONCAT the same as that of REPLACE: https://github.com/projectfluent/python-fluent/commit/b40264348efe0dbad0565678533c24c21a2c4b0e. Right now both accept Patterns, PatternElements (TextElements and Placeables) and Expressions. I'm not sure if they should accept Expressions, however. I filed https://github.com/projectfluent/python-fluent/issues/43 to discuss this.

Comment 52

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #49)

> 1) The error you noticed was because we don't support calling
> `FTL.CallExpression` without arguments. Since it's going to be fairly
> common, I filed a PR to add it -
> https://github.com/projectfluent/python-fluent/pull/42 - if :stas agrees,
> he'll take it for 0.6.1. If not, I'll add it to the patch.

It's a bug in python-fluent (thanks for the PR) and fluent-syntax. As per the ASDL the CallExpression should be allowed to have no arguments in which case the arguments field should be an empty list. Cf. https://github.com/projectfluent/fluent/blob/69ecabcc06cf6f5b3654c439a9defc8575744313/spec/fluent.asdl#L52.

Comment 53

[Bob Silverberg [:bsilverberg]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20)
> 
> > > Can we file a bug to clean up the naming schema here of the extension-controlled-* so we don't have to do this ugly conversion of periods and underscores to hyphens?
> > 
> > Sure! Will do! Do you want me also to transform camel case to underscores in
> > FTL and use a camelCaseToSnakeCase converter when calling for strings?
> 
> It's OK with me, but let's make sure that we have buy-in from the add-ons
> people who have been adding these strings. @bsilverberg, are you OK with the
> naming convention changes?

The reason all of those strings are as they currently are (including the ones with dots in their names) is that the name of the string matches the name of the corresponding setting in the ExtensionSettingsStore, so ideally we'd still be able to refer to them using those same strings in the code we need in browser/components/preferences/in-content/. It looks like the new settingNameToL10nID function takes care of that. If that's the case then I have no concerns about changing the names of the strings.

I also want to point out that I have a patch in review for bug 1429593 which makes significant changes to some of the same files that will result in some conflicts, which one of us will have to resolve once the other patch lands. Perhaps we want to try to coordinate this? Any idea when this patch might land?

Comment 54

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

> which one of us will have to resolve once the other patch lands. Perhaps we want to try to coordinate this? Any idea when this patch might land?

I think you should land yours since your patch is more advanced and seems to have all dependencies resolved. I'm still tackling dependencies for this one.

Comment 55

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208354/diff/12-13/

Comment 56

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Updated:

 - removed the message with two <images/> from the migration leaving it for once we land the DOM Overlays feature
 - switched to use HTML <img/> tag
 - tested all the strings in the UI and the migration

At this point I believe the patch it ready to be reviewed, but would like to get the final approval from the l10n-drivers stakeholders before I ask :jaws to review it.

Comment 57

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

Since we're not ready, I'm going to set the explicit dependency on bug 1429593 so that Bob can land his patch first. I started refactoring my patch to match his work and it seems like our FTL won't be much affected.

On the other hand I posted an initial patch for General section (XUL part), which together should give us a better picture into how a big preferences.ftl will look like. (bug 1435912).

It's a good opportunity to iron out any FTL message ID conventions that we'd like to keep (for example, I noticed that I have tabCount and tabsCount there, and we sometimes put `id-label` and sometimes just `id` for <label data-l10n-id="X">).

Comment 58

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review223794

::: browser/locales/en-US/browser/preferences/preferences.ftl:66
(Diff revision 13)
> +## Extension Impact Notifications
> +##
> +## These strings are used to inform the user
> +## about changes made by extensions to browser settings.
> +##
> +## <image/> is going to be replaced by the extension icon

will fix in the next iteration

Comment 59

[Francesco Lodolo [:flod]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review223830

::: browser/locales/en-US/browser/preferences/preferences.ftl:12
(Diff revision 13)
> -do-not-track-option-default
> +do-not-track-option-default =
>      .label = Only when using Tracking Protection
> -do-not-track-option-always
> +do-not-track-option-always =
>      .label = Always
> +
> +pref-page =

Just noticed that this is indented differently in the output FTL file, might be worth using the same

pref-page =
    .title = { PLATFORM() ->
            [windows] Options
           *[other] Preferences
        }

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:271
(Diff revision 13)
> +                value=REPLACE(
> +                    'browser/chrome/browser/preferences/preferences.properties',
> +                    'extensionControlled.homepage_override',
> +                    {
> +                        '%S': CONCAT(
> +                            FTL.TextElement('<image/> '),

<img/> in all the migrations (not sure if your previous comment about fixing later included this)

Comment 60

[Axel Hecht [:Pike]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

I'll go ahead and say that flod's comments are an f-, hope that's OK.

Also, as a nit, I'd love if the use of " and ' was consistent in the python migration file. Not that I'm good at that in my own code, just noticed it searching in the code for stuff, though.

Comment 61

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208354/diff/13-14/

Comment 62

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Thanks, Flod, Pike. Updated to your feedback and requesting f? again.

Comment 63

[Staś Małolepszy :stas]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review223900

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:89
(Diff revision 14)
> +                        FTL.Pattern(
> +                            elements=[
> +                                FTL.Placeable(
> +                                    expression=FTL.MessageReference(
> +                                        id=FTL.Identifier(
> +                                            'pane-general-title'

This is quite verbose. You could replace it with:

    FTL.Message(
        id = FTL.Identifier('category-general'),
        attributes = [
            FTL.Attribute(
                FTL.Identifier('tooltiptext'),
                CONCAT(
                    MESSAGE_REFERENCE('pane-general-title')
                )
            )
        ]
    )

COPY, CONCAT, PLURALS and REPLACE always create new Patterns. A CONCAT with a single element can be used as a shortcut here because it accepts other Pattern, PatternElements (TextElements and Placeable) as well as Expressions as arguments.

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:273
(Diff revision 14)
> +                    'extensionControlled.homepage_override',
> +                    {
> +                        '%S': CONCAT(
> +                            FTL.TextElement('<img/> '),
> +                            FTL.Placeable(
> +                                EXTERNAL_ARGUMENT('name')

You don't need the FTL.Placeable here. I'd prefer either of the following two approaches:

    CONCAT(
        FTL.TextElement(),
        EXTERNAL_ARGUMENT())
    
or:

    CONCAT(
        FTL.TextElement(),
        FTL.Placeable(
            FTL.ExternalArgument(
                FTL.Identifier()))
                
it's up to you, but I think we should try to be consistent in how we use the helpers.

Comment 64

[Staś Małolepszy :stas]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

I think that we shouldn't be migrating any of the extension-controlled-* strings in this bug.

For these strings to work, we need bug 1435464, which I don't know if it makes sense in the context of the recent discussion in https://github.com/projectfluent/fluent.js/issues/128.

Also related to https://github.com/projectfluent/fluent.js/issues/128 is that whatever we come up with there will likely require changes to the extension-controlled-* strings. For instance, we might need to add data-l10n-name attributes to <img> in the translation to make it match the source elements.

I'm not sure if it's a good idea to 1) change fluent-dom temporarily knowing that we're already talking about the real solution and 2) migrate strings now only to have to change it when the real solution is available.

I don't want to f- the patch based on this because the reason are more on the project management side than the technical side.

Comment 65

[Francesco Lodolo [:flod]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

The FTL looks good to me, and migration works (both this version, and one patched with Stas' comments).

I totally see Stas' point though: if there is a chance that we have to change those strings with <img/>, maybe it's worth excluding them from this initial patch.

Comment 66

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208354/diff/14-15/

Comment 67

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Updated the patch:

 - Rebased on top of the just-landed bug 1429593
 - Applied feedback from stas
 - Coincidentally, the patch moves out extension-controlled-* strings out of preferences.js

Requesting another round of feedback

Comment 68

[Staś Małolepszy :stas]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review223922

What a lucky coincidence :) LGTM.

I built Nightly with this patch earlier this morning and I got a "translation is undefined" error in https://searchfox.org/mozilla-central/source/intl/l10n/DOMLocalization.jsm#76. Is it related to one of the dependencies still being open?

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:10
(Diff revision 15)
> +
> +from __future__ import absolute_import
> +import fluent.syntax.ast as FTL
> +from fluent.migrate.helpers import MESSAGE_REFERENCE, EXTERNAL_ARGUMENT
> +from fluent.migrate.transforms import REPLACE
> +from fluent.migrate import COPY, CONCAT

Supernit: `from fluent.migrate import CONCAT, COPY, REPLACE`.

::: python/l10n/fluent_migrations/bug_1424682_preferences_chrome.py:27
(Diff revision 15)
> +                attributes=[
> +                    FTL.Attribute(
> +                        FTL.Identifier('title'),
> +                        FTL.Pattern(
> +                            elements=[
> +                                FTL.Placeable(

You can use `CONCAT(FTL.SelectExpression())` here to reduce the indentation.

Comment 69

[Axel Hecht [:Pike]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review223984

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:305
(Diff revisions 14 - 15)
>  # This string is shown to notify the user how to enable an extension that they disabled.
>  extensionControlled.enable = To enable the extension go to %1$S Add-ons in the %2$S menu.
> +
> +# LOCALIZATION NOTE (connectionDesc.label):
> +# %S is the brandShortName from brand.properties (for example "Nightly")
> +connectionDesc.label = Configure how %S connects to the internet.

Do you want to convert this in this cycle, or should we push that out to the next? Or include when you update for bug 1429593?

Comment 70

[Axel Hecht [:Pike]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Aside of the question on rebasing, f=me.

Comment 71

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

Ok, now I think it's ready for your r? jaws.

There's a minor hiccup with bug 1429593 which changed two strings in a minor way, and unfortunately, the old system has no way to notify localizers without updating the ID.
I hope to change that with Fluent in the future, but for now we have to go there and update the ID, but I think it should not affect this patch (I'll rebase it once the changes land).

Please notice that we are still waiting for some tooling to get updated and then we'll have to land two other patches before this one, so in order to test it you still need to:

1) Apply the brand.ftl patch
2) Apply the PLATFORM patch
3) Apply this patch

Comment 72

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

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review225828

Comment 73

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 8 changes to 8 files
remote: (ftl_check check enabled per config override)
remote: 
remote: ************************ ERROR *************************
remote: You are trying to commit a change to an FTL file.
remote: At the moment modifying FTL files requires a review from
remote: one of the L10n Drivers.
remote: Please, request review from either:
remote:     - Francesco Lodolo (:flod)
remote:     - Zibi Braniecki (:gandalf)
remote:     - Axel Hecht (:pike)
remote:     - Stas Malolepszy (:stas)
remote: ********************************************************
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.mozhooks hook failed
abort: push failed on remote

Comment 74

[Francesco Lodolo [:flod]]

Comment on attachment 8937653 [details]
Bug 1424682 - Migrate the chrome of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/208354/#review226224

Comment 75

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e74601cabde
Migrate the chrome of Preferences to the new Localization API. r=flod,jaws

Comment 76

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7e74601cabde