1005760 - [email] on language change, cached card templates not updated

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[James Burke [:jrburke]]

Once a card definition is loaded, if the language is changed, the template in that definition is still bound to the old language definition.

I believe it is enough for tmpl.js to keep track of the translated nodes it exports for module values, and then listen for language changes and then re-translate the nodes.

Need to find the event to listen for, but want to get this tracked first.

Comment 1

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

If they are in DOM, they should be retranslated automatically on language change.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

They're not currently in the DOM.

https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/tmpl.js loads text and converts them to un-rooted DOM nodes at require() time.  Those nodes never hit the DOM directly; clones are made using cloneNode(true) and those then get inserted into the DOM.

There are basically 3 options, I think:

A) What :jrburke proposes, tmpl.js just updates its fragments as time goes on.  The code is already aware of l10n.js, so it's not a big deal.

B) We change our idiom so that all template nodes get inserted into the DOM under a "display: none" node.  I think <template> might be too powerful?

C) l10n.js creates a MutationObserver.

"C" seems like it would have unacceptably high levels of overhead.

"B" has debugging upsides/downsides.  It's more noise for someone using the DOM inspector, but on the flip-side they can potentially play with things by directly manipulating things.

"B" is potentially slightly more magic/brittle since there's a greater chance that l10n.js could change in a way that does not break code.  (But higher level tests could detect this.)

"B" exposes stuff to the DOM that could have small performance impacts, but we also run the chance that naive code could get tripped up by the template nodes.  For example the python tests I think might break (although we don't care / could fix them).

"A" and "B" assume that the code, once loaded, is eternal.  But that is how our AMD modules work, so that seems fine.

So, on balance of all the stuff, I agree that "A" makes a good deal of sense.  I'd bias the implementation so there's a single module-level list of nodes to fix-up so we only have to register with l10n once so that we can do a single console.log of "re-localizing N template nodes" rather than N messages of "re-localizing template node".

Comment 3

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

(In reply to Andrew Sutherland (:asuth) from comment #2)
> C) l10n.js creates a MutationObserver.
> 
> "C" seems like it would have unacceptably high levels of overhead.

We actually are going to do this. Bug 992473 :)

Do you want to set the dependency? That would raise the priority for work in that bug for me.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Zibi Braniecki [:gandalf] from comment #3)
> (In reply to Andrew Sutherland (:asuth) from comment #2)
> > C) l10n.js creates a MutationObserver.
> > 
> > "C" seems like it would have unacceptably high levels of overhead.
> 
> We actually are going to do this. Bug 992473 :)

I agree that it's good to support that as a foolproof mode of operation, but I think apps will need to be able to opt out and email is one of them[1].   Qualcomm has filed multiple shipping-blocker bugs about checkerboarding in email.  Since the email app has explicit template boundary points, the mutation observers would just be overhead.

1: Messages and contacts will probably also want to opt out.

Comment 5

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

(In reply to Andrew Sutherland (:asuth) from comment #4)
> I agree that it's good to support that as a foolproof mode of operation, but
> I think apps will need to be able to opt out and email is one of them[1].  
> Qualcomm has filed multiple shipping-blocker bugs about checkerboarding in
> email.  Since the email app has explicit template boundary points, the
> mutation observers would just be overhead.
> 
> 1: Messages and contacts will probably also want to opt out.

Why would any app want to opt-out from mutation observer on l10n? Asking b-cause we aim at standardizing one day WebL10n with this behavior.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Zibi Braniecki [:gandalf] from comment #5)
> Why would any app want to opt-out from mutation observer on l10n? Asking
> b-cause we aim at standardizing one day WebL10n with this behavior.

So I was saying that mainly on the basis that for the virtual scrolling email does and those apps do, the DOM manipulation is on the critical path and is not something we can precalculate.  Adding any additional processing time to that inherently slows us down.

In contrast, localization is something that needs to happen as content is loaded from disk or in the (extremely rare) event that the locale changes.  In the email app, content loading already happens off the critical path.  At startup, our UI is actually loaded out of a cookie and injected via innerHTML and the actual backing code/HTML is then loaded (so we can tolerate slight inefficiency for that deferred load).  We eagerly pre-load the AMD modules for cards that might be loaded from the current code so when it comes time to actually display them, all localization that doesn't involve data/macros is already done.


Looking in more detail at the MutationObserver spec, your existing prototype that uses attributeFilter, and the Gecko implementation, I am optimistic, but I think the big questions are:

- What is the platform team's plan for implementing for webl10n "natively"?  It use of webl10n is effectively opt-in, it wouldn't surprise me if the answer is using a conditionally registered MutationObserver with an attributeListener.  (The alternative is for special-cased atom-checks for the l10n-id/l10n-data which avoids having a MutationObserver at the root of every document ever.)

- Are MutationObservers already in use in b2g apps by platform internals that would register on the document itself so the traversal cost is already being paid?  Looking at http://dxr.mozilla.org/mozilla-central/search?q=nsStubMutationObserver&case=false and following some links it looks like while mutation observers may be used on specific nodes, they're not used on the entire document and so there would be a new cost.

- Assuming there is some new overhead, what is the overhead to something like the virtual list prototype on bug 975831 or those at http://people.mozilla.org/~vladimir/misc/gfxapp/ with a) somewhat complex DOM structure for each item but no l10n-id's and b) somewhat complex DOM for each item and some l10n-id's in each item.

In the event webl10n will use mutation observers too, it's then a question of making sure any performance hits are known and sufficiently small for potentially affected apps.  Although we have the stated goal of improving the platform so that no one has to do weird hacky stuff, the reality right now is that we frequently do have to do weird/hacky stuff because our partners as well as internal product/UX demand things the platform and our hardware are not yet capable of doing.

It's probably worth talking with the Gaia performance functional team to see if they can help get the numbers since they're experienced with gathering numbers in this area and will likely be the people who would find out this is a performance problem after the fact, possibly after frustrated investigation.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andrew Sutherland (:asuth) from comment #6)
> So I was saying that mainly on the basis that for the virtual scrolling
> email does and those apps do,

Er, clarifying, I don't think contacts yet does virtual scrolling; that's bug 975680.  I also don't know if messages does yet, but I'm reasonably sure it will need to if it does not.

Comment 8

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

(In reply to Andrew Sutherland (:asuth) from comment #6)
> - What is the platform team's plan for implementing for webl10n "natively"? 

We do plan to put the target API (that is different from what l10n.js in Gaia does right now, but not that much) on the path to standardization as a WebAPI and support it natively using ShadowDOM (or sth similar) and MutationObservers.

That's a recommendation from the platform team (sicking & co).

> - Assuming there is some new overhead, what is the overhead to something
> like the virtual list prototype on bug 975831 or those at
> http://people.mozilla.org/~vladimir/misc/gfxapp/ with a) somewhat complex
> DOM structure for each item but no l10n-id's and b) somewhat complex DOM for
> each item and some l10n-id's in each item.

Good example to test again. Thanks!

> In the event webl10n will use mutation observers too, it's then a question
> of making sure any performance hits are known and sufficiently small for
> potentially affected apps.  Although we have the stated goal of improving
> the platform so that no one has to do weird hacky stuff, the reality right
> now is that we frequently do have to do weird/hacky stuff because our
> partners as well as internal product/UX demand things the platform and our
> hardware are not yet capable of doing.
>
> It's probably worth talking with the Gaia performance functional team to see
> if they can help get the numbers since they're experienced with gathering
> numbers in this area and will likely be the people who would find out this
> is a performance problem after the fact, possibly after frustrated
> investigation.

Great points. Thank you! I expect to have a more polished version of the mutation observer patch soon and I'll get in touch with the performance team ASAP.

I'd like to get MO plugged within 2.0 CL phase.

If that approach will be successful, will you want to use it for this bug?

Comment 9

[James Burke [:jrburke]]

I am a bit cautious about using MutationObservers (MO) for this, as I do believe the frequency of needing this kind of update is so very small compared to the frequency of normal DOM operations for the app. While the attribute filtering might help, some code still needs to do scan for those cases. While it may be fast, I still think it needs more proving out, and I would like to get a fix for basic operation sooner rather than later.

So I will likely proceed with the original suggestion in this bug for now, but we can revisit once MO use in l10n.js is proven out. It may be good to allow a consumer of l10n.js to opt out of MO use if it does not want to take the cost, particularly if the perf analysis suggests there are cases where it would adversely affect performance.

Comment 10

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

(In reply to James Burke [:jrburke] from comment #9)
> So I will likely proceed with the original suggestion in this bug for now,
> but we can revisit once MO use in l10n.js is proven out. It may be good to
> allow a consumer of l10n.js to opt out of MO use if it does not want to take
> the cost, particularly if the perf analysis suggests there are cases where
> it would adversely affect performance.

opting out of runtime l10n mutationobserver may be necessary if MO has noticeable cost. I agree.

But I also believe that it will be a workaround and as such I'd prefer to delay introducing it until we have proven that we need it.

So far I did not see any data that would confirm that MO bring such cost. Also, Olli, who wrote MO code, did not raise any flags to this approach.

If you believe that it may cost too much, can you help me test that hypothesis?

I just submitted a pull-request in bug 992473 which you can apply on master and test with your code. You can switch between MO-enabled and disabled by replacing code in onReady:

Instead of:
  var nodeObserver = new L20nMutationObserver(onNodeModified);
  nodeObserver.observe(null, null, !isPretranslated);

use:
  if (!isPretranslated) {
    this.translate();
  }

and you'll have the current's master behavior without MO.

I'll be doing my perf/mem profiling tomorrow, but could use more eyes on that patch :)

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Zibi Braniecki [:gandalf] from comment #8)
> (In reply to Andrew Sutherland (:asuth) from comment #6)
> > - What is the platform team's plan for implementing for webl10n "natively"? 
> 
> We do plan to put the target API (that is different from what l10n.js in
> Gaia does right now, but not that much) on the path to standardization as a
> WebAPI and support it natively using ShadowDOM (or sth similar) and
> MutationObservers.
> 
> That's a recommendation from the platform team (sicking & co).

The performance impact of MutationObserver came up at the b2g coordination summit this week and Jonas seemed somewhat concerned about the performance of MutationObservers.  He also did not rule out use of specialized attribute handling that avoids MutationObservers being involved.  (I was playing devil's advocate in favor of MutationObservers, although conceding that although they seem used for many other things, the l10n/l20n API is likely the only one that wants to listen to effectively everything in the tree.)

So I think the performance numbers are all that really matter, but if they end up being slow it does seem like MutationObserver may not be the only course of action that the native implementation pursues.

Comment 12

[James Burke [:jrburke]]

After further investigation by :gandalf, he indicated email may be better off doing the manual refreshes of the templates in the tmpl.js. Some notes for that fix if someone were to get to this bug before I can:

1) As :asuth mentioned, tmpl.js should just keep one list of all the templates, and update them in bulk.

2) the newish "languagechange" event can be used to know when the language changed. Example from the l10n.js library:

https://github.com/mozilla-b2g/gaia/blob/4152286b9cbadaa8fe2fecfa82226e99ae41a679/shared/js/l10n.js#L1338

Comment 13

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

wrt. 2) while this is true, it's even better to use our mozL10n.ready wrapper to get your code called on each retranslation.

This is just a bit safer for the future when we may want to retranslate without navigator.languages change. (LangPacks etc.)

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

See also'ing bug 906580 "[email][l10n][meta] Support dynamic language changes by setting data-l10n-id and data-l10n-args on all dynamic nodes" which is a related bug orthogonal issue.

Comment 15

[Alex Wissmann [:awiss]]

Attachment: Patch: based on suggestions in comments 12 and 13

Comment 16

[James Burke [:jrburke]]

Comment on attachment 8437237 [details] [review]
Patch: based on suggestions in comments 12 and 13

r+ if the nodes.push() location is changed, and the nits/lint issues are sorted out. With those changes, and a squash with a green travis, it should be good to land.

Comment 17

[Marcus Cavanaugh [:mcav]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/6b1cad2e5756caa48218ef5b527d96c6058de431

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Note that this is being addressed on older versions of FxOS via bug 1008859