Closed Bug 1445694 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Settings UI, enhancement, P3)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

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

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.
59 bytes, text/x-review-board-request
flod
: review+
markh
: review+
stas
: 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/sync.xul
Assignee: nobody → gandalf
Blocks: 1415730
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

This patch covers what I'd like to land here now.

It basically migrates the whole XUL and all of relevant JS for sync.xul/sync.js.

It leaves out a single complex DTD entity that is waiting for DOM Overlays 2 and we'll migrate it in April (and remove sync.dtd then).

Flod - Regular request to look through the l10n changes and preferences.ftl structure and l10n ids.

Gijs - I made one non-trivial change here that I'd like to run through you before I move on.

The UI used a <label/> with class="fxaEmailAccount" which was populated and updated from the JS.

It was used in three places: first as a standalone, and then in two places inside a sentence.

I moved the standalone to use an ID and I moved the sentences to use the email address as an argument.

Let me know if that seems reasonable to you.
Attachment #8958944 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8958944 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review235720

::: browser/locales/en-US/browser/preferences/preferences.ftl:484
(Diff revision 3)
> +## Sync Section - Signed out
> +
> +sync-signedout-caption = Take Your Web With You
> +sync-signedout-description = Synchronize your bookmarks, history, tabs, passwords, add-ons, and preferences across all your devices.
> +
> +sync-signedout-account-title = Connect with a { -fxaccount-brand-name }

Really excited to be able to use attributes on this term. The current Italian localization looks off.

::: browser/locales/en-US/browser/preferences/preferences.ftl:486
(Diff revision 3)
> +sync-signedout-caption = Take Your Web With You
> +sync-signedout-description = Synchronize your bookmarks, history, tabs, passwords, add-ons, and preferences across all your devices.
> +
> +sync-signedout-account-title = Connect with a { -fxaccount-brand-name }
> +sync-signedout-account-create = Don’t have an account? Get started
> +    .accesskey = C

c (lowercase)

::: browser/locales/en-US/browser/preferences/preferences.ftl:513
(Diff revision 3)
> +    .label = Resend Verification
> +    .accesskey = d
> +
> +sync-remove-account =
> +    .label = Remove Account
> +    .accesskey = p

I flagged this in bug 1444162, it will likely need update

::: browser/locales/en-US/browser/preferences/preferences.ftl:533
(Diff revision 3)
> +    .accesskey = r
> +
> +sync-engine-tabs =
> +    .label = Open tabs
> +    .tooltiptext = A list of what’s open on all synced devices
> +    .accesskey = T

t (lowercase)

::: browser/locales/en-US/browser/preferences/preferences.ftl:562
(Diff revision 3)
> +        { PLATFORM() ->
> +            [windows] Options
> +           *[other] Preferences
> +        }
> +    .tooltiptext = General, Privacy, and Security settings you’ve changed
> +    .accesskey = S

s (lowercase)

::: browser/locales/en-US/browser/preferences/preferences.ftl:575
(Diff revision 3)
> +sync-device-name-cancel =
> +    .label = Cancel
> +    .accesskey = n
> +
> +sync-device-name-save =
> +    .label = SAve

Save
Attachment #8958944 - Flags: feedback?(francesco.lodolo) → feedback+
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review235962

The JS stuff looks OK to me, yeah. Might be good to get someone familiar with sync to do the actual review, also to spread some fluent knowledge, some review load, and because I'm out all of tomorrow and some of Monday? :-)

::: browser/themes/shared/incontentprefs/preferences.inc.css:611
(Diff revision 3)
> -.fxaEmailAddress {
> +#fxaEmailAddress {
>    margin-inline-end: 8px !important;
>  }

This will no longer match the in-sentence uses (which I guess is probably a *good* thing?). It also changes specificity, which I think won't practically make any difference. Just checking if you've looked at this...
Attachment #8958944 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review235962

> This will no longer match the in-sentence uses (which I guess is probably a *good* thing?). It also changes specificity, which I think won't practically make any difference. Just checking if you've looked at this...

It seems to me (code archeology!) that originally there was only one element with the class and the CSS was for it. Then we started using the same class to refresh the email and looping over it and the CSS remained.

I think that new behavior is what should happen, but I'll let Mark decide :)
Mark - the patch is ready. It should be applied on top of the patch from bug 1446180, and you don't have to worry about the ftl migration script (the python code at the end). This script is there to allow us to migrate all existing translations and preserve the localizers hard work! :)

I touched the JS code in a couple places as Gijs pointed out, and the rest are rather 1-1 migrations of DTD to Fluent.

Flod - the patch is fairly regular except of two strings that use the `$email` external argument. Due to the concatenation that was performed before, I'm injecting a space before and after the argument, but that unfortunately adds an extraneous leading or trailing white space.

I think it's ok since a) we skip them anyway and b) HTML then skips them anyway, but if you think it may be an issue we may want to do something pretty non-trivial in the migration script to detect if the before/after pieces are empty.
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review237216

I haven't looked at the code, but there's definitely an issue with the migration. For example, Italian results in

sync-signedin-unverified = { " " }{ $email } non è verificato.
sync-signedin-login-failure = Accedi per riattivare la connessione { $email }{ " " }

Because either .pre or .post are empty, but there's the space you add around $email. Is it too naive to trim the resulting concatenated string, before serializing it? Not even sure it's possible.

Migration for -fxaccount-brand-name is also missing (from browser/chrome/browser/syncBrand.dtd:syncBrand.fxAccount.label)

::: browser/locales/en-US/browser/branding/sync-brand.ftl:11
(Diff revision 4)
>  
>  # “Sync” can be localized, “Firefox” must be treated as a brand,
>  # and kept in English.
>  -sync-brand-name = Firefox Sync
> +
> +-fxaccount-brand-name = Firefox Account

Add a note similar as Sync: “Account” can be localized, etc.

::: python/l10n/fluent_migrations/bug_1445694_preferences_sync.py:17
(Diff revision 4)
> +def migrate(ctx):
> +    """Bug 1445694 - Migrate Preferences::Sync to Fluent, part {index}."""
> +
> +    ctx.add_transforms(
> +        'browser/browser/preferences/preferences.ftl',
> +        'browser/locales/en-US/browser/preferences/preferences.ftl',

Drop locales/en-US
Attachment #8958944 - Flags: review?(francesco.lodolo) → review-
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review237398

This looks fine to me in general (and obviously Flod's comments must be addressed). I'd r+ this except for the fact I'm scared about what our UX person will do to me if we change the style of any elements. Would it be possible to get a screenshot of before and after?

::: browser/components/preferences/in-content/sync.js:291
(Diff revision 4)
>        // resolve itself)
>        fxaLoginStatus.selectedIndex = FXA_LOGIN_VERIFIED;
>        syncReady = true;
>      }
>      fxaEmailAddressLabels.forEach((label) => {
> -      label.value = state.email;
> +      label.setAttribute("data-l10n-args", JSON.parse({email: state.email}));

it looks like this should be JSON.stringify()?

::: browser/themes/shared/incontentprefs/preferences.inc.css:607
(Diff revision 4)
>  
>  #fxaDisplayName {
>    margin-inline-end: 10px !important;
>  }
>  
> -.fxaEmailAddress {
> +#fxaEmailAddress {

Doesn't this change mean that only 1 element now gets this style, whereas previously a few did? Our UX person gets upset at the pixel level ;)
Attachment #8958944 - Flags: review?(markh)
Stas - is there a way migration script could help us with the trailing/leading whitespaces?

Basically the problem is that we're migrating:

DTD:
key1.before = 
key2.after =

XUL:

<description>
  &key1.before;
  <label/>
  &key1.after;
</description>

to:

FTL:
key1 = before { $email } after

using the following migration:

```
            FTL.Message(
                id=FTL.Identifier('sync-signedin-unverified'),
                value=CONCAT(
                    COPY(
                        'browser/chrome/browser/preferences/sync.dtd',
                        'signedInUnverified.beforename.label',
                    ),
                    FTL.TextElement(' '),
                    EXTERNAL_ARGUMENT('email'),
                    FTL.TextElement(' '),
                    COPY(
                        'browser/chrome/browser/preferences/sync.dtd',
                        'signedInUnverified.aftername.label',
                    ),
                ),
```

which produces: `sync-signedin-unverified = { " " }{ $email } non è verificato.`

but if I remove the `FTL.TextElement` insertions, I'll get `{ $email }non è verificato.`
Flags: needinfo?(stas)
We'll need some kind of conditional CONCAT for this to work. I filed bug 1449509, feel free to set it as a dependency if you think it's blocking.
Flags: needinfo?(stas)
Depends on: 1449509
(In reply to Mark Hammond [:markh] from comment #11)
> Comment on attachment 8958944 [details]
> Bug 1445694 - Migrate the "Sync" section of Preferences to the new
> Localization API.
> 
> https://reviewboard.mozilla.org/r/227800/#review237398
> 
> This looks fine to me in general (and obviously Flod's comments must be
> addressed). I'd r+ this except for the fact I'm scared about what our UX
> person will do to me if we change the style of any elements. Would it be
> possible to get a screenshot of before and after?

That's actually tricky as the elements in question are only shown in certain states. Ed, would it be possible for you to check whether the CSS changes in this patch matter in practice?
Flags: needinfo?(eoger)
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review237644

::: browser/themes/shared/incontentprefs/preferences.inc.css:607
(Diff revision 4)
>  
>  #fxaDisplayName {
>    margin-inline-end: 10px !important;
>  }
>  
> -.fxaEmailAddress {
> +#fxaEmailAddress {

Actually we can remove that rule completly, it is overriden by [0] anyway.

[0] https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/toolkit/themes/shared/in-content/common.inc.css#110-115
Flags: needinfo?(eoger)
This is still blocked on bug 1449509, but the rest of the patch should be ready for your review with all feedback applied.
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review238636

The changes here look fine to me, but I'll leave the finer points of l10n-specific changes to :flod.

::: browser/components/preferences/in-content/sync.xul:139
(Diff revision 5)
>          <groupbox id="syncOptions">
> -          <caption><label>&signedIn.settings.label;</label></caption>
> -          <description>&signedIn.settings.description;</description>
> +          <caption><label data-l10n-id="sync-signedin-settings-header"/></caption>
> +          <description data-l10n-id="sync-signedin-settings-desc"/>
>            <hbox id="fxaSyncEngines">
>              <vbox flex="1">
>                <!-- by design, no tooltip for bookmarks or history -->

It might make sense to copy/move this comment to the relevant section in preferences.ftl

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd:5
(Diff revision 5)
> -<!ENTITY signedOut.accountBox.signin2.accesskey "I">
> -
> -<!ENTITY signedIn.settings.label       "Sync Settings">
> -<!ENTITY signedIn.settings.description "Choose what to synchronize on your devices using &brandShortName;.">
> -
>  <!-- LOCALIZATION NOTE (mobilePromo3.*): the following strings will be used to

Is there a followup to remove these too?
Attachment #8958944 - Flags: review?(markh) → review+
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review238684

Still a couple of nits to fix, I'd like to see the diff before r+

Should we also discuss about moving those two strings blocked by migration in a follow-up bug?

::: browser/locales/en-US/browser/preferences/preferences.ftl:547
(Diff revision 5)
> +    .tooltiptext = Names, numbers and expiry dates (desktop only)
> +    .accesskey = C
> +
> +sync-engine-addons =
> +    .label = Add-ons
> +    .tooltiptext = Extensions and themes for { -brand-short-name } desktop

Looks like the original string has hard coded Firefox by design: https://bugzilla.mozilla.org/show_bug.cgi?id=1395453#c5

I noticed because the migration is "broken", trying to migrate a &brandShortName; that doesn't exist in translations, and generating a string with a warning. I would also expect this to show up when testing en-US to en-US migration?

::: python/l10n/fluent_migrations/bug_1445694_preferences_sync.py:17
(Diff revision 5)
> +def migrate(ctx):
> +    """Bug 1445694 - Migrate Preferences::Sync to Fluent, part {index}."""
> +
> +    ctx.add_transforms(
> +        'browser/browser/branding/sync-brand.ftl',
> +        'browser/locales/en-US/browser/branding/sync-brand.ftl',

Drop locales/en-US

::: python/l10n/fluent_migrations/bug_1445694_preferences_sync.py:31
(Diff revision 5)
> +        ]
> +    )
> +
> +    ctx.add_transforms(
> +        'browser/browser/preferences/preferences.ftl',
> +        'browser/locales/en-US/browser/preferences/preferences.ftl',

Drop locales/en-US (this was already reported as an issue in rev4).

::: python/l10n/fluent_migrations/bug_1445694_preferences_sync.py:403
(Diff revision 5)
> +                        FTL.Identifier('tooltiptext'),
> +                        REPLACE(
> +                            'browser/chrome/browser/preferences/sync.dtd',
> +                            'engine.addons.title',
> +                            {
> +                                '&brandShortName;': MESSAGE_REFERENCE('-brand-short-name')

This needs to be updated (replacing 'Firefox) or removed.
Attachment #8958944 - Flags: review?(francesco.lodolo) → review-
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review238636

> It might make sense to copy/move this comment to the relevant section in preferences.ftl

I don't think there's a value of having it in the L10n resource. Localizers cannot choose which attributes to localize. Flod?
I updated the patch to feedback hardcoding the "Firefox" term. I also wrote a custom extension of `CONCAT` transform helper which adds " " when concatenating.

Flod - can you review the code bar the CONCAT_WITH_WS part? I set r?stas for this one.

Stas - I know the code is not pretty, but it handles the use case. I'll leave it to your judgement if it's better to use it (and update it when we'll be landing updated python-fluent) or wait for bug 1449509.
Flod pointed out on IRC that he's using master python-fluent, so I updated the CONCAT_WITH_WS to extend the new logic. Should be testable now!

Still up to Stas if he prefers to use the one-off or get this into python-fluent.
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review238804

I've tested the migration with a couple of locales (it, cs) combining empty/translation before and after, and it seems to work fine.

I'll leave a proper review to stas for that code. On one side I'm a bit concerned about having this piece of code untested, but I'd also like to unblock this bug, since otherwise it's ready to land at this point.
Attachment #8958944 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review238636

> I don't think there's a value of having it in the L10n resource. Localizers cannot choose which attributes to localize. Flod?

I agree with zibi. I think this is mostly a concern for devs looking at the code, and wondering if the lack of tooltips is a mistake.
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review238850

I think it's OK to add a new transform in the migration recipe if it makes things easier. That said, I think we should follow two rules when doing so:

  1. Either make the transform generic enough so that it can be used as a base for the upstream implementation of a similar helper in python-fluent. In this case CONCAT_WITH_WS looks like it might want to do that (e.g. by using len(self.elements), but in fact it will not work for any other use-case than this bug's. It makes hard-wired assumptions on the order of the pattern elements it's passed. If you want to follow this patch, please implement PREPEND and APPEND from bug 1449509.

  2. Or, make the transform very very specific. For instance, call it CONCAT_BEFORE_AFTER(before, email, after) and have it accept exactly three arguments and even add an assert to make sure it's not being run in any other manner. In that case you can avoid checking the index and len().

::: python/l10n/fluent_migrations/bug_1445694_preferences_sync.py:14
(Diff revision 7)
> +from fluent.migrate import COPY, CONCAT, REPLACE
> +from fluent.migrate.transforms import Transform
> +
> +class CONCAT_WITH_WS(CONCAT):
> +    def __call__(self, ctx):
> +        for i in range(len(self.elements)):

Python's enumerate will give you both the index and the element. However, if you decide to follow rule #2 from above, I think it's better to make this code explicitly accept only three arguments (before, email, after) which will also have the side-effect of avoiding the need for the i and len() checks.

::: python/l10n/fluent_migrations/bug_1445694_preferences_sync.py:15
(Diff revision 7)
> +from fluent.migrate.transforms import Transform
> +
> +class CONCAT_WITH_WS(CONCAT):
> +    def __call__(self, ctx):
> +        for i in range(len(self.elements)):
> +            # Skip non-patterns

What do you mean by `skip` here? These elements still end up being concatenated, don't they?
Attachment #8958944 - Flags: review?(stas) → review-
(In reply to Staś Małolepszy :stas from comment #27)

> If you want to follow this patch, please implement PREPEND and APPEND from bug 1449509.

Typo: "…follow this path…".
(In reply to Francesco Lodolo [:flod] from comment #26)
> Comment on attachment 8958944 [details]
> Bug 1445694 - Migrate the "Sync" section of Preferences to the new
> Localization API.
> 
> https://reviewboard.mozilla.org/r/227800/#review238636
> 
> > I don't think there's a value of having it in the L10n resource. Localizers cannot choose which attributes to localize. Flod?
> 
> I agree with zibi. I think this is mostly a concern for devs looking at the
> code, and wondering if the lack of tooltips is a mistake.

My issue is that the developers looking at the XUL code have no idea from the code around the comment that some elements don't have tooltips. It's when the developers are editing the en-US version of the .ftl that they may wonder if the lack of tooltips is a mistake.
Thanks stas! I updated the patch to use the (2) approach.

>  It's when the developers are editing the en-US version of the .ftl that they may wonder if the lack of tooltips is a mistake.

Hmm, interesting. I don't think we have any way to handle developer-targeted comments in the FTL file. They're currently all displayed in the localization tools as hints on how to translate content. Maybe it's something we should consider?
Comment on attachment 8958944 [details]
Bug 1445694 - Migrate the "Sync" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227800/#review239118

::: python/l10n/fluent_migrations/bug_1445694_preferences_sync.py:16
(Diff revision 8)
> +
> +# Custom extension of the CONCAT migration tailored to concat
> +# two strings separated by a placeable.
> +class CONCAT_BEFORE_AFTER(CONCAT):
> +    def __call__(self, ctx):
> +        assert len(self.elements) == 3

Since the assert guarantees self.elements has exactly three arguments, we can destruct it like the following to make it a bit more self-documenting:

    pattern_before, placeable, pattern_after = self.elements
    elem_before = pattern_before.elements[0]
    elem_after = pattern_after.elements[0]

::: python/l10n/fluent_migrations/bug_1445694_preferences_sync.py:23
(Diff revision 8)
> +        if isinstance(elem0, FTL.TextElement) and elem0.value[-1] != " ":
> +            elem0.value += " "
> +        elem2 = self.elements[2].elements[0]
> +        if isinstance(elem2, FTL.TextElement) and elem2.value[0] != " ":
> +            elem2.value = " " + elem2.value
> +        return Transform.pattern_of(*self.elements)

Maybe use `super` here to avoid importing `Transform` and also keep this forward-compatible with upstream changes to `CONCAT`?
Attachment #8958944 - Flags: review?(stas) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a5788b2cbb8
Migrate the "Sync" section of Preferences to the new Localization API. r=flod,markh,stas
https://hg.mozilla.org/mozilla-central/rev/0a5788b2cbb8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Depends on: 1451685
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: