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

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: flore, Assigned: zbraniecki)

Tracking

({nightly-community, regression})

55 Branch
Firefox 55
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec55+, firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55+ fixed)

Details

Attachments

(3 attachments)

Reporter

Description

2 years ago
No description provided.
Reporter

Comment 1

2 years ago
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.
Reporter

Updated

2 years ago
Whiteboard: [nightly-community],[mozfr-community]
[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?
tracking-fennec: --- → ?
Flags: needinfo?(gandalf)
Keywords: regression
Hardware: ARM → All
Thank you for reporting it! I'm investigating.
Reporter

Updated

2 years ago
Whiteboard: [nightly-community],[mozfr-community]
Tracking 55+ for this regression.
Assignee: nobody → gandalf
tracking-fennec: ? → 55+

Updated

2 years ago
Duplicate of this bug: 1352053

Updated

2 years ago
Summary: Long click menu is not localized anymore → Gecko-side localisation (e.g. context menu) broken in Fennec

Comment 6

2 years ago
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.
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.
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.
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
Flags: needinfo?(gandalf)
Status: NEW → ASSIGNED

Updated

2 years ago
Blocks: 1347306
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.
Depends on: 1337078
(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 hidden (mozreview-request)
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

2 years ago
mozreview-review
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…
Attachment #8853185 - Flags: review?(rnewman) → review+
Comment hidden (mozreview-request)
Assignee

Comment 16

2 years ago
mozreview-review-reply
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

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf053eeaeafd
Bind OSPreferences::GetSystemLocale to use `intl.locale.os` pref. r=rnewman

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf053eeaeafd
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Issue is not fixed in 55.0a1 20170401 (Sony Xperia Z3 Compact, Android 6.0.1).
Status: RESOLVED → REOPENED
Flags: needinfo?(gandalf)
Resolution: FIXED → ---
Sebastian, do you see intl.locale.os in about:config?
Flags: needinfo?(aryx.bugmail)
Yes, it's set to 'de-DE'.
Flags: needinfo?(aryx.bugmail)
: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!
Flags: needinfo?(aryx.bugmail)
>  - 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
Flags: needinfo?(aryx.bugmail)
general.useragent.locale is always a complex string on Android, rather than *sometimes* being one as on desktop.
Reporter

Comment 25

2 years ago
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!
> > Services.prefs.getComplexValue("general.useragent.locale", Ci.nsIPrefLocalizedString).data;
> < "en-US"
Priority: -- → P1
Found the problem and filed bug 1354055 to fix it.
Depends on: 1354055
Flags: needinfo?(gandalf)
Duplicate of this bug: 1355804
This should be fixed now. Reopen if needed.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Duplicate of this bug: 1356590
Reporter

Comment 31

2 years ago
looks good to me. about is now correctly localized

Updated

2 years ago
See Also: → 1360596
You need to log in before you can comment on or make changes to this bug.