824418 - Install a hosted app that has a locale override that matches the device's locale - install prompt, install confirmation, homescreen app name, app perms UI, and app updates download prompt all need to use the overridden properties consistently

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect, P3)

Description

[Jason Smith [:jsmith]]

Build: B2G18 12/23/2012
Device: Unagi

Steps:

1. Go to http://jds2501.github.com/webapi-permissions-tests/
2. Install the hosted app from "Hosted App Test Case 6" or "Hosted App Test Case 7" with the phone's locale set to English

Expected:

The app name on the homescreen should use the localized app name.

Actual:

The default name is seen on the homescreen. This is incorrect - the localized app name is supposed to be used from a locale if the locale specified matches the phone's locale.

Additional Notes:

This is definitely a v1 blocker. Why? Because the large majority (if not almost all) of the hosted apps submitted to marketplace default to using English for their default values and use a locale overrides for each target market this phone aims to hit. In short, this means our users in our target markets will be stuck with english-based install prompts for those apps, not the target locale that phone belongs to (e.g. pt-BR locale in locale override won't be used, so Porteguese won't be used in the install prompt, English would be).

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

Triage: BB+, C3, P3 - agree with additional notes in description.
Please renom if the risk to resolve this issue is way too high given the time we have

Comment 2

[David Scravaglieri [:scravag]]

I wonder if I prefer to abandon the project or ship it knowing this issue ? Renominating...

Comment 3

[Jason Smith [:jsmith]]

(In reply to David Scravaglieri [:scravag] from comment #2)
> I wonder if I prefer to abandon the project or ship it knowing this issue ?
> Renominating...

If we don't fix this, then pretty much every app installed from marketplace will always be in English. And there really isn't a work-around on this issue.

Firefox Marketplace 1.0's success is as critical as the on-device code. If we fail to provide an effective experience, then our apps ecosystem for the phone will fail. So yeah, I still stand to believe this is a showstopper given our target markets primarily being in a non-English locale.

Comment 4

[Lawrence Mandel [:lmandel] (use needinfo)]

Jason, can you please clarify a couple of points from your description:
1. You state in your STR "with the phone's locale set to English". Do you then change the locale to another language after installation? Does the scenario work as expected if the locale is set to another language, like Portuguese?
2. You state in actual that the default name is seen instead of the localized app name. I think you mean a default string from the app. Do you mean the default English string en-US? Can you provide a concrete example of the issue?

Comment 5

[Jason Smith [:jsmith]]

(In reply to Lawrence Mandel [:lmandel] from comment #4)
> Jason, can you please clarify a couple of points from your description:
> 1. You state in your STR "with the phone's locale set to English". Do you
> then change the locale to another language after installation? Does the
> scenario work as expected if the locale is set to another language, like
> Portuguese?

Not after install. It was English from the very beginning and after (no language change in the process).

I modified my test case to use pt-BR vs. English and retested:

{
  "version": "1.0",
  "name": "Test WebAPI Permissions",
  "description": "A test app for testing out web api permissions",
  "launch_path": "/webapi-permissions-tests/webapi_permissions_test.html",
  "icons": {
    "126": "/webapi-permissions-tests/qalogo.png"
  },
  "type": "web",
  "permissions": {
    "geolocation": {
      "description": "Come see my geolocation"
    },
    "desktop-notification": {
      "description": "Come see my notifications"
    },
    "fmradio": {
      "description": "Come see my fmradio"
    }
  },
  "developer": {
    "name": "Mozilla QA",
    "url": "http://quality.mozilla.org"
  },
  "locales": {
    "en": {
      "name": "Localized app name for English",
      "developer": {
        "name": "Localized developer name for English",
        "url": "Localized developer url for English"
      }
    },
    "pt-BR": {
      "name": "Localized app name for Brazil pt-BR",
      "developer": {
        "name": "Localized developer name for Brazil pt-BR",
        "url": "Localized developer url for Brazil pt-BR"
      }
    }
  },
  "default_locale": "es"
}

When I tested this with the phone's locale to pt-BR, the install prompt did end up having the correct app name in the prompt ("Localized app name for Brazil pt-BR"), but the developer name and url were still wrong (defaults were used - Mozilla QA and https://quality.mozilla.org). After I complete installation of the web app, the install web app notification ends up showing the wrong app name (Test WebAPI Permissions instead of Localized app name for Brazil pt-BR). The app name on the homescreen ended up being correct - Localized app name for Brazil pt-BR. The settings app perms UI has the wrong app name (Test WebAPI Permissions instead of Localized app name for Brazil pt-BR) and the wrong developer info (it had Mozilla QA, https://quality.mozilla.org).

So...looks like we are inconsistently passing in some areas and failing in others. I'm going to dupe the other bugs on this one and just generalize this bug to "fixing" the kinks for this.

Odds are with more test cases I'll hit more unpredictable behavior, but let me know if more analysis is needed.

> 2. You state in actual that the default name is seen instead of the
> localized app name. I think you mean a default string from the app. Do you
> mean the default English string en-US? Can you provide a concrete example of
> the issue?

I'll describe this using an example from the calculator app in gaia:

{
  "name": "Calculator",
  "description": "Calculator",
  "type": "certified",
  "launch_path": "/index.html",
  "developer": {
    "name": "The Gaia Team",
    "url": "https://github.com/mozilla-b2g/gaia"
  },
  "locales": {
    "ar": {
      "name": "الحاسبة",
      "description": "Gaia الحاسبة"
    },
    "en-US": {
      "name": "Calculator",
      "description": "Gaia Calculator"
    },
    "fr": {
      "name": "Calculatrice",
      "description": "Calculatrice Gaia"
    },
    "zh-TW": {
      "name": "計算機",
      "description": "計算機"
    }
  },
  "default_locale": "en-US",
  "icons": {
    "120": "/style/icons/Calculator.png",
    "60": "/style/icons/60/Calculator.png"
  },
  "orientation": "portrait-primary"
}

If we look above, the default_locale is en-US, which means that manifest properties not in the locales property are in English (e.g. name is in English, description is English, developer name is in English). As you see above, there are alternative locale overrides that exist for the "ar" locale (Arabic), "fr" locale (French), and "zh-TW" locale (Chinese - Taiwan (Traditional Chinese)). So just as an example, let's say I try to install this packaged app off of marketplace with the type of web (since you can't install certified apps). Here's what happens based on the phone's locale with this app with one example with the override and one example without.

Override Found Example:

If the phone's locale is Arabic, then the install prompt would show me the Arabic app name "الحاسبة" wants to be installed with the default developer information of The Gaia team along with the developer url shown above (since the locale did not provide an override for the developer information). When I confirm installation, the install notification upon app installation completion would say "الحاسبة was installed." The app name on the homescreen would show الحاسبة. If I went to the app perms UI, then I would see the app name الحاسبة listed.

Override not found example:

If the phone's locale is Portuguese - Brazil (pt-BR), then the install prompt would show me the default name in English (Calculator) and the default developer info (The Gaia team along with the URL above). When I confirm installation, the install notification upon app installation completion would say "Calculator" was installed. The app name on the homescreen would show Calculator.If I went to the app perms UI, then I would see the app name Calculator listed.

To find out more information on the usage with existing hosted apps I'll have to do some data mining of the app manifests on marketplace (which is possible to do, but will take some time). Let me know if that's needed to make a call here.

What I've seen so far for the common reasons for a locale override are:

* A locale-specific app name to show the app name translated to that particular locale
* A locale-specific developer name to show the developer name translated to that particular locale
* A locale-specific developer url, as the developer has specific subdomains/urls on a per locale basis (e.g. es.thisexample.com for es locale)

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

Triage: BB+, back to original decision

Comment 9

[:Margaret Leibovic]

Looking through the code (searching for "manifest.locales"), it appears we only actually use manifest.locales for localizing the app name.

Jason, have you seen problems where the app name is not translated correctly?

Other than the developer name and url, are there other things we should support localizing? Is there a document somewhere that actually talks about what properties we support localizing?

I don't see places in Gaia where we're using "description" from the manifest, but we're using it in Gecko in WebappsInstaller.jsm:
http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/WebappsInstaller.jsm#109

I wonder if we'd also need to start digging into Gecko.

Comment 10

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #9)
> Looking through the code (searching for "manifest.locales"), it appears we
> only actually use manifest.locales for localizing the app name.
> 
> Jason, have you seen problems where the app name is not translated correctly?

A quick search for "manifest.name" shows me that there are definitely places where we're not checking the locales property first.

> Other than the developer name and url, are there other things we should
> support localizing? Is there a document somewhere that actually talks about
> what properties we support localizing?

Oh, hm, on MDN [1] it says we support overriding everything in the manifest. It seems to me like we need a solution that replaces the manifest data itself with the localized data, rather than doing ad-hoc checks of the locales property all over the place. Is there a place where it would make sense to do this?

[1] https://developer.mozilla.org/en-US/docs/Apps/Manifest#locales

Comment 11

[:Margaret Leibovic]

I wonder what Fabrice thinks about this.

Comment 12

[:Margaret Leibovic]

In Gecko, ManifestHelper takes care of this l10n stuff:
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#333

So maybe we need to make a similar helper for all the places Gaia tries to use manifest data.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Margaret Leibovic [:margaret] from comment #9)
> Looking through the code (searching for "manifest.locales"), it appears we
> only actually use manifest.locales for localizing the app name.
> 
> Jason, have you seen problems where the app name is not translated correctly?

Yup. In the case of pt-BR, the two failures I saw was:

* The install success notification
* The app name in the settings app perms UI

> 
> Other than the developer name and url, are there other things we should
> support localizing? Is there a document somewhere that actually talks about
> what properties we support localizing?

[1] as you've called out in comment 10 is the golden reference I've used. In terms of existing app manifests, only app name, description, dev name, and dev url are the properties I've typically seen with existing hosted apps that get overridden by a locale override. As you've noted above however, the original design of locales allowed overrides on more than just those properties.

Comment 14

[:Margaret Leibovic]

Attachment: WIP

This seems to be working well, but I want to make sure there aren't things I'm missing.

Comment 15

[Etienne Segonzac (:etienne)]

Vivien just pointed out that the app name is also present on the update download prompt.

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/updatable.js#L19

Comment 16

[:Margaret Leibovic]

(In reply to Etienne Segonzac (:etienne) from comment #15)
> Vivien just pointed out that the app name is also present on the update
> download prompt.
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/updatable.
> js#L19

Thanks for catching that. I also noticed we use manifest.name in everything.me code. Is that the type of thing I should fix here, or do people work on everything.me in a separate repo?

Comment 17

[:Margaret Leibovic]

Attachment: patch

I added the case Etienne mentioned, but I didn't touch everything.me. I can change that here, or we can file a follow-up bug for that.

I also put this in a github branch here:
https://github.com/leibovic/gaia/tree/manifest-locales

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 696316 [details] [diff] [review]
patch

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

::: shared/js/manifest_helper.js
@@ +16,5 @@
> +  var locale = document.documentElement.lang;
> +  if (this._manifest.locales && this._manifest.locales[locale]) {
> +    this._localeRoot = this._manifest.locales[locale];
> +  }
> +  else if (this._manifest.locales) {

nit: } else if (...) {

@@ +21,5 @@
> +    // try with the language part of the locale ("en" for en-GB) only
> +    var lang = locale.split('-')[0];
> +    if (lang != locale && this._manifest.locales[lang])
> +      this._localeRoot = this._manifest.locales[lang];
> +  }

var locales = this._manifest.locales;

@@ +32,5 @@
> +    return this._manifest[prop];
> +  },
> +
> +  get name() {
> +    return this._localeProp("name");

nit: " -> '

Same for the other getters.

@@ +65,5 @@
> +  },
> +
> +  get package_path() {
> +    return this._localeProp("package_path");
> +  }

Also instead of having all those getters why can't we simply generate the properties for the object when it is instantiated?

var ManifestHelper = function(manifest) {
  var localeRoot = manifest;
 
  var locales = manifest.locales;
  if (locales) {
    var lang = document.documentElement.lang;
    
    // If there is a manifest entry for the curret locale, use it, otherwise
    // fallback on the default manifest.
    localeRoot = locales[lang] || locales[lang.split('-')[0]] || manifest;
  }

  [properties_list].forEach(function(name) {
    this[name] = localeRoot[name] || manifest[name];
  }.bind(this));
};

Comment 19

[:Margaret Leibovic]

Attachment: patch v2

I changed manifest_helper.js to address your comments, and I agree that's much simpler. I was just using a slimmed-down version of gecko's ManifestHelper before, and I didn't think about changing it like that.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 696350 [details] [diff] [review]
patch v2

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

::: shared/js/manifest_helper.js
@@ +7,5 @@
> + * Helper object to access manifest information with locale support.
> + */
> +
> +var ManifestHelper = function(manifest) {
> +  var localeRoot = manifest; 

nit: extra whitespace at the end

@@ +12,5 @@
> +  var locales = manifest.locales;
> +
> +  if (locales) {
> +    var lang = document.documentElement.lang;
> +    

nit: extra whitespaces

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

A followup for e.me sounds good. The list of apps they have is already out of date I think so it needs to be clean up anyway.

Comment 22

[:Margaret Leibovic]

https://github.com/mozilla-b2g/gaia/commit/6e7b53a0709e3c09285ae197778ec8ba94b6de82

Comment 23

[:Margaret Leibovic]

Don't need info from Fabrice anymore, but maybe we should file a follow-up bug to get all this localized data directly from the gecko APIs, instead of needing to worry about it in Gaia.

Comment 24

[Jason Smith [:jsmith]]

Verified with 2 different hosted apps doing locale overrides - looks good.