l10n.js needs a safer startup

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
6 years ago
3 years ago

People

(Reporter: kaze, Unassigned)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 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
(Reporter)

Updated

6 years ago
Blocks: 879188
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.
blocking-b2g: --- → leo?
Blocking only because this is needed for 881985 which is leo+
blocking-b2g: leo? → leo+
(Reporter)

Updated

6 years ago
Assignee: nobody → kaze
(Reporter)

Comment 4

6 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.
Attachment #763290 - Flags: review?(timdream)
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!
Attachment #763290 - Flags: review?(timdream) → review+
(Reporter)

Comment 6

6 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!
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

Comment 8

6 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.
Hi Kaze, are we ready to land this or do we need to wait for whats coming with comment 7?
(Reporter)

Comment 10

6 years ago
My concern is mostly the blocker reported in comment 8. Working on it.
(Reporter)

Comment 11

6 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
(Reporter)

Comment 12

6 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…
Attachment #763290 - Flags: review+ → review?(timdream)
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? :-/
Attachment #763290 - Flags: review?(timdream)
(Reporter)

Comment 14

6 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.

Updated

6 years ago
Whiteboard: [TD-44870]
Target Milestone: --- → 1.1 QE3 (26jun)

Updated

6 years ago
Whiteboard: [TD-44870]
Target Milestone: 1.1 QE3 (26jun) → 1.1 QE4
Kaze, do you think this bug here will also cover bug 886715?
Flags: needinfo?(kaze)
(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.
(Reporter)

Comment 17

6 years ago
Makes sense. Working on that.
Flags: needinfo?(kaze)
(Reporter)

Comment 18

6 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.
No longer blocks: 888206

Comment 19

6 years ago
Bug 881985 is resolved. so remove leo+.
blocking-b2g: leo+ → leo?
blocking-b2g: leo? → -
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.
Hey Kaze, what's missing here ? :) Maybe gnarf can help you finishing this patch ?
Adding new "Blocks" bug here for similar symptom. Please feel free to correct if I am wrong.
Blocks: 907599
Blocks: 908541
I'd love to work on this after bug 914414 lands.
Component: Gaia → Gaia::L10n
Depends on: 914414
Heads up. In bug 993189 Stas is adding mozL10n.once and in bug 993188 we're going to clean up mozL10n.ready
See Also: → bug 993189, bug 993188
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
Assignee: fabien → nobody
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: 1.1 QE4 (15jul) → ---
Blocks: 999779
l10n.js is deprecated now.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.