906580 - [email][l10n][meta] Support dynamic language changes by setting data-l10n-id and data-l10n-args on all dynamic nodes

Status: RESOLVED FIXED

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

Description

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

There are various places in our UI where we manually call mozL10n.get() to localize a string and then set the textContent on nodes.  This works and is correct from a localization perspective as long as the language doesn't change.  When the language does change, all our nodes that had static strings set via data-l10n-id magically update, but not our computed strings.  This is sad.

I think we should change our dynamic localizations so that we always poke this information into the element so we get the dynamic language changes for free.  This may also pay off with l20n which supports dynamic dependencies that can cause retriggering.

mozL10n.localize's signature is actually localizeElement(element, id, args) and it hands the JSON.stringify call on the args, so our code needn't get any uglier.  (The comment indicates this was done so that the calls could be made prior to mozL10n being ready, but it's still great API design for this case too.  I'm sending positive karma to the l10n peeps right now.)

I'm creating this as a meta-bug since a current grep counts 7 or 8 different cards that are affected.  We may end up fixing these piecemeal.  Also, I expect that we may experience more dupe issues if we just dupe everything to a single issue.

Comment 1

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

Specific details from my comment at https://bugzilla.mozilla.org/show_bug.cgi?id=906569#c9

The signature for localizeElement is: function localizeElement(element, id, args) 

The code in question is here:
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1118

So if our bad code looks like this:
  elem.textContent = mozL10n.get(id, args);
or this:
  elem.textContent = mozL10n.get(id);

Then we would want to convert it to:
  mozL10n.localize(elem, id, args);
or (as appropriate):
  mozL10n.localize(elem, id);

Comment 2

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

I'll be happy to mentor

Comment 3

[Manuel Casas Barrado [:mancas]]

Attachment: Setting l10n attributes dynamically

Comment 4

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

Comment on attachment 8454384 [details] [review]
Setting l10n attributes dynamically

We're phasing out mozL10n.localize. Please, use mozL10n.setAttributes.

Comment 5

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

Comment on attachment 8454384 [details] [review]
Setting l10n attributes dynamically

Thanks for pursuing this!  I found one correctness issue that I noted on the pull request.  Once you account for the mozl10n deprecation issue that :gandalf references and correct that, I think this is good to land.  Please direct the final review at me (which will be quick) since an email owner/peer's review is required for landing in the email app.

Comment 6

[Manuel Casas Barrado [:mancas]]

(In reply to Andrew Sutherland [:asuth] from comment #5)
> Comment on attachment 8454384 [details] [review]
> Setting l10n attributes dynamically
> 
> Thanks for pursuing this!  I found one correctness issue that I noted on the
> pull request.  Once you account for the mozl10n deprecation issue that
> :gandalf references and correct that, I think this is good to land.  Please
> direct the final review at me (which will be quick) since an email
> owner/peer's review is required for landing in the email app.

At first, thanks for your comments. I've fixed the changes you noted on the PR, please take a look at the updated PR

Comment 7

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

I did a minor cleanup pass and landed.  Thanks again for the patch!

Landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/21719
https://github.com/mozilla-b2g/gaia/commit/72f75b0886b7aa50b8450adc14a141c4b8fae07a

Comment 8

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

Comment on attachment 8454384 [details] [review]
Setting l10n attributes dynamically

(Note that because of the cleanup patch I ended up landing on the separate pull request cited above.  I ran the linter locally including the commit hook, so I killed the other travisy jobs/etc. especially since travis has a large backlog.)

Comment 9

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

Note that this cannot be uplifted as it exists/as the v2.0 and earlier branches exist because they lack mozL10n.setAttributes.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/f0b4c93bde27a34e77f2eec6683218ff6c2d2778 because it broke Gu tests on TBPL: https://tbpl.mozilla.org/php/getParsedLog.php?id=43775531&tree=B2g-Inbound

(The top few failures in that log are unrelated to this and were dealt with elsewhere.)

Comment 11

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

Ugh, deeply sorry about that and thank you for backing out.  Forgot about https://github.com/mozilla-b2g/gaia/blob/master/apps/email/test/unit/mock_l10n.js and practiced bad build/test hygiene.

Comment 12

[Manuel Casas Barrado [:mancas]]

Attachment: Tests fixed

Comment 13

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

landed on gaia/master after clean gaia-try results:
https://github.com/mozilla-b2g/gaia/pull/22290
https://github.com/mozilla-b2g/gaia/commit/a2a0c0b81021f70b65cc55624122606eeb5a9571

Comment 14

[Chaochao Huang]

Andrew,
I have verified patch https://github.com/mozilla-b2g/gaia/commit/a2a0c0b81021f70b65cc55624122606eeb5a9571 from Comment13 on branch 1.3T and 1.4. There is no  mozL10n.setAttributes(elem, id, args) in shared/js/l10n.js, so I use mozL10n.localize(elem, id, args) instead of mozL10n.setAttributes(elem, id, args). Plz help to load on gaia/1.3T and gaia/1.4. Thanks.

Comment 15

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

Can you provide the version you tested on v1.3T/v1.4 as a pull request or a patch?  That way we don't need to reformulate the patch and then have you re-test it, etc.  Thanks.

Comment 16

[Chaochao Huang]

Attachment: bug906580_for_branch1.4

Andrew,

The attachment is patch for branch 1.4. Is it necessary to delete the file of apps/email/test/unit/mock_l10n.js ? Plz review.

There are various places in our UI where we manually call mozL10n.get(). The patch which I offer maybe cannot solve all of this kind problem. Similar problems still exist, but we didnot find them. At the same times, using  mozL10n.localize() or mozL10n.setAttributes() to solve the problem one by one is troublesome. Whether is there a better method?

Comment 17

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

I'm going to hand this off to James Burke.  But to your question, I don't think there's a better method, although we definitely are going to try and avoid using mozL10n.get in the future in the email app in favor of always using data-l10n-id/data-l10n-args, probably via mozL10n.setAttributes.  We have recently introduced a few new uses for transient toaster displays that should be refactored but which are less problematic since the toaster displays clear themselves after a few seconds and any new toasts after the language change will be in the new/correct language.  For the other l10n cleanup work we need to do, please see my comments on bug 1008859 where I explicitly called them out.

Comment 18

[James Burke [:jrburke]]

Comment on attachment 8467585 [details] [diff] [review]
bug906580_for_branch1.4

Review of attachment 8467585 [details] [diff] [review]:
-----------------------------------------------------------------

I did a quick skim of the patch file, but no testing. It looks mostly like a mechanical translation, except some of the HTML files have their data-l10n-id removed, and some of the work is done in JS instead. This seems incorrect, and I do not recommend it.

In general, I would not advise applying this to 1.4 unless you have a test case that it fixes that is worse than the existing l10n language switching problems already documented in bug 1008859. The language switching will never be 100% due to those issues.

This bug was more about converting code to the best practices for the post-1.4 l10n.js file. The l10n.js localize() on the 1.4 branch does not operate exactly like the setAtributes() on master, so it could require more testing to confirm the change does not introduce new problems. Also, by applying the change it provides more code change that may make it harder to uplift other non-l10n changes to the 1.4 branch email code.

::: apps/email/js/cards/message_list.js
@@ +1535,5 @@
> +    mozL10n.localize(msgDeleteCancel, 'message-multiedit-cancel');
> +
> +    var MsgDeleteOK = dialog.getElementsByTagName('button')[1];
> +    mozL10n.localize(MsgDeleteOK, 'message-edit-menu-delete');
> +

It is unclear why these are needed if deleteConfirmMsgNode points to delete_confirm.html, which should already be using the data-l10n attributes.

Comment 19

[Chaochao Huang]

(In reply to James Burke [:jrburke] from comment #18)

The patch bug906580_for_branch1.4 is based on patch https://github.com/mozilla-b2g/gaia/commit/a2a0c0b81021f70b65cc55624122606eeb5a9571 from Comment13. Would you have good advice? 

> It is unclear why these are needed if deleteConfirmMsgNode points to
> delete_confirm.html, which should already be using the data-l10n attributes.
Launch Email APP, go to message_list.html, no open delete_confirm.html, then go to Settings APP for changing language. Back to message_list.html, open delete_confirm.html. You will find that the language of delete_confirm.html strings donot change.

Comment 20

[James Burke [:jrburke]]

(In reply to Chaochao Huang from comment #19)
> (In reply to James Burke [:jrburke] from comment #18)
> 
> The patch bug906580_for_branch1.4 is based on patch
> https://github.com/mozilla-b2g/gaia/commit/
> a2a0c0b81021f70b65cc55624122606eeb5a9571 from Comment13. Would you have good
> advice? 

Comment 18 was my attempt at advice.

> > It is unclear why these are needed if deleteConfirmMsgNode points to
> > delete_confirm.html, which should already be using the data-l10n attributes.
> Launch Email APP, go to message_list.html, no open delete_confirm.html, then
> go to Settings APP for changing language. Back to message_list.html, open
> delete_confirm.html. You will find that the language of delete_confirm.html
> strings donot change.

This happens because of bug 1026296, which is resolved invalid on master because l10n operates differently now, where it uses mutation observers to do the translation, so we no longer needed to do a manual fix. But the fix suggested in bug 1026296 would be better to do than adding more JS code in each file and removing the data-l10n attributes in the HTML templates.

Comment 21

[Chaochao Huang]

(In reply to James Burke [:jrburke] from comment #20)
> This happens because of bug 1026296, which is resolved invalid on master
> because l10n operates differently now, where it uses mutation observers to
> do the translation, so we no longer needed to do a manual fix. But the fix
> suggested in bug 1026296 would be better to do than adding more JS code in
> each file and removing the data-l10n attributes in the HTML templates.

The issue still exists on branch 1.4. Could you tell me how the mutation observer changes in l10n.js? 
It is better that you can help merge the PR into branch 1.4. Thanks.

Comment 22

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

One really simple possibility is that we could just make the email app on v1.4 close itself if it detects a languagechange.

Comment 23

[Chaochao Huang]

(In reply to Andrew Sutherland [:asuth] from comment #22)
> One really simple possibility is that we could just make the email app on
> v1.4 close itself if it detects a languagechange.

Indeed，the method can fix the all bugs about language localization. But is how the email app closed if it detects a languagechange?

Comment 24

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

(In reply to Chaochao Huang from comment #23)
> Indeed，the method can fix the all bugs about language localization. But is
> how the email app closed if it detects a languagechange?

The same way l10n.js does it on v1.4, by listening to the mozSettings setting that indicates the locale.

Here's an example pull request against v1.4 based on the l10n.js code that should work, but I haven't tested it:
https://github.com/mozilla-b2g/gaia/pull/22604

Comment 25

[James Burke [:jrburke]]

Closing on language change is best if fully re-localized content is desired for 1.4.

Comment 26

[Chaochao Huang]

(In reply to Andrew Sutherland [:asuth] from comment #24)
> Here's an example pull request against v1.4 based on the l10n.js code that
> should work, but I haven't tested it:
> https://github.com/mozilla-b2g/gaia/pull/22604

Verify OK.

I've checked the patch on the branch v1.4 locally, it is OK. But open the Email APP again, the loading interface is message_list.html instead of the interface before changing language.

Comment 27

[Chaochao Huang]

(In reply to Andrew Sutherland [:asuth] from comment #24)
Andrew,
The method is OK. lz help to load on gaia/1.4. Thanks.

Comment 28

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

(In reply to Chaochao Huang from comment #27)
> (In reply to Andrew Sutherland [:asuth] from comment #24)
> Andrew,
> The method is OK. lz help to load on gaia/1.4. Thanks.

I filed bug 1051653 with the pull request attached and asking for approval to land on v1.4.  I've cc'ed you.

Comment 29

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

(I think maybe there's a Bugzilla bug or something with mentors... I did the "see also" from the other bug as I was filing it, which realllly should not have affected the mentor field of this bug.  I'll file a BMO bug now if I can't find an existing one.)