language.current should map to general.useragent.locale instead of intl.accept_languages

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Pike, Assigned: vingtetun)

Tracking

unspecified
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Right now, the gecko settings observer syncs language.current to intl.accept_languages. That doesn't do the right thing, neither for requests sent to websites, nor for switching locale settings for gecko-based localized strings like neterror.

It should sync with general.useragent.locale instead.

I have a local build, but that does funky things on macos desktop, and I didn't extensively test the unpatched version, so I don't know who's at fault.

This is required for bootcamp to make the firefox OS browser work against the web.
(Reporter)

Updated

6 years ago
Blocks: 796424
(Reporter)

Updated

6 years ago
Blocks: 768939
(Reporter)

Comment 1

6 years ago
Created attachment 666946 [details] [diff] [review]
sync general.useragent.locale instead of accept-lang

I tried this on my osx desktop build, but that was all around funky. So funky that I'm not sure this patch had anything to do with it.

Kaze, can you have a look? I tested this with switching the language of the phone in the settings app, and you'd need to have both netError.dtd and appstrings.properties localized and registered on the gecko side. For now, I did that manually. And then I hit DNS errors in the browser app. That did the right thing, but I'm not sure if I totally regressed language switching in gaia, or if that wasn't working well to begin with.
Attachment #666946 - Flags: review?(kaze)
(Reporter)

Comment 2

6 years ago
PS: No idea how one would write tests for this, too.
Assuming this blocks localization, marking as a blocker.
blocking-basecamp: ? → +
Comment on attachment 666946 [details] [diff] [review]
sync general.useragent.locale instead of accept-lang

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

This will prevent web applications to access the user selected value from navigator.language. You may want to set up the 2 prefs when the setting is changed instead of only one.
Attachment #666946 - Flags: review?(kaze) → review-
Assignee: nobody → l10n
(Reporter)

Comment 5

6 years ago
You don't want to set accept-lang to a single value, that's breaking the web. Any multilingual site will get its language negotiation corrupted.

The way this should work in theory is that you set general.useragent.locale, and the navigator code picks that up through the generic value 
pref("intl.accept_languages",               "chrome://global/locale/intl.properties");
which then ends up that navigator.language is the first entry there.

This works as long as the first entry in intl.properties is actually the language we intend to select for the UI, but that should be true in general. And it is for the P1 languages we care about.

Updated

6 years ago
Priority: -- → P1
(Reporter)

Comment 6

6 years ago
Unassigning as I'm on PTO.
Assignee: l10n → nobody

Comment 7

6 years ago
Vivien,  thoughts on axel's proposal in comment 5? could you work on a patch that does that while axel is on pto until the 29th, or do we need to wait?
Created attachment 673312 [details] [diff] [review]
Patch

Pike let me know if that what you expect.
Attachment #673312 - Flags: review?(l10n)
(Reporter)

Comment 9

6 years ago
Comment on attachment 673312 [details] [diff] [review]
Patch

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

We really shouldn't have to set accept_languages, the generic value should be fine. If that doesn't work, it'd be cached somewhere (which I didn't find).

PS: This will only start to work once we have the gecko localizations.

Maybe this:

Set the locale, get the selected locale for the 'global' provider, if that's the same language (region may differ, I think, for es vs. es-ES), reset the accept_languages pref, otherwise set it to the given language. That should hopefully be proof to mixed gecko/gaia locale setups, but it's not really how things should work, just how we can make gaia work on top of mis-localized geckos.

The right fix would be a create a new webapi to determine UI language independent of navigator.language, there are too many implications here, I'm afraid.

Or to put the accept-language headers into gaia itself, which, actually, might be a good thing to do for V2, too. It'd be nice if users could edit their languages preferences on the web in the device.
Attachment #673312 - Flags: review?(l10n) → review-
(In reply to Axel Hecht [:Pike] (back Oct 29) from comment #9)
> We really shouldn't have to set accept_languages, the generic value should
> be fine.

We must set `accept_languages' from Gaia, and this has to be reflected in `navigator.language': in Gaia there’s no difference between UI and content.

We could use the `language.current' setting to expose the `general.useragent.locale', in order to properly localize toolkit messages.

We could set a list of locales in a `language.accepted' setting and map it to the `accept_languages' pref — e.g. for `pt-BR' we’d set `accept_languages' to `pt-BR, pt, en-US, en', as we do already for Firefox [1].

And selecting a language in the Gaia Settings would set both values. Thoughts?

[1] http://hg.mozilla.org/l10n-central/pt-BR/file/533ebea92a48/toolkit/chrome/global/intl.properties#l33
Created attachment 673817 [details] [diff] [review]
Patch v2

This is fairly easy to set both prefs if needed. This will let Gaia set the UI locale as well as a list for accept_languages synced with the values in  intl.properties files.

This needs some Gaia changes as well that I would add to this bug if you agree with this patch.
Attachment #666946 - Attachment is obsolete: true
Attachment #673312 - Attachment is obsolete: true
Attachment #673817 - Flags: review?(l10n)
(Reporter)

Comment 12

6 years ago
(In reply to Fabien Cazenave [:kaze] from comment #10)
> (In reply to Axel Hecht [:Pike] (back Oct 29) from comment #9)
> > We really shouldn't have to set accept_languages, the generic value should
> > be fine.
> 
> We must set `accept_languages' from Gaia, and this has to be reflected in
> `navigator.language': in Gaia there’s no difference between UI and content.

You will pick up localized and thus correct values once we have a localized gecko underneath gaia.

> We could use the `language.current' setting to expose the
> `general.useragent.locale', in order to properly localize toolkit messages.
> 
> We could set a list of locales in a `language.accepted' setting and map it
> to the `accept_languages' pref — e.g. for `pt-BR' we’d set
> `accept_languages' to `pt-BR, pt, en-US, en', as we do already for Firefox
> [1].
> 
> And selecting a language in the Gaia Settings would set both values.
> Thoughts?
> 
> [1]
> http://hg.mozilla.org/l10n-central/pt-BR/file/533ebea92a48/toolkit/chrome/
> global/intl.properties#l33

We don't have any good UX for setting accept-lang headers, even on desktop. I don't think we should burden ourselves with trying to create one for gaia v1.
(Reporter)

Comment 13

6 years ago
I've reconsidered the code path from comment 9, to work around the lack of gecko localization support for a gaia-locale:

set general.useragent.locale
check if accept-lang has a user-set value, reset if so
get the complex value for an nsIPrefLocalizedString for accept-lang
if the first chunk of that value doesn't match the set locale
  prefix the found value with the set value
  set the user string pref for accept-lang

This code should only be hit for development, and not be used on production devices, too.
(Reporter)

Comment 14

6 years ago
Comment on attachment 673817 [details] [diff] [review]
Patch v2

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

r- based on my previous comments.
Attachment #673817 - Flags: review?(l10n) → review-
(In reply to Axel Hecht [:Pike] (back Oct 29) from comment #12)
> (In reply to Fabien Cazenave [:kaze] from comment #10)
> > (In reply to Axel Hecht [:Pike] (back Oct 29) from comment #9)
> > > We really shouldn't have to set accept_languages, the generic value should
> > > be fine.
> > 
> > We must set `accept_languages' from Gaia, and this has to be reflected in
> > `navigator.language': in Gaia there’s no difference between UI and content.
> 
> You will pick up localized and thus correct values once we have a localized
> gecko underneath gaia.
> 

The result would be the same if we reuse the values defined in Gecko in the `settings.[locale].properties' files — and that’d be much easier for us.

> > We could use the `language.current' setting to expose the
> > `general.useragent.locale', in order to properly localize toolkit messages.
> > 
> > We could set a list of locales in a `language.accepted' setting and map it
> > to the `accept_languages' pref — e.g. for `pt-BR' we’d set
> > `accept_languages' to `pt-BR, pt, en-US, en', as we do already for Firefox
> > [1].
> > 
> > And selecting a language in the Gaia Settings would set both values.
> > Thoughts?
> > 
> > [1]
> > http://hg.mozilla.org/l10n-central/pt-BR/file/533ebea92a48/toolkit/chrome/
> > global/intl.properties#l33
> 
> We don't have any good UX for setting accept-lang headers, even on desktop.
> I don't think we should burden ourselves with trying to create one for gaia
> v1.

I’m not suggesting to design an accept-lang setting panel: I’m saying that if these `accept_languages' values are defined in the `settings.[locale].properties' files, selecting a locale will automatically apply the same `accept_languages' pref as Gecko would do, without bothering the user. And if advanced users want to tweak this `accept_languages' pref, it’ll still be possible to design a Gaia app for that.
Created attachment 674180 [details] [diff] [review]
Patch v3

Let's try to implement your idea.
Attachment #673817 - Attachment is obsolete: true
Attachment #674180 - Flags: review?(l10n)
(Reporter)

Comment 17

6 years ago
Comment on attachment 674180 [details] [diff] [review]
Patch v3

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

r=me with a slightly more rigorous test, Asturian and Assamese are counter examples I can theoretically see.

::: b2g/chrome/content/settings.js
@@ +79,5 @@
> +    intl = Services.prefs.getComplexValue(prefName,
> +                                          Ci.nsIPrefLocalizedString).data;
> +  } catch(e) {}
> +
> +  if (intl.indexOf(value) != 0) {

I think we need to make this check more rigorous, right now, "ast" and "as" make this pass. Maybe a regexp that only allows non-ascii characters?
Attachment #674180 - Flags: review?(l10n) → review+
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Oups I didn't meant to close it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [checking-needed-aurora]
https://hg.mozilla.org/mozilla-central/rev/f6293f153a6b
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [checking-needed-aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/eae4cdc7f382
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.