Closed Bug 1013207 Opened 9 years ago Closed 9 years ago

[Keyboard][Performance] Lazily load l10n.js

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S4 (20june)
Tracking Status
b2g-v2.1 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

(Keywords: perf, Whiteboard: [c=progress p= s=2014.06.20.t u=] [save=~50ms][p=3])

Attachments

(1 file, 2 obsolete files)

+++ 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.
Attached file WIP (obsolete) —
Need more test
Keywords: perf
Priority: P1 → P2
Whiteboard: [c=progress p= s= u=2.0] → [c=progress p= s= u=]
The patch will save us around 50ms but I am not sure if it will regress l10n.
Whiteboard: [c=progress p= s= u=] → [c=progress p= s= u=] [save=~50ms]
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.
Flags: needinfo?(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?
Flags: needinfo?(gandalf)
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.
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #8425391 - Attachment is obsolete: true
Attachment #8427594 - Flags: feedback?(gandalf)
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.
Flags: needinfo?(timdream)
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.
Flags: needinfo?(timdream)
Whiteboard: [c=progress p= s= u=] [save=~50ms] → [c=progress p= s= u=] [save=~50ms][p=3]
Target Milestone: --- → 2.0 S3 (6june)
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
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?
Attachment #8427594 - Flags: feedback?(gandalf) → feedback+
(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!
I will be starting working on this after bug 1015643 lands.
Attachment #8427594 - Attachment is obsolete: true
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.
Attachment #8437594 - Flags: review?(rlu)
Comment on attachment 8437594 [details] [review]
mozilla-b2g:master PR#20287

r=me with some nits to be addressed.

Thank you.
Attachment #8437594 - Flags: review?(rlu) → review+
master: https://github.com/mozilla-b2g/gaia/commit/48e200f9631c191a3bc0819f04f879c531221055
No longer blocks: 1023729
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [c=progress p= s= u=] [save=~50ms][p=3] → [c=progress p= s=2014.06.20.t u=] [save=~50ms][p=3]
No longer blocks: 1025633
Depends on: 1025633
(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?
You need to log in before you can comment on or make changes to this bug.