1013207 - [Keyboard][Performance] Lazily load l10n.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect, P2)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

+++ This bug was initially created as a clone of Bug #970188 +++

It looks like even with bug 1007558 we still need to parse and run l10n.js before anything that is dynamically loaded (e.g. layout/en.js). We should do that afterwards if there isn't a side effect.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP

Need more test

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The patch will save us around 50ms but I am not sure if it will regress l10n.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

OK, I figured out how to not regress ariaLabel. In fact, I am able to fix the current bug where the first layout being drawn will have no aria label since l10n.js is not ready.

The trick is to set the dataset.l10nId and mozL10n.translate() the element instead of using mozL10n.get(). If translate() is not available, l10nId in the DOM will result mozL10n to translate the document when it is ready.

(and, we can get rid of the translate() call when bug 994519 lands)

diff --git a/apps/keyboard/js/render.js b/apps/keyboard/js/render.js
index a6e1149..6815daf 100644
--- a/apps/keyboard/js/render.js
+++ b/apps/keyboard/js/render.js
@@ -857,13 +857,15 @@ const IMERender = (function() {
   };
 
   var candidatePanelCode = function() {
-    var _ = (navigator.mozL10n &&
-             navigator.mozL10n.readyState === 'complete') ?
-      navigator.mozL10n.get : function(x) { return x };
+    var __ = (navigator.mozL10n &&
+              navigator.mozL10n.readyState === 'complete') ?
+      navigator.mozL10n.translate : function() { };
 
     var candidatePanel = document.createElement('div');
     candidatePanel.setAttribute('role', 'group');
-    candidatePanel.setAttribute('aria-label', _('wordSuggestions'));
+    candidatePanel.dataset.l10nId = 'wordSuggestions';
+    __(candidatePanel);
+
     candidatePanel.classList.add('keyboard-candidate-panel');
     if (inputMethodName)
       candidatePanel.classList.add(inputMethodName);
@@ -872,7 +874,9 @@ const IMERender = (function() {
     dismissButton.classList.add('dismiss-suggestions-button');
     dismissButton.classList.add('hide');
     dismissButton.setAttribute('role', 'button');
-    dismissButton.setAttribute('aria-label', _('dismiss'));
+    dismissButton.dataset.l10nId = 'dismiss';
+    __(dismissButton);
+
     candidatePanel.appendChild(dismissButton);
 
     var suggestionContainer = document.createElement('div');


The problem is that after having some fun with properties files I realized I cannot set

wordSuggestions.ariaLabel=Word suggestions

without setting

wordSuggestions=

to something first. I don't want l10n.js to set the textContent of these elements so set it to anything, including empty string, is wrong. So, :gandalf, do we have this feature available in l10n.js? Keyboard localization is entirely about aria labels only.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #3)
> The problem is that after having some fun with properties files I realized I
> cannot set
> 
> wordSuggestions.ariaLabel=Word suggestions
> 
> without setting
> 
> wordSuggestions=
> 
> to something first. I don't want l10n.js to set the textContent of these
> elements so set it to anything, including empty string, is wrong.

That's false. You definitely can have an entity with only an attribute and it will not set textContent at all. We use it in many places.

I'd have to see the code that you used to test this assumption to find why you got the wrong conclusion.

Also, Instead of creating no-ops, I'd recommend just not setting data-l10n-id until localization is available:

navigator.mozL10n.once(function() {
  element.dataset.l10nId = 'wordSuggestions');
  navigator.mozL10n.translate(element); // to be removed once bug 992473 lands

  element2.dataset.l10nId = 'dismiss');
  navigator.mozL10n.translate(element); // to be removed once bug 992473 lands

});

Does it make sense?

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Yeah you are right about id.ariaLabel, I don't know why it didn't work for me for the first time.

So I am now trying to trick mozL10n to translate elements that is dynamically added into the DOM *before* mozL10n, but must be translated when it loads.

It seems that l10n.js do

isPretranslated = (document.documentElement.lang === navigator.language);

so

document.documentElement.lang = 'x-untranslated';

can trick it to do so.

I cannot use once() in my translate function since mozL10n did not exist yet.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP

The patch is dependent on bug 1013155 -- I had to fix a mozL10n.get() race there so there are conflict between two. Please look at the translateElement() wrapping function and give me some feedback on that.

I can verify the DOM is indeed translated by inspecting it with App Manager afterwards, for element inserted both before and after mozL10n.

Comment 7

[Staś Małolepszy :stas]

Tim, I wonder if a better solution would be to add ready() and once() methods to LazyL10n, which would serve as proxies to their real counterparts.  LazyL10n is a tiny file and this would allow you to safely do:

LazyL10n.once(function() {
    element.setAttribute('data-l10n-id', 'wordSuggestions');
});

Without having to bother to check for the existence of mozL10n or its readyState.

I can help with the implementation.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am not convinced that we should be using LazyL10n, which is depend on LazyLoader :P so it's a little bigger than you think. Anyhow, I believe everything here is a workaround/migration to bug 992473 and the fact my team screw up the start-up order of keyboard app. Once we fix all that translateElement() and the ugly setTimeout() will be gone.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8427594 [details] [diff] [review]
WIP

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

::: apps/keyboard/js/keyboard.js
@@ +316,5 @@
>    }
> +
> +  // Force l10n.js to think we are not pre-translated, and it must transverse
> +  // the DOM when it loads.
> +  document.documentElement.lang = 'x-untranslated';

Can you add a note stating that once we fix bug 1022889 this will not be needed?

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Zibi Braniecki [:gandalf] from comment #9) 
> Can you add a note stating that once we fix bug 1022889 this will not be
> needed?

Sure, I love the fact you push for zerrrro direct calls to mozL10n functions from app!

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I will be starting working on this after bug 1015643 lands.

Comment 12

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#20287

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8437594 [details] [review]
mozilla-b2g:master PR#20287

I removed the setTimeout() and move the loading to a loader module, and call it when after anything at the critical launch are loaded.

I also leveraged the JS int tests to ensure we always have aria-label set eventually, as I didn't have anything in the code to tell when l10n is ready. I believe the test will keep us from regressing.

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8437594 [details] [review]
mozilla-b2g:master PR#20287

r=me with some nits to be addressed.

Thank you.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=2a141eb0a10f

https://travis-ci.org/mozilla-b2g/gaia/builds/27283742

Review comment addressed.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

master: https://github.com/mozilla-b2g/gaia/commit/48e200f9631c191a3bc0819f04f879c531221055

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> (In reply to Zibi Braniecki [:gandalf] from comment #9) 
> > Can you add a note stating that once we fix bug 1022889 this will not be
> > needed?
> 
> Sure, I love the fact you push for zerrrro direct calls to mozL10n functions
> from app!

Aand it landed. Want me to file a separate bug to update this, or just patch in this one?