Fabien Cazenave [:kaze] Reporter
	Description • 11 years ago

As Ian-Liu pointed out recently, there are a few cases where the `localized' event can be missed: this race condition is not easy to reproduce, but it leads to apps failing to initialize. Bug 879188 is probably a symptom of this issue (among other bugs).


1/ This is the main reason why I added the mozL10n.ready() method: instead of using

  window.addEventListener('localized', myInitFunction);

it is safer to use

  mozL10n.ready(myInitFunction);

because in case the l10n resources are already loaded, it triggers the callback immediately, without waiting for a `localized' event; and if the l10n resources aren’t ready yet, a `localized' event listener is used.


2/ Tim has also spotted the reason why l10n.js can’t be deferred, see his comment on the webL10n page [1]. He’s proposed two patches which I’ve just merged, but they haven’t been backported to Gaia yet.


Proposal:
 • sync l10n.js with the latest lib on the webL10n repository to ensure the startup is consistent;
 • spot all « addEventListener('localized') » in Gaia and use « mozL10n.ready() » instead.


Note that some apps could be initialized without waiting for the `localized' event at all, see bug 882591.

[1] https://github.com/fabi1cazenave/webL10n/issues/30

	David Flanagan [:djf]
	Comment 1 • 11 years ago

Given the nasty nature of the symptom for Gallery (fixed in bug 880943) I'm proposing that we block on this because we really should stop using the localized event for startup.

	bhavana bajaj [:bajaj]
	Comment 2 • 11 years ago

Blocking only because this is needed for 881985 which is leo+

	Fabien Cazenave [:kaze] Reporter
	Comment 3 • 11 years ago

https://github.com/mozilla-b2g/gaia/pull/10410

	Fabien Cazenave [:kaze] Reporter
	Comment 4 • 11 years ago

Comment on attachment 763290 [details]
link to pull request — WIP

https://github.com/mozilla-b2g/gaia/pull/10410

 • sync l10n.js with the upstream version;
 • use `mozL10n.ready()' rather than waiting for a `localized' event in System and Settings apps.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 5 • 11 years ago

Comment on attachment 763290 [details]
link to pull request — WIP

I wonder if |webL10n.ready()| is the best approach here.

-- It still tangles l10n startup with application start-up -- I think with webL10n.localize(element) in bug 882591, once all apps get rid of webL10n.get(element) calls, app start-up should be able to decouple with webL10n readiness safely(*1).
-- IMHO it is really not a problem on showing people an screen without strings. It's the prefect start-up splash. But it would still make sense to use |webL10n.ready()| for people who doesn't like my point here.
-- What about restartless locale change? If we replace 'localize' event listener with |ready()|, the application UI should change w/o a problem right(*2)?


*1 Except for the place where you cannot get rid of get() calls like |alert(_(stringId))|.
*2 I imagine it would, except when webL10n user use it incorrectly e.g. do |el.textContent = _('id')| without |el.dataset.l10nId = id;|

So my concrete suggestion would be, this patch looks good, but we need to actually audit the usage of webL10n within Gaia to see if for each app, we could (1) remove webL10n from the start-up path, (2) use localize() instead of get().

Let's file bugs on these and have people working on them.

Thank you!

	Fabien Cazenave [:kaze] Reporter
	Comment 6 • 11 years ago

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #5)
> Comment on attachment 763290 [details]
> link to pull request — WIP
> 
> I wonder if |webL10n.ready()| is the best approach here.
> 

It’s better than waiting for a `localized' event that might have been sent already, but as you wrote: it’s not the best approach.

> -- It still tangles l10n startup with application start-up -- I think with
> webL10n.localize(element) in bug 882591, once all apps get rid of
> webL10n.get(element) calls, app start-up should be able to decouple with
> webL10n readiness safely(*1).

Exactly.

> -- IMHO it is really not a problem on showing people an screen without
> strings. It's the prefect start-up splash. But it would still make sense to
> use |webL10n.ready()| for people who doesn't like my point here.

I agree with this too, as far as I’m concerned.

> -- What about restartless locale change? If we replace 'localize' event
> listener with |ready()|, the application UI should change w/o a problem
> right(*2)?
> 

Right: webL10n.ready should behave exactly like a `localized' event listener when the user language is changed.

> So my concrete suggestion would be, this patch looks good, but we need to
> actually audit the usage of webL10n within Gaia to see if for each app, we
> could (1) remove webL10n from the start-up path, (2) use localize() instead
> of get().
> 
> Let's file bugs on these and have people working on them.

Cannot agree more: this patch is just a first step, the real improvement will be to use localize() instead of ready() + get().

Thanks for your time!

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 7 • 11 years ago

Comment on attachment 763290 [details]
link to pull request — WIP

The patch breaks retranslation of any code localized within ready() wrapper.

Current behavior of ready wrapper [1] is inconsistent - it does not register event listener in the scenario when gReadyState is complete at the time when ready is called.

The solution should be similar to what l20n did in bug 884810

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1005

	hyuna.cho
	Comment 8 • 11 years ago

I try to apply the patch in leo but the device doesn't run to homescreen.
The device keep stopping on start-up animation.
Please check this patch in leo.

	Wayne Chang [:wchang]
	Comment 9 • 11 years ago

Hi Kaze, are we ready to land this or do we need to wait for whats coming with comment 7?

	Fabien Cazenave [:kaze] Reporter
	Comment 10 • 11 years ago

My concern is mostly the blocker reported in comment 8. Working on it.

	Fabien Cazenave [:kaze] Reporter
	Comment 11 • 11 years ago

(In reply to Zbigniew Braniecki [:gandalf] from comment #7)
> Comment on attachment 763290 [details]
> The patch breaks retranslation of any code localized within ready() wrapper.

Thanks Gandalf, this should be fixed now:
https://github.com/mozilla-b2g/gaia/pull/10410/files#L14R1032

	Fabien Cazenave [:kaze] Reporter
	Comment 12 • 11 years ago

Comment on attachment 763290 [details]
link to pull request — WIP

https://github.com/mozilla-b2g/gaia/pull/10410

(In reply to hyuna.cho from comment #8)
> I try to apply the patch in leo but the device doesn't run to homescreen.
> The device keep stopping on start-up animation.
> Please check this patch in leo.

The problem was related to our build system, it should be fixed now.
Clearing the review flag…

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 13 • 11 years ago

Comment on attachment 763290 [details]
link to pull request — WIP

I am deferring my review here because I don't think comment 7 the right behavior of |ready()| method. I expect ready() to execute callback _once_ when the content is localized; if some page really need to do something, it still has too register |localized| event manually. We are now using |ready()| in the app init -- app aren't support to be init'd again when the locale changed, right? :-/

	Fabien Cazenave [:kaze] Reporter
	Comment 14 • 11 years ago

Tim, to be fair that’s the rationale I had in mind: ready() should be started once when the page has been translated after DOMContentLoaded, whereas the “localized” event should be sent whenever the locale is changed.

However, Gandalf is right when he says that it will change the behavior we had with window.addEventListener('localized'). Worse: the current `mozL10n.ready()' behavior (= before patch) adds a `localized' event listener if mozL10n.readyState is not complete, but only triggers the callback once if complete.

Gandalf’s proposal is the easiest way to ensure consistency; but if we want `mozL10n.ready()' to start only once (which would make sense, imho) it will require a bit more changes:

 • make sure the `localized' callback is triggered only once (e.g. by using an array of callbacks rather than event listeners in l10n.js);

 • everywhere in the current code where `addEventListener('localized')' is used, make sure this event listener is added after mozL10n.ready():
  navigator.mozL10n.ready(function startup() {
    init(); // init whole app
    window.addEventListener('localized', updateUI);
  });

Tim, let me know if you prefer me to do this, it wouldn’t take me that long.

Note that when the locale is changed, the whole HTML document is automatically re-translated by l10n.js; if all elements that have been dynamically inserted into the DOM carry data-l10n attributes (hence the async mozL10n.localize() method), the whole page should be fine and probably won’t require any `localized' event listener.

Note also that we might get a `languagechanged' event soon, see bug 780953.

	Wayne Chang [:wchang]
	Comment 15 • 11 years ago

Kaze, do you think this bug here will also cover bug 886715?

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 16 • 11 years ago

(In reply to Fabien Cazenave [:kaze] from comment #14)
>   navigator.mozL10n.ready(function startup() {
>     init(); // init whole app
init() implies initUI() too.
>     window.addEventListener('localized', updateUI);
>   });

This is the use case I have in my mind. So for apps who doesn't need to updateUI() on their own (i.e. reply on data-l10n-id entirely), it could skip the |addEventListener| part.

	Fabien Cazenave [:kaze] Reporter
	Comment 17 • 11 years ago

Makes sense. Working on that.

	Fabien Cazenave [:kaze] Reporter
	Comment 18 • 11 years ago

(In reply to Wayne Chang [:wchang] from comment #15)
> Kaze, do you think this bug here will also cover bug 886715?

Yes, this is related.

	Leo
	Comment 19 • 11 years ago

Bug 881985 is resolved. so remove leo+.

	Corey Frang [:gnarf]
	Comment 20 • 11 years ago

Looking for an update here - I was about to rewrite the app-startup sequence in clock and want to know if I should write it like this patch is landing.

	Julien Wajsberg [:julienw]
	Comment 21 • 11 years ago

Hey Kaze, what's missing here ? :) Maybe gnarf can help you finishing this patch ?

	Wayne Chang [:wchang]
	Comment 22 • 11 years ago

Adding new "Blocks" bug here for similar symptom. Please feel free to correct if I am wrong.

	Staś Małolepszy :stas
	Comment 23 • 10 years ago

I'd love to work on this after bug 914414 lands.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 24 • 10 years ago

Heads up. In bug 993189 Stas is adding mozL10n.once and in bug 993188 we're going to clean up mozL10n.ready

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 25 • 10 years ago

This is a multipart effort that at the moment consists:

* Bug 993188 - add mozL10n.once
* Bug 992473 - switch to use MutationObservers for runtime DOM operations
* Bug 993188 - clean up uses of mozL10n.ready and remove race conditions
* Bug 993193 - Move runtime localization logic post document loading for pretranslation case 
* Bug 994357 - Clean up heuristics for localization of node content 
* Bug 994370 - Improve app startup for non-default locale 
* Bug 994519 - Clean up mozL10n.localize API

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 26 • 8 years ago

l10n.js is deprecated now.