Bug 739249: Implement HTML Localization. #387

Merged
merged 1 commit into from Apr 23, 2012

Projects

None yet

2 participants

@ochameau
Mozilla member

HTML localization feature described here:
https://github.com/mozilla/addon-sdk/wiki/HTML-Page-Localization
And discussed there:
https://groups.google.com/d/msg/mozilla-labs-jetpack/-/Jc-YQjf8tiQJ
Related bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=739249

This pull request is not completely ready, I'd like to get some feedback on the new method based on DOM instead of templates.
This patch needs some more unit tests, but is considered closed to what can land.

@Gozala Gozala was assigned Apr 7, 2012
@Gozala Gozala and 1 other commented on an outdated diff Apr 12, 2012
packages/addon-kit/data/test-localization.html
@@ -0,0 +1,23 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+ - License, v. 2.0. If a copy of the MPL was not distributed with this
+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+
+<html>
+ <head>
+ <title>HTML Localization</title>
+ </head>
+ <body>
+ <div data-l10n-id="Not translated">Kept as-is</div>
+ <ul data-l10n-id="Translated">
+ <li>Inner html content is replaced,</li>
+ <li data-l10n-id="text-content">
+ Elements with data-l10n-id, that are a (in)direct child of an element
+ with data-l10n-id, are lost.
@Gozala
Gozala Apr 12, 2012

Do you mean are not localized ? Lost sounds quite brutal.

@Gozala
Gozala Apr 12, 2012

Ok it looks like the owner element will be localized replacing all the content, but I think it's still better to rephrase this. Maybe @wbamberg can help.

@ochameau
ochameau Apr 12, 2012

What do you think about: Elements with data-l10n-id attribute whose parent element is translated will be replaced by the content of the translation.

@Gozala
Gozala Apr 12, 2012

I think it's better.

@Gozala Gozala and 1 other commented on an outdated diff Apr 12, 2012
packages/addon-kit/lib/l10n.js
const { getRulesForLocale } = require("api-utils/l10n/plural-rules");
-// Get URI for the addon root folder:
-const { rootURI } = require("@packaging");
-
-let globalHash = {};
-let pluralMappingFunction = getRulesForLocale("en");
+// Retrieve the plural mapping function
+let gPluralMappingFunction = null;
+if (core.locale) {
+ let shortLocaleCode = core.locale.split("-")[0].toLowerCase();
+ gPluralMappingFunction = getRulesForLocale(shortLocaleCode);
@Gozala
Gozala Apr 12, 2012

It feels to me that api-utils/l10n/core should directly export short local.

@ochameau
ochameau Apr 12, 2012

I've added language attribute to core.
It looks as the best matching name for such attribute:
http://tools.ietf.org/html/rfc4646#page-5
But we can use shortLocale instead?

-or-

Rename locale to language and add shortLanguage in order to better match navigator.language (which is equal to current locale attribute)

@Gozala
Gozala Apr 12, 2012

language sounds good to me.

@Gozala Gozala commented on an outdated diff Apr 12, 2012
packages/addon-kit/lib/l10n.js
const { getRulesForLocale } = require("api-utils/l10n/plural-rules");
-// Get URI for the addon root folder:
-const { rootURI } = require("@packaging");
-
-let globalHash = {};
-let pluralMappingFunction = getRulesForLocale("en");
+// Retrieve the plural mapping function
+let gPluralMappingFunction = null;
@Gozala
Gozala Apr 12, 2012

Why prefixing it with g ? In jetpack we don't use g prefixing pattern, as we don't have globals. Also in this case it does not looks like a global.

@Gozala Gozala and 1 other commented on an outdated diff Apr 12, 2012
packages/api-utils/lib/cuddlefish.js
@@ -231,6 +231,17 @@ const Loader = {
// in the addon's context. This function loads and executes the addon's
// entry point module.
main: function main(id, path) {
+ // Try initializing localization module before running main module
+ try {
+ // Do not enable HTML localization while running test as it is hard to
+ // disable. Because unit tests are evaluated in a another Loader who
+ // doesn't have access to this current loader.
+ if (path !== "test-harness/lib/run-tests.js")
+ this.require("api-utils/l10n/html").enable();
@Gozala
Gozala Apr 12, 2012

I don't like this direction as it makes landing loader to firefox more complicated. As matter of fact in bug 743359 we're working on making loader independent of other modules in order to be able to land it to firefox. This on the other hand introduces new dependency. Unfortunately I do not have any good suggestions yet.

@ochameau
ochameau Apr 12, 2012

I totally agree with you and I don't think there is a better place for this. We are missing something for bug 743359. We just need to factors out this main method out of cuddlefish. We should not try to get rid of it. It just need to find another place.
Please see my comment in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=743359#c5
Can you just tell me if I should hold this during this discussion or can I go ahead and land this ?

@Gozala
Gozala Apr 12, 2012

I will try to submit patch that would refactor parts of loader out in a similar way you've suggested. I'd rather see this change land on top of that one.

@Gozala Gozala commented on an outdated diff Apr 12, 2012
packages/api-utils/lib/l10n/html.js
+ // Accept only HTML documents
+ if (!(document instanceof Ci.nsIDOMHTMLDocument))
+ return;
+
+ // Accept only document from this addon
+ if (document.location.href.indexOf(uriPrefix) !== 0)
+ return;
+
+ // First hide content of the document in order to have content blinking
+ // between untranslated and translated states
+ // TODO: use result of bug 737003 discussion in order to avoid any conflict
+ // with document CSS
+ document.documentElement.style.visibility = "hidden";
+
+ // Wait for DOM tree to be built before applying localization
+ document.addEventListener("DOMContentLoaded", function listener() {
@Gozala
Gozala Apr 12, 2012

Nit: would be nice if we could reuse same listener, which I think should be possible if you use event.target in the listener.

@Gozala
Mozilla member

Overall it looks good! Not quite sure why this g prefix conventions all over the place.
Only real concern is a loader change, which would require landing all of this to firefox along with loader which I don't think is a good idea. We need to find a way to decouple add-on bootstrap stuff form loader itself.

@ochameau
Mozilla member

I rebased against last master. So that html localization is now initialized in addon/runner.js.

@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/addon-kit/locale/en-GB.properties
@@ -4,6 +4,8 @@
Translated= Yes
+text-content=no <span>HTML</span> injection
@Gozala
Gozala Apr 19, 2012

Maybe <strong> or <italic> is better so it's visually distinct ?

@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/addon/runner.js
@@ -62,6 +62,16 @@ function startup(reason, options) {
if (reason === 'startup')
return wait(reason, options);
+ // Try initializing localization module before running main module
+ try {
+ // Do not enable HTML localization while running test as it is hard to
+ // disable. Because unit tests are evaluated in a another Loader who
+ // doesn't have access to this current loader.
+ if (options.loader.main.path !== "test-harness/lib/run-tests.js")
@Gozala
Gozala Apr 19, 2012

Nit: I think it would be better off assertinng against main.id instead!! (Assuming value there is `'test-harness/run-tests')

@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/addon/runner.js
@@ -62,6 +62,16 @@ function startup(reason, options) {
if (reason === 'startup')
return wait(reason, options);
+ // Try initializing localization module before running main module
+ try {
@Gozala
Gozala Apr 19, 2012

This looks like if we fail to enable localization we will just log error. Could explain why in the comment.

@Gozala Gozala and 1 other commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/l10n/core.js
+}
+
+Object.defineProperties(exports, {
+ // Returns the full length locale code: ja-JP-mac, en-US or fr
+ locale: {
+ get: function () {
+ return bestMatchingLocale;
+ }
+ },
+ // Returns the short locale code: ja, en, fr
+ language: {
+ get: function () {
+ return bestMatchingLocale.split("-")[0].toLowerCase();
+ }
+ }
+});
@Gozala
Gozala Apr 19, 2012

I don't want to be picky, but is there reason why not us just simple functions instead, like locale() or getLocale() ? You can ignore it if you like as we do this in other places as well, but it looks like we won't be able to do it with ES.next modules so I'd suggest use of plain functions in new code so we don't increase surface for incompatibilities with harmony.

@ochameau
ochameau Apr 23, 2012

That's unfortunate! It is defined as getter just because I won't be able to get locale value synchronously in the long run.
Converting them to functions just sounds like a workaround ES.next limitation :(
I have in mind to convert l10n files access to asynchronous access,
We will have to delay addon main module evaluation before l10n files are loaded.
(We have to do that, as require("l10n").get is synchronous.)
Something like this in addon/runner.js:

require("api-utils/l10n/core").load().then(function () {
  // load main
  ...
  let program = load(loader, loader.main).exports;
  ...
});

But then, the unecessary init method in l10n will become this load method and be asynchronous.
So that we won't be able to set exports.locale during module loading
Do you have any idea how to define an attribute in ES.next that can't be defined during module load?
Is this a usecase where we have to end up with functions?

@Gozala
Gozala Apr 23, 2012

ES.next won't allow dynamic properties / accessors as there is no exports object and imported modules are not an objects. You import objects / functions that were marked for export. Solution there is to export an object with such getters, which may be an option here as well.

As for async API that you've outlined above, I think it would be better to have it from day one, since usually users don't like switching from sync APIs to async. Also implications of such change may be bigger than it usually is.

Finally your then callback can return object with getters / properties l10n which may be a good alternative.

@Gozala Gozala commented on the diff Apr 19, 2012
packages/api-utils/lib/l10n/core.js
+ // Returns the short locale code: ja, en, fr
+ language: {
+ get: function () {
+ return bestMatchingLocale.split("-")[0].toLowerCase();
+ }
+ }
+});
+
+function readURI(uri) {
+ let request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].
+ createInstance(Ci.nsIXMLHttpRequest);
+ request.open('GET', uri, false);
+ request.overrideMimeType('text/plain');
+ request.send();
+ return request.responseText;
+}
@Gozala
Gozala Apr 19, 2012

Now that's yet another place we implement readURI I remember talking with @ZER0 about factoring it out to a separate module, don't know if he end up doing it, but maybe you could if he did not ?

If you prefer not to then please create a follow up bug to do that and put a comment referring to it.

@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/l10n/core.js
+ return JSON.parse(readURI(uri));
+ }
+ catch(e) {
+ console.error("Error while reading locale file:\n" + uri + "\n" + e);
+ }
+ return {};
+}
+
+// Returns the array stored in `locales.json` manifest that list available
+// locales files
+function getAvailableLocales() {
+ let uri = rootURI + "locales.json";
+ let manifest = readJsonUri(uri);
+
+ return "locales" in manifest && Array.isArray(manifest.locales) ?
+ manifest.locales : [];
@Gozala
Gozala Apr 19, 2012

Nit: It would be easier to read if you head line break just before Array.isArray in order to have complete condition in one line.

@Gozala Gozala commented on the diff Apr 19, 2012
packages/api-utils/lib/l10n/core.js
+ return rootURI + "locale/" + bestMatchingLocale + ".json";
+}
+
+function init() {
+ // First, search for a locale file:
+ let localeURI = getBestLocaleFile();
+ if (!localeURI)
+ return;
+
+ // Locale files only contains one big JSON object that is used as
+ // an hashtable of: "key to translate" => "translated key"
+ // TODO: We are likely to change this in order to be able to overload
+ // a specific key translation. For a specific package, module or line?
+ globalHash = readJsonUri(localeURI);
+}
+init();
@Gozala
Gozala Apr 19, 2012

It looks like you don't really need this function. In fact I would prefer following instead:

const globalHash = new function GlobalHash() {
   // your init function body here
}
@ochameau
ochameau Apr 23, 2012

It does not only set globalHash but inderectly set bestMatchingLocale.
I can just remove this method and put all this code in top level?
(but this method is meant to be exported, be asynchronous and be called by addon runner)
(See #387 (comment) for more info)

@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/l10n/html.js
@@ -0,0 +1,84 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const { Ci } = require("chrome");
+const observers = require("api-utils/observer-service");
+const core = require("api-utils/l10n/core");
+const { uriPrefix } = require("@packaging");
@Gozala
Gozala Apr 19, 2012

could you please use prefixURI instead.

@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/l10n/html.js
+
+// Taken from Gaia:
+// https://github.com/andreasgal/gaia/blob/04fde2640a7f40314643016a5a6c98bf3755f5fd/webapi.js#L1470
+function translateElement(element) {
+ element = element || document;
+
+ // check all translatable children (= w/ a `data-l10n-id' attribute)
+ var children = element.querySelectorAll('*[data-l10n-id]');
+ var elementCount = children.length;
+ for (var i = 0; i < elementCount; i++) {
+ var child = children[i];
+
+ // translate the child
+ var key = child.dataset.l10nId;
+ var data = core.get(key);
+ if (!data)
@Gozala
Gozala Apr 19, 2012

why not ?

if (data)
  child.textContent = data;
@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/l10n/html.js
@@ -0,0 +1,84 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const { Ci } = require("chrome");
+const observers = require("api-utils/observer-service");
@Gozala
Gozala Apr 19, 2012

Please use api-utils/system/events instead. I plan to deprecate this soonish!

@Gozala
Mozilla member

Looks good r+ but please make sure to address following comments before landing:

All others are optional and I'd leave it up to you.

Thanks!

@Gozala
Mozilla member

BTW do we really need to read all this file sync here ? Can't we do it async instead to avoid blocking firefox (specially since this happens at startup per add-on) ?

@ochameau
Mozilla member

I fixed prefixURI and used new observer event API.
I'd like to keep readURI refactoring in a followup patch as it won't be specific to html localization.
And yes, synchronous file access is bad and can be avoided. Especially now that we have addon/runner!
I can open another bug to start discussion how we should do that.
(see #387 (comment))

I didn't changed locale/language defined as getter. I'd like to avoid ending with function if possible.
There might be a way to avoid using getters while implementing asynchronous version!

May I land in these conditions?

@Gozala
Mozilla member

@ochameau I'm not able to catch on IRC, so I'm writing here. I can leave with locale and language being getters (as we already do it in other places), but would really encourage to use simple functions instead, that way we can always improve API by switching to getters, which is much better than breaking API in a future and making it less pleasing.

Also I added comment on getters and ES.next modules above. Feel free to land as is if you feel strong about it. Also if you end up with a change no need for another review.

@ochameau
Mozilla member

I've changed these attributes to functions. It isn't that big deal for me, especially if we can find ways to improve it latter if we decide to give more visibility to these attributes.

@ochameau ochameau merged commit 9bd8d3b into mozilla:master Apr 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment