Closed Bug 1142758 Opened 5 years ago Closed 5 years ago

Built-in app names not localized on homescreen: getLocalizedValue should use the passed locale to pick the right locale value from the manifest

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(blocking-b2g:2.5?, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
mozilla39
blocking-b2g 2.5?
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: zbraniecki, Assigned: fabrice)

References

Details

Attachments

(2 files, 1 obsolete file)

Currently, app.getLocalizedValue('name', 'fr').then(n => {console.log(n)}); will return en-US name from the manifest.

Instead it should return the value from manifest.locales['fr'].name.
Are you 100% sure the device is running with a french locale? If it is this is the one we select to get manifest values.

What should not work with the current code is doing getLocalizedValue('foo', 'de') on a french device even if manifest.locales['de'].foo exists (we would fallback to manifest.locales['fr'].foo or manifest.foo, or fail).
STR:

 - install master gaia on your device with at least one other locale than en-us (build multilocale gaia)
 - switch to the non-default locale (for example 'fr')
 - plug webide to verticalhomescreen
 - test that navigator.language is 'fr' and so is document.documentElement.lang and navigator.mozL10n.language.code
 - try app.getLocalizedValue('name', 'fr')

Current result:
 - the value returned is an en-US name

Expected Result:
 - value should be in french

If there is some other setting that should be switched to 'fr' in Gecko to get the righr value, I don't know. But navigator.language is 'fr'.
Duplicate of this bug: 1142874
Duplicate of this bug: 1143012
Duplicate of this bug: 1143229
[Blocking Requested - why for this release]:
This results in all homescreen apps appearing unlocalized. Thus nominating
blocking-b2g: --- → 3.0?
Sorry Gandalf, editing the title of this bug a bit in order to make it clearer avoid more dupes ;)
Summary: getLocalizedValue should use the passed locale to pick the right locale value from the manifest → Homescreen Buil-in apps not localized: getLocalizedValue should use the passed locale to pick the right locale value from the manifest
Summary: Homescreen Buil-in apps not localized: getLocalizedValue should use the passed locale to pick the right locale value from the manifest → Built-in app names not localized on homescreen: getLocalizedValue should use the passed locale to pick the right locale value from the manifest
Flags: needinfo?(taz)
We know the root cause here, no need to look for a regression range.
SO, the locale we get from https://mxr.mozilla.org/mozilla-central/source/dom/apps/AppsUtils.jsm#804 is still en-US even after switching to French. Let's fix that.
The issue is that gecko only has the en-US locale and so doesn't fallback to the gaia selected locale.

For instance, browse to https://people.mozilla.org/~fdesre/locale that uses some gecko-provided localization for the <file type=input> control. It's always en-US and this is what we use in the manifest locale code.
(In reply to Fabrice Desré [:fabrice] from comment #9)
> SO, the locale we get from
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/AppsUtils.jsm#804 is
> still en-US even after switching to French. Let's fix that.

I'm confused here. While I do believe that it would be nice to fix this so that locale we get there matches the locale we set and use for navigator.language, I also see the getLocalizedValue as an API that takes locale that should be returned. Which means that whatever locale I pass, should be used there, not the one from Gecko.

I believe that it would be a poor design decision to have the API take a locale and then use it in some scenarios (langpack) and not in others (manifest).
Attached patch l10n-manifest.patch (obsolete) — Splinter Review
Since we only ship the en-US gecko locale with B2G, this patch uses instead the general.useragent.locale pref that we set when changing locale from gaia.

Tested with a multilocale build, works fine.
Assignee: nobody → fabrice
Attachment #8578131 - Flags: review?(ferjmoreno)
Kevin, Jonas, what's your take on Fabrice's approach vs. comment 11?

I verified that the patch fixes the bug, but am worried about the API design here. Another option, although limiting, could be to remove the locale param and take it from the setting noth for langpack and manifest.
Flags: needinfo?(kgrandon)
Flags: needinfo?(jonas)
(In reply to Zibi Braniecki [:gandalf] from comment #13)
> I verified that the patch fixes the bug, but am worried about the API design
> here. Another option, although limiting, could be to remove the locale param
> and take it from the setting noth for langpack and manifest.

If we don't actually need a locale param then it makes sense to me to remove it. Maybe I am missing something though?
Flags: needinfo?(kgrandon)
Using general.useragent.locale or navigator.language implies two things, both of which are not good IMO:

 - it implies that the value is a language code whose format is exactly the same as the one specified in the manifest, and
 - it implies that the language preference is a single value, whereas a lot of work has gone into making the shift to thinking in terms of an array of preferred languages (navigator.languages).

By explicitly passing the language to the API, we can properly negotiate languages in l10n.js using navigator.languages.  The negotiated array of languages is guaranteed to be using the same values as the languages available in the manifest.  There's no risk of navigator.language being 'fr' and the manifest specifying 'fr-FR.'
Attachment #8578131 - Flags: review?(ferjmoreno)
Thanks for the details Staś! However the gecko side can't trust the content to pass a language that is present in the manifest. I'm gonna change my patch to explicitly select the locale that is sent by the getLocalizedValue() and fallback to the default locale.
(In reply to Fabrice Desré [:fabrice] from comment #16)
> However the gecko side can't trust the content
> to pass a language that is present in the manifest.

At least when a build system similar to the one we have in Gaia is used, I think we actually can.  I'm assuming here that any reasonable build system will use the same language codes for adding localized fields in the manifest and for value of <meta name="availableLanguages">.  Then, the localization library will conduct language negotiation using availableLanguages and navigator.languages and the result of that will be an array of language codes taken from availableLanguages which matched something in navigator.languages.  We can now use those codes in getLocalizedValue() to match against the locales in the manifest.
This addresses previous comments, by explicitly selecting the locale to use in the manifest when needed.

Gandalf, can you test on your side?
Attachment #8578131 - Attachment is obsolete: true
Attachment #8579515 - Flags: feedback?(gandalf)
Comment on attachment 8579515 [details] [diff] [review]
l10n-manifest.patch v2

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

Tested with:
 - pretranslated locale
 - built locale
 - langpack

Everything seems to work!
Attachment #8579515 - Flags: feedback?(gandalf) → feedback+
Attachment #8579515 - Flags: review?(ferjmoreno)
Attachment #8579515 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8579515 [details] [diff] [review]
l10n-manifest.patch v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): App namr API
User impact if declined: langpacks without homescreen
Testing completed: device
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none
Flags: needinfo?(taz)
Flags: needinfo?(jonas)
Attachment #8579515 - Flags: approval-mozilla-b2g37?
Stas & Gandalf, do you know why https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/test/marionette/grid_layout_test.js could be broken by this change?
Flags: needinfo?(stas)
Flags: needinfo?(gandalf)
Trying to figure out. I can confirm that the qps-plocm works on the device, so it's a test bug.
So, when I try to launch the test locally:

TEST_FILES=./apps/verticalhome/test/marionette/grid_layout_test.js VERBOSE=1 make test-integration

I'm getting:

[marionette error] app://system.gaiamobile.org/js/bootstrap.js:228 TypeError: window.performance.mark is not a function
[marionette error] app://verticalhome.gaiamobile.org/js/sources/application.js:15 TypeError: window.performance.mark is not a function
[marionette error] app://verticalhome.gaiamobile.org/js/app.js:13 TypeError: window.performance.mark is not a function
[marionette error] app://verticalhome.gaiamobile.org/js/statusbar.js:77 ReferenceError: app is not defined
[marionette error] chrome://marionette/content/marionette-listener.js:58 TypeError: sandbox is null
[marionette error] app://system.gaiamobile.org/js/init_logo_handler.js:224 TypeError: window.performance.mark is not a function

, when I manually add a noop for performance.mark in those files, I'm getting:

[marionette error] chrome://global/content/BrowserElementChildPreload.js:75 TypeError: content.document.body is null
[marionette error] app://verticalhome.gaiamobile.org/js/configurator.js:44 IccHelper isn't enabled. SingleVariant configuration can't be loaded
  loadSingleVariantConf @ app://verticalhome.gaiamobile.org/js/configurator.js:44
  onLoadInitJSON @ app://verticalhome.gaiamobile.org/js/configurator.js:132
[marionette error] app://verticalhome.gaiamobile.org/shared/elements/gaia_grid/js/items/mozapp.js:201 TypeError: this.app.getLocalizedValue is not a function
[marionette error] app://verticalhome.gaiamobile.org/shared/elements/gaia_grid/js/items/mozapp.js:201 TypeError: this.app.getLocalizedValue is not a function

-----------

I tried changing Makefile to download B2G from today:

B2G_SDK_VERSION := 39.0a1
B2G_SDK_DATE := 2015/03/2015-03-19-01-02-01

but it still displays the same errors.

So, I can't get this test to even run on master.


I tried to verify that home.localizedAppName('communications', 'dialer', 'qps-plocm') returns ""Ԁɥou

which is the same string as displayed in the screenshot from the treeherder error log and on the phone.

I don't understand why it doesn't work and I'm not sure how to debug it :(
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #25)
> I tried changing Makefile to download B2G from today:

Sure, but I guess that wouldn't have the gecko patch in it? Did you try building b2g with the gecko patch to see what the result was?
No, because I dont know how to build B2G for macos, but performance.mark and app.getLocalizedValue should be in m-c without this patch so there's something else going on...
Here's my theory:

https://github.com/mozilla-b2g/gaia/pull/28510 landed and it added asyncName method to gaia_grid, which pseudolocalizes the name of the app/entry_point if the current language is one of mozL10n.qps.

Because we had this bug, getLocalizeValue returned en-US, which was being pseudolocalized and all worked well.

However, with the fix, getLocalizeValue actually already returns the pseudolocalized name, because the pseudolocales are built and packaged on buildtime and that includes the manifest fields.

We end up running the pseudolocalized name through the pseudolocalization logic again, which breaks the test.

I think that changing 

  userLang in navigator.mozL10n.qps

to

  userLang in navigator.mozL10n.ctx.qps

in

  https://github.com/mozilla-b2g/gaia/blob/640001a772e4c9bd35a8b0017f842b86fbab95a8/shared/elements/gaia_grid/js/items/mozapp.js#L25

should fix this.  mozL10n.ctx.qps is the list of runtime pseudolocales and it excludes the packaged ones.
Flags: needinfo?(stas)
(In reply to Staś Małolepszy :stas from comment #28)

> I think that changing 
> 
>   userLang in navigator.mozL10n.qps
> 
> to
> 
>   userLang in navigator.mozL10n.ctx.qps

Oops, I forgot the ctx.qps is an array, not an object :/  The above should use indexOf.  I'm thinking maybe we should make this consistent with mozL10n.qps one way or another.
(In reply to Staś Małolepszy :stas from comment #29)
> (In reply to Staś Małolepszy :stas from comment #28)
> 
> > I think that changing 
> > 
> >   userLang in navigator.mozL10n.qps
> > 
> > to
> > 
> >   userLang in navigator.mozL10n.ctx.qps
> 
> Oops, I forgot the ctx.qps is an array, not an object :/  The above should
> use indexOf.  I'm thinking maybe we should make this consistent with
> mozL10n.qps one way or another.

Are these changes you can do on the gaia side before I re-land the gecko part or do they need to be coordinated?
I think we'll need to coordinate because the test currently relies on the broken behavior of getLocalizedValue.  Should we disable the test and then re-enabled it fixed after you land?
(In reply to Staś Małolepszy :stas from comment #28)
> Here's my theory:

And Stas is right, nothing new here... :)

I tested double translation of qps-plocm and the only difference between the first translation of "Phone" and the double translation is the second letter being "u" instead of "n".

Then I looked again at the screenshot from the failed test and confirmed that there's "u" as a second letter in the "Phone" icon, which basically confirms that we are double-translating the names for pseudolocales here.

I also agree with Stas's approach - let's disable the test, land this patch, and I'll write a patch to update and re-enable the test.

Will that work for you Fabrice?
Flags: needinfo?(fabrice)
That works for me!
Flags: needinfo?(fabrice)
Comment on attachment 8579515 [details] [diff] [review]
l10n-manifest.patch v2

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

Tested with:
 - pretranslated locale
 - built locale
 - langpack

Everything seems to work!
Attachment #8579515 - Flags: approval-mozilla-b2g37?
Comment on attachment 8580878 [details] [review]
[gaia] zbraniecki:1142758-disable-rtl-test > mozilla-b2g:master

Kevin, we want to disable the test temporarily to land the Gecko patch.

I'll submit the patch to reenable the test here today.
Attachment #8580878 - Flags: review?(kgrandon)
Comment on attachment 8580878 [details] [review]
[gaia] zbraniecki:1142758-disable-rtl-test > mozilla-b2g:master

Sounds fine to me, thanks.
Attachment #8580878 - Flags: review?(kgrandon) → review+
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
Yeah, autolander isn't meant to work with bugs with gecko & gaia patches yet =| 

Going to add checkin-needed once more to see if it sticks. (Autolander might complain)
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/64bf333eab25
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8579515 - Flags: approval-mozilla-b2g37?
This patch is good to be uplifted once we receive the approval. We should uplift it first before we uplift gaia patches to prevent orange tests.
Attachment #8579515 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Need the gecko patch to be landed on b2g37 pls!
Keywords: checkin-needed
Please avoid using checkin-needed for uplifts. We have different bug queries for them.
Sorry for the keyword! The bug you linked seems unrelated?

We want to get gecko patches landed - that should not break any tests. Then I'll prepare a combo Gaia patch.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #48)
> Sorry for the keyword! The bug you linked seems unrelated?

It's the bug that originally added grid_layout_test.js. When it was uplifted to v2.2, the test wasn't included.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #47)
> Looks like the uplift of bug 1115104 to v2.2 didn't include the test?

True, I don't know why...

Thanks for the uplift!
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.