1351873 - Gecko-side localisation (e.g. context menu) broken in Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Comment 1

[Flore Allemandou [:flore]]

Attachment: Screenshot_20170329-235754.png

I'm using a French version of nightly and often use a long press on links to trigger this menu and as of today (55.0a1(2017-03-29)), it's in english. On yesterday's version it was correctly localized.

Comment 2

[Jan Henning [:JanH]]

[Tracking Requested - why for this release]: Context menu localisation is broken

This is how we set the context menu strings: https://dxr.mozilla.org/mozilla-central/rev/272ce6c2572164f5f6a9fba2a980ba9ccf50770c/mobile/android/chrome/content/browser.js#608

Given how this simply uses Strings.browser.GetStringFromName(), are any other Gecko strings affected as well?

If I've got this right, this should be the push log from the first 2017-03-28 to the last 2017-03-29 build:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5182b2c4b963ed87d038c7d9a4021463917076cd&tochange=272ce6c2572164f5f6a9fba2a980ba9ccf50770c,

so bug 1347306 would be a prime suspect. Zibi, do you think this might be the case?

Comment 3

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

Thank you for reporting it! I'm investigating.

Comment 4

[Marcia Knous [:marcia]]

Tracking 55+ for this regression.

Comment 6

[Clément Lefèvre]

As an addition, just in case those weren't noticed, are also affected the "report an issue" item in the main menu, and the whole About page page.

Comment 7

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

yup, as the updated bug title suggests - seems like all gecko strings are affected. I'm debugging.

We transitioned from ChromeRegistry to LocaleService as a central mechanism for language selection and it seems that Fennec's Java side didn't pick it up.

The fix should be easy, but I need to first find where it fails.

Comment 8

[Richard Newman [:rnewman]]

Note that on Android, the Android-side locale mechanism is in control of choosing a locale -- either by picking a locale in Android Settings, or using the built-in locale picker in Fennec -- which it reflects to Gecko when the locale changes.

See BrowserLocaleManager.java, which sends Locale:Changed and Locale:OS to browser.js.

I believe you set the handling for Locale:Changed to call Services.locale.setRequestedLocales. Bear in mind that Locale:Change is only sent once. I don't see any interaction with Services.locale in the handling for Locale:OS.

Comment 9

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

Ok, I think I figured out what happens.

After bug 1347306 landed, we negotiate languages in LocaleService, and ChromeRegistry returns LocaleService negotiated locale when asked for "GetSelectedLocale" [0].

So in order for the system to work, we need to make sure that LocaleService can negotitate languages properly. That happens when LocaleService asks [1] ChromeRegistry for available languages (which it then negotiates against requested ones from the user).
This code asks for all available locales in package "global".

Unfortunately, Fennec does not register package "global" for all locales. It only register package "global" for en-US, while for other locales it only registers packages `branding` and `browser`.

So, I'm not perfectly sure how it worked before my change, but it's something about Fennec taking whatever locales it could for "browser" and whatever locales it could for "global".
It just so happens that for "browser" it had multiple locales, and for "global" it had only "en-US".

Well, with the change, we now have one negotiated locale chain, and we negotiate it around "global" package, so we end up with "en-US".

[0] https://hg.mozilla.org/mozilla-central/file/tip/chrome/nsChromeRegistryChrome.cpp#l224
[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/intl/locale/LocaleService.cpp#185

Comment 10

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

Ok, I'm getting to know this bug more:

It only affects users who use "Use System Locale" option and have non-en-US OS.

The problem described in Comment 9 exists, and we should fix it, but it doesn't directly cause this bug.

What causes this bug is that we now consult OSPreferences::GetSystemLocales from LocaleService::GetRequestedLocales when user selected "Use System Locales".

That's a pretty good and clean API chain.

The problem is that for Android,  OSPreferences::GetSystemLocales currently just returns en-US no matter what - http://searchfox.org/mozilla-central/source/intl/locale/android/OSPreferences_android.cpp#27

We need to improve that.

Comment 11

[Richard Newman [:rnewman]]

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

> It only affects users who use "Use System Locale" option and have non-en-US
> OS.

Note that it would also affect users who don't use the system locale, if there's any situation in which Gecko is launched in the background and there's no Android code to init the locale service. I don't think there are any such situations at present (we weed them out when we find them), but see my comment in Bug 1337078.

Comment 12

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

Attachment: Bug 1351873 - Bind OSPreferences::GetSystemLocale to use `intl.locale.os` pref.

This patch fixes a regression that we hit on Fennec when we handed over control
over language negotiation to LocaleService in bug 1347306.

The problem is that we currently do not have good Android bindings for
OSPreferences::GetSystemLocale and in result we could not negotiate
languages between user-selected OS locales and available locales.

The temporary solution implemented here binds android OS preferences to
`intl.locale.os` pref which is set on Locale:OS event in browser.js.
In LocaleService we add a listener on the pref to renegotiate languages
when OS locale changes.

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

Comment 13

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

Ok, since it's a regression that affects user experience, I decided to go for a quick and dirty solution to give us time to come up with a better one.

The PR I submitted uses `intl.locale.os` preference as a proxy for Java's `Locale.getDefault`. I tested it and it works well, and should even react to language changes in the OS.

It's also a sad abuse of the new APIs that we designed, so I hope we'll get rid of it as soon as possible.

The proper solution involves:

1) Writing the correct bindings from C++ to Java via JNI to retrieve OS locales - bug 1337078. That includes triggering "Refresh" when languages change
2) Extending the code in LocaleService::GetInstance to listen for "intl:system-locales-changed" event which will be triggered by OSPreferences. I didn't add it yet because other platforms didn't use it yet.

I started looking into (1) with help from :rnewman, but since I've never worked in Java->C++ FFI, I realized that the amount of learning that it may require from me to write that binding may take a long time and I didn't want to keep the regression in the wild.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8853185 [details]
Bug 1351873 - Bind OSPreferences::GetSystemLocale to use `intl.locale.os` pref.

https://reviewboard.mozilla.org/r/125266/#review127826

This seems like a perfectly good hack to me, bearing in mind my concern about fallback behavior.

::: intl/locale/android/OSPreferences_android.cpp:20
(Diff revision 1)
> -
> +  if (!NS_SUCCEEDED(Preferences::GetCString("intl.locale.os", &locale))) {
> -  if (CanonicalizeLanguageTag(defaultLang)) {
> -    aLocaleList.AppendElement(defaultLang);
> -    return true;
> -  }
> -  return false;
> +    return false;

AIUI, if the pref doesn't exist, `Preferences::GetCString` returns `NS_ERROR_UNEXPECTED`.

The pref does not have a default: it's only present if set, which means that some small slice of the time it will not be set, and this function will return `false`.

Are you sure you want to `return false` here, rather than falling back to `en-US`? I'm not sure what returning false from `ReadSystemLocales` means…

Comment 15

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

Comment on attachment 8853185 [details]
Bug 1351873 - Bind OSPreferences::GetSystemLocale to use `intl.locale.os` pref.

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

Comment 16

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

Comment on attachment 8853185 [details]
Bug 1351873 - Bind OSPreferences::GetSystemLocale to use `intl.locale.os` pref.

https://reviewboard.mozilla.org/r/125266/#review127826

> AIUI, if the pref doesn't exist, `Preferences::GetCString` returns `NS_ERROR_UNEXPECTED`.
> 
> The pref does not have a default: it's only present if set, which means that some small slice of the time it will not be set, and this function will return `false`.
> 
> Are you sure you want to `return false` here, rather than falling back to `en-US`? I'm not sure what returning false from `ReadSystemLocales` means…

Good point. In case of an error, we'd return false from GetSystemLocales and in LocaleService::GetRequestedLocales fallback on reading user `general.useragent.locale` - so nothing tragic, but potentially confusing. Fixed it before landing.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf053eeaeafd
Bind OSPreferences::GetSystemLocale to use `intl.locale.os` pref. r=rnewman

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

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

Issue is not fixed in 55.0a1 20170401 (Sony Xperia Z3 Compact, Android 6.0.1).

Comment 20

[Richard Newman [:rnewman]]

Sebastian, do you see intl.locale.os in about:config?

Comment 21

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

Yes, it's set to 'de-DE'.

Comment 22

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

:aryx, I'm rebuilding from central to verify, but in the meantime, can you provide me:

 - your android locale
 - the locale your main Fennec UI is in
 - the language you see the Tools>Addons page in
 - If you connect WebIDE to your Fennec and in the browser console (main process is fine) tell me the output of:
   - Services.locale.getAvailableLocales();
   - Services.locale.getRequestedLocales();
   - Services.locale.getAppLocalesAsLangTags();
 - the values from about:config of:
   - general.useragent.locale
   - intl.locale.matchOS

Thanks!

Comment 23

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

>  - your android locale
"Deutsch (Deutschland)" / "German (Germany)"

>  - the locale your main Fennec UI is in
system default (uses German)

>  - the language you see the Tools>Addons page in
English

>  - If you connect WebIDE to your Fennec and in the browser console (main
> process is fine) tell me the output of:
>    - Services.locale.getAvailableLocales();
Array with these values: "zh-TW zh-CN sv-SE sk ru pt-PT pt-BR pl nl nb-NO ko ja it fr fi es-ES de da cs en-US"

>    - Services.locale.getRequestedLocales();
Array [ "en-US" ]

>    - Services.locale.getAppLocalesAsLangTags();
Array [ "en-US" ]

>  - the values from about:config of:
>    - general.useragent.locale
chome://global/locale/intl.properties

>    - intl.locale.matchOS
true

Comment 24

[Richard Newman [:rnewman]]

general.useragent.locale is always a complex string on Android, rather than *sometimes* being one as on desktop.

Comment 25

[Flore Allemandou [:flore]]

I have the same problem, this bug is not resolved for me either.

>  - your android locale:
Français / France (French / France)

>  - the locale your main Fennec UI is in :
french

>  - the language you see the Tools>Addons page in :
English

>  - If you connect WebIDE to your Fennec and in the browser console (main
> process is fine) tell me the output of:
>    - Services.locale.getAvailableLocales();
Array [ "zh-TW", "zh-CN", "sv-SE", "sk", "ru", "pt-PT", "pt-BR", "pl", "nl", "nb-NO", 10 de plus… ], when I click on "10 de plus", I also get : "ko", "ja", "it", "fr", "fi", "es-ES", "de", "da", "cs", "en-US" (listed on the side)
>    - Services.locale.getRequestedLocales();
Array [ "en-US" ]
>    - Services.locale.getAppLocalesAsLangTags();
Array [ "en-US" ]
>  - the values from about:config of:
>    - general.useragent.locale
chome://global/locale/intl.properties
>    - intl.locale.matchOS
true
> 
> Thanks!

Comment 26

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

> > Services.prefs.getComplexValue("general.useragent.locale", Ci.nsIPrefLocalizedString).data;
> < "en-US"

Comment 27

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

Found the problem and filed bug 1354055 to fix it.

Comment 29

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

This should be fixed now. Reopen if needed.

Comment 31

[Flore Allemandou [:flore]]

Attachment: screenshot about:nightly on a fr version

looks good to me. about is now correctly localized