1435875 - Localize about:studies

Status: VERIFIED FIXED

(Firefox :: Normandy Client, enhancement, P3)

Description

[Michael Cooper [:mythmon]]

From https://github.com/mozilla/normandy/issues/1083

The add-on's listing of opt-out studies, about:studies, isn't localizable. This gets us in trouble if we want to run studies with non-English populations. We should fix this.

Comment 1

[Francesco Lodolo [:flod]]

Hi Mike, any update on this? I've CCed zibi in case there are questions about the technology that is available.

Comment 2

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

This seems to be a regular react addon. They can use fluent-react package and bundle their l10n resources and that would be my recommendation (since Firefox is being move to Fluent it'll be easier for them to interoperate between Fx and their code)

CC'ing :stas in case I'm missing something.

Comment 3

[Michael Cooper [:mythmon]]

Francesco: Mostly we put this off until after bug 1436113 was done. Now that Normandy is a toolkit component instead of a system add-on, the approach Zibi mentioned should be much simpler, and we'll be doing that soon.

Comment 4

[Jonathan Kingston [:jkt] he/him]

I'm not convinced this is as simple as Comment 3 makes out.

- Old Webpack code that was hosted on github appears not to compile react-fluent even with much massaging
- No other system addon appears to bundle the code in this manner
- Including Services would require this `about:studies` to be moved to be privileged which hasn't happened I think for a reason.

Message passing to the framescript might be the simplest option assuming using a DTD injected into the page would be rejected as a solution.

Until other projects have used react/fluent I think the simplest solution should be taken here to unblock landing this for other translations.

Comment 5

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1435875.patch

As mentioned over IRC to :Gijs this isn't a great solution however it does solve the problem quickly. I'm going to try and use message passing instead.

Comment 6

[Francesco Lodolo [:flod]]

(In reply to Jonathan Kingston [:jkt] from comment #5)
> Created attachment 8973993 [details] [diff] [review]
> bug-1435875.patch
> 
> As mentioned over IRC to :Gijs this isn't a great solution however it does
> solve the problem quickly. I'm going to try and use message passing instead.

I assumed this is a WIP/POC, but note that the DTD/properties files need to be created as part of a patch. The code makes it look like you had those files, but didn't include them?

Having said, given that we're moving away from DTD and properties, it would be really sad to create a new about page using , to then migrate it to Fluent in a few months.

And some of that code won't work as it is, because it needs replacements, e.g. for brand names in JS.

Comment 7

[Jonathan Kingston [:jkt] he/him]

There was a DTD yeah I forgot to add it there though, however the properties file I have added to the latest patch.

> it would be really sad to create a new about page using , to then migrate it to Fluent in a few months.

I'm not sure the work in migrating properties files however there is only a handful of strings, I don't think we should delay on this however happy to change my mind on this. Given that I tried for several hours to get fluent to work in Comment 4 it doesn't seem possible to work without a lot more work.

> And some of that code won't work as it is, because it needs replacements, e.g. for brand names in JS.

Indeed, however these are currently broken anyway.

Comment 8

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1435875-a.patch

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 8974021 [details] [diff] [review]
bug-1435875-a.patch

Review of attachment 8974021 [details] [diff] [review]:
-----------------------------------------------------------------

There are a few things to fix in the .properties file, commenting mostly to make sure they're not overlooked.

> Indeed, however these are currently broken anyway.

Not sure I get this part.

::: toolkit/locales/en-US/chrome/global/aboutStudies.properties
@@ +2,5 @@
> +# 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/.
> +
> +title = Shield Studies
> +remove = Remove

Let's pick better IDs, e.g. removeButton

@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +title = Shield Studies
> +remove = Remove
> +active = Active

Add a comment, i.e.

# Displayed for an active study

@@ +4,5 @@
> +
> +title = Shield Studies
> +remove = Remove
> +active = Active
> +complete = Complete

Add a comment, i.e.

# Displayed for a study that is already complete

@@ +5,5 @@
> +title = Shield Studies
> +remove = Remove
> +active = Active
> +complete = Complete
> +updatePreferences = Update Preferences

This needs *nix vs Windows (Options) variants.

@@ +9,5 @@
> +updatePreferences = Update Preferences
> +learnMore = Learn more
> +noStudies = You have not participated in any studies.
> +disabledList = This is a list of studies that you have participated in. No new studies will run.
> +enabledList = What's this? Firefox may install and run studies from time to time.

What's -> What’s (apostrophe), otherwise it will fail tests.

Firefox should be %S, and replaced at run-time with brand name.

Comment 10

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1435875-a.patch

Comment 11

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1435875 - Make about:studies translatable

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

Comment 12

[Francesco Lodolo [:flod]]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

https://reviewboard.mozilla.org/r/242372/#review248308

Strings-wise this looks good, possibly one more comment to add.

Also confirmed with the rest of the team that going with .properties is the right approach at this point.

::: toolkit/locales/en-US/chrome/global/aboutStudies.properties:5
(Diff revision 1)
> +# 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/.
> +
> +title = Shield Studies

I wonder if we need a link explaining what Shield Studies are, and what to do with "Shield".

Maybe add a comment like this:

# LOCALIZATION NOTE (title): keep "Shield" in English. See
# https://wiki.mozilla.org/Firefox/Shield/Shield_Studies for more information

Comment 13

[Michael Cooper [:mythmon]]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

https://reviewboard.mozilla.org/r/242372/#review248366

The Normandy part of this, and specifically the React parts, look good to me. I'll leave comments about localization to :flod.

Comment 14

[:Gijs (out for now; he/him)]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

https://reviewboard.mozilla.org/r/242372/#review248476

Sorry for being the grouchy non-r+-y person. Notes below - I mean, this is probably mostly done, but the react thing is... crazy.

::: toolkit/components/normandy/content/about-studies/shield-studies.js:142
(Diff revision 1)
>            study.name.slice(0, 1)
>          ),
>          r("div", {className: "study-details"},
>            r("div", {className: "study-name"}, study.name),
>            r("div", {className: "study-description", title: study.description},
> -            r("span", {className: "study-status"}, study.active ? "Active" : "Complete"),
> +            r("span", {className: "study-status"}, study.active ? this.props.translations.activeStatus : this.props.translations.completeStatus),

I don't pretend to understand any of the react goop here, but why are we accessing these directly but added helpers in the other places? Can we be consistent?

Also, the helper deals with null, and these direct accesses do not. Is it not possible for `render` on the StudyList to be called without there being translations (thus passing `null` to any StudyListItem instances)? If not, why does that code need null-checks? If it is, why doesn't this code need null-checks?

(Also, why is this on `props` here and on `state` in the other bits?)

Also, it feels ugly that we're passing *all* the translations around (not just the ones the components need) and so it's basically global state that we keep passing to places. Can we just have a global helper that deals with this instead of the react stuff, or is that "un-react-y" ?

::: toolkit/components/normandy/content/shield-content-frame.js:75
(Diff revision 1)
> +        const strings = {};
> +        const stringKeys = ["title", "learnMore", "noStudies", "completeStatus", "removeButton", "activeStatus", "disabledList"];
> +        for (const string of stringKeys) {
> +          strings[string] = gStringBundle.GetStringFromName(string);
> +        }
> +        const brandName = gBrandBundle.GetStringFromName("brandShortName");
> +        strings.enabledList = gStringBundle.formatStringFromName("enabledList", [brandName], 1);
> +        const updateButtonKey = AppConstants.platform == "win"
> +                                ? "updateButtonWin"
> +                                : "updateButtonUnix";
> +        strings.updateButton = gStringBundle.GetStringFromName(updateButtonKey);

Instead of triplicating the information (strings file, about: page itself, message passing here with a list of keys), you can use `gStringBundle.getSimpleEnumeration()` to get all the strings. You may still need to have special-casing for the formatted one. You should be able to push the update button logic down into the page and use `navigator.platform` to figure out which one you need.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIStringBundle#getSimpleEnumeration()

Comment 15

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

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

Comment 16

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

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

Comment 17

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

https://reviewboard.mozilla.org/r/242372/#review248476

> I don't pretend to understand any of the react goop here, but why are we accessing these directly but added helpers in the other places? Can we be consistent?
> 
> Also, the helper deals with null, and these direct accesses do not. Is it not possible for `render` on the StudyList to be called without there being translations (thus passing `null` to any StudyListItem instances)? If not, why does that code need null-checks? If it is, why doesn't this code need null-checks?
> 
> (Also, why is this on `props` here and on `state` in the other bits?)
> 
> Also, it feels ugly that we're passing *all* the translations around (not just the ones the components need) and so it's basically global state that we keep passing to places. Can we just have a global helper that deals with this instead of the react stuff, or is that "un-react-y" ?

My understanding is this would be usually solved by something like redux or https://reactjs.org/docs/context.html but we don't have either. I'm going to make the t() method in StudyListItem also as this then keeps us uniform.

> (Also, why is this on props here and on state in the other bits?)

Mostly due to the amount of change that will happen in StudyListItem I am passing in the properties.

> why doesn't this code need null-checks?

Null checks shouldn't be needed for props as I can see however the state starts off null in the other cases.

Comment 18

[:Gijs (out for now; he/him)]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

https://reviewboard.mozilla.org/r/242372/#review248748

LGTM but please get mythmon to check the react-ness stuff.

::: toolkit/components/normandy/content/about-studies/shield-studies.js:197
(Diff revision 3)
> -      message = r("span", {}, "This is a list of studies that you have participated in. No new studies will run.");
> +      message = r("span", {}, translations.disabledList);
>      } else if (studiesEnabled === true) {
> -      message = r("span", {}, "What's this? Firefox may install and run studies from time to time.");
> +      message = r("span", {}, translations.enabledList);
>      }
>  
> +    const updateButtonKey = /Win/.test(navigator.platform) ? "updateButtonWin" : "updateButtonUnix";

Nit: use `.includes` instead of a regex.

Comment 19

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

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

Comment 20

[Jonathan Kingston [:jkt] he/him]

Hey could you check the latest changes as it's a fairly big change.

I moved the subscribing to the top of the component tree and used props all the way down. This removes more of the subscribing to simplify the code. This is simpler and actually is more React-y from what I can tell. I removed the t() methods as all the props are set by the time we get to the components. I also don't render the page until translations have loaded.

Comment 21

[Michael Cooper [:mythmon]]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

https://reviewboard.mozilla.org/r/242372/#review248948

I like the pattern of only holding state (requesting the translations) at the top level and passing it down better. Things like Redux are mostly just a formalization of that pattern. I think you should provide a default value for the translations state property, but besides that this looks good to me, and an improvement over what you had before.

::: toolkit/components/normandy/content/about-studies/about-studies.js:35
(Diff revision 4)
>      if (!PAGES.has(hash)) {
>        hash = "shieldStudies";
>      }
>  
>      this.state = {
>        currentPageId: hash,

For consistency, you should add a `translations` key to this object. maybe with a value of `{}`?

::: toolkit/components/normandy/content/about-studies/about-studies.js:78
(Diff revision 4)
>      const currentPage = PAGES.get(currentPageId);
> +    const { translations } = this.state;
>  
>      return (
>        r("div", {className: "about-studies-container"},
> -        r(Sidebar, {},
> +        translations && r(Sidebar, {},

If you used `{}` as a default value for translations, then you could simplify this section and always render the components. I'm not sure if that would be better or worse for loading time though.

Comment 22

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

You may also want to look at how fluent-react https://github.com/projectfluent/fluent.js/tree/master/fluent-react handles l10n of components, since at some point we'll want to migrate it to it.

Comment 23

[Jonathan Kingston [:jkt] he/him]

> You may also want to look at how fluent-react https://github.com/projectfluent/fluent.js/tree/master/fluent-react handles l10n of components, since at some point we'll want to migrate it to it.

I know this is a component and ID based solution so a little different. I don't think this would be a big amount of work to change over given the size of the page.

As mentioned earlier I tried and failed to integrate this into the page.
Mostly due to there is no prior art in M-C for react fluent, the build chain has been removed since this page became managed by M-C, trying to get the previous web pack compilation was impossible even after updating it and changing code.

I think after this work is finished we should clean up this page and add a build step back in as the React is outdated and shouldn't need to be minified like it is.

Comment 24

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

I'm not advocating for trying it now :) I think we'll want to land bug 1425104 first. I'm just advocating for keeping the approach close to make the transition easier later :)

Comment 25

[Jonathan Kingston [:jkt] he/him]

> I'm just advocating for keeping the approach close to make the transition easier later

I think the plan would be swap the .properties to be .ftl. Add in the Localized components and ids around each section.
The solutions are pretty different however I think trying to build the Localized component would be just as much work again (it's a small enough page I'm confident enough it could be done in a day or so).

Happy to take pointers if you had any (I don't do react at all however we would like this page translated this week really).

I am a little confused why the react solution chooses to have the Default language text duplication into the template however. This isn't how fluent is used in about:preferences.

Comment 26

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

I believe the reason we put strings in content is because that's closer to how React programming patterns work. In case of XUL/HTML we're less constrain. I may be wrong tho, I'm not very familiar with React. Setting NI on :stas who designed fluent-react.

Comment 27

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8974037 [details]
Bug 1435875 - Make about:studies translatable

https://reviewboard.mozilla.org/r/242372/#review248948

> If you used `{}` as a default value for translations, then you could simplify this section and always render the components. I'm not sure if that would be better or worse for loading time though.

I made this null instead as it would just move the complexity back into the component. I'm not expecting it would be any faster as the components would have to rerender anyway.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6bd58667ac1
Make about:studies translatable r=flod,Gijs,mythmon

Comment 29

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26)
> I believe the reason we put strings in content is because that's closer to
> how React programming patterns work. In case of XUL/HTML we're less
> constrain. I may be wrong tho, I'm not very familiar with React. Setting NI
> on :stas who designed fluent-react.

Putting strings inside of <Localized> components is not required. If present, they will be used as as fallback when translations are not available. In the future, they may also be used to extract the copy from JSX into FTL.

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/e6bd58667ac1

Comment 31

[Francesco Lodolo [:flod]]

Verified in 62.0a1 (2018-05-12) (64 bit)