Closed Bug 868696 Opened 11 years ago Closed 11 years ago

Sort out events

Categories

(L20n :: HTML Bindings, defect, P1)

Product:
L20n
Component:
HTML Bindings
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
1.0 beta

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Details

Attachments

(1 file, 1 obsolete file)

Emit DocumentLocalized only once
2.05 KB, patch
zbraniecki
: review+
Details | Diff | Splinter Review 
We currently fire two global events:

LocalizationReady
DocumentLocalized

We need a clear description on when those two are fired/refired depending on retranslation.

Stas - do you remember what we agreed upon at FrontTrends?
Assignee: nobody → gandalf
Target Milestone: --- → 1.0 beta
IIRC, we wanted to fire LocalizationReady every time the language changes and DocumentLocalized only once, when HTML bindings finish the initial translation of the DOM.

The rationale was that all UI logic that needs to be run on each language change should live in ctx.localize or ctx.addEventListener('ready').  This includes HTML bindings' own call to localize.

Looking at Gaia, their 'localized' event is fired every time the user switches the language.  For instance, this is what the Browser app does:

        mozL10n.ready(function browser_localizeElements() {
          elementsToLoad.forEach(function l10nElement(element) {
            mozL10n.translate(element);
          });
        });

(mozL10n.ready simply does window.addEventListener('localized', callback);)


Other apps, like Gallery, take a different approach:

/ The localized event is the main entry point for the app.
// We don't do anything until we receive it.
window.addEventListener('localized', function showBody() {
  window.removeEventListener('localized', showBody);

  // Set the 'lang' and 'dir' attributes to <html> when the page is translated
  document.documentElement.lang = navigator.mozL10n.language.code;
  document.documentElement.dir = navigator.mozL10n.language.direction;

  // <body> children are hidden until the UI is translated
  document.body.classList.remove('hidden');

  // Now initialize the rest of the app. But don't re-initialize if the user
  // switches languages when the app is already running
  if (!photodb)
    init();
});

And then, all other code is in a 'localized' handler, like here:  https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/open.js

Based on this, we might want to reconsider firing DocumentLocalized more than once.

* * *

I see some redundancy here, too.  Both of the following two things do exaclty the same thing:

  - document.addEventListener('LocalizationReady', fn)
  - document.l10n.addEventListener('ready', fn)

Should we just keep the second one?

* * *

BTW there's also LocalizationFailed which used to check for error codes defined on L20n.  I think we can just remove it because of how fallbacks work right now.  (LocalizationFailed comes from the era of thinking about "integral" subcontexts (equivalent of today's Locale).  An integral subctx was a context which had all resources loaded properly.  If the subctx wasn't integral, we would immediately fall back to the next one.)
Attached patch patch 1 (obsolete) — — Splinter Review 
patch.

1) Removed LocalizationFailed
2) Removed LocalizationReady
3) Added console.log to ctx errors
Attachment #746041 - Flags: review?(stas)
Comment on attachment 746041 [details] [diff] [review]
patch 1

Actually, the patch is not enough.

Right now we're firing DocumentLocalized on each retranslate, but we really only want to do this either:

1) The first time we localize document
2) After each language switch

The 2) is very hard to achieve because the retranslate is asynchronous, so we'd have to pass information on the reason why it has been fired.
I suggest we go for 1) and fire only one DocumentLocalized

That leaves us with document.10n.LocalizationReady fired after each locale switch, but fired before the document has been retranslated with no real way to plug your code after that.

Is that ok for 1.0 Stas?
Attachment #746041 - Flags: review?(stas) → feedback?(stas)
I need to think about this a little bit more and test some code.  I'll come back with an answer tomorrow.
Priority: -- → P1
How about this:

 1. ready is emitted every time the locale list changes,
 2. DocumentLocalized is emitted only once,
 3. for compatibility with Gaia, bindings/l20n/gaia (bug 872260) would emit 'localized' on every locale change.

Is 3. even needed or can we use 1. for that?
1) document's event or context's event?

I'm not convinced we need 3). It doesn't give us anything and transition to l20n should be pretty explicit anyway.
(In reply to Zbigniew Braniecki [:gandalf] from comment #6)
> 1) document's event or context's event?

Context.
Patch in attachment 746041 [details] [diff] [review] landed as part of bug 868677.

This patch changes DocumentLocalized to be emitted only once per document.  As such it can be used as a replacement for DOMContentLoaded.

It also fixes a regression introduced in bug 868677 in Context::registerLocales.
Attachment #746041 - Attachment is obsolete: true
Attachment #746041 - Flags: feedback?(stas)
Attachment #750955 - Flags: review?(gandalf)
Attachment #750955 - Flags: review?(gandalf) → review+
Landed in https://github.com/l20n/l20n.js/commit/bf975cc01a1b4052b09b3498eb6b0f21dfb1a896
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: