Like Doug said[1], it's preferable to use an l10n ID instead of the "node.textContent = _('via-sim', {n: n});" pattern. [1] https://github.com/mozilla-b2g/gaia/pull/25843/files#r20816223 [2] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/handled_call.js#L73

[1] https://github.com/mozilla-b2g/gaia/pull/25843#discussion-diff-20816223

Comment on attachment 8608218 [details] [gaia] gabrielesvelto:bug-1104667-callscreen-l10n > mozilla-b2g:master This is a first try to address this issue that also touches other uses of obsolete l10n functions (and do not support proper language switching, an issue that's been present in the callscreen for a long time). Here's a few details: - The HandledCall code has been overhauled and a lot of redundancy removed from it. Most of the methods that deal with updating phone numbers and other information now both accepts raw strings (for the numbers) as well as data-l10n-id/args combinations for strings or composite structures (such as "number, operator"). - I've used a mix of standard properties and a new l20n file to tackle the problem. The strings in the l20n file are deliberately different from their properties equivalent so that they won't clash. - The LazyL10n object has been removed and all the relevant code modified not to use it anymore (including tests). - Unit-tests have been adjusted accordingly. There's still a few problems with this patch: - When applied sometimes the gaia build process gets stuck in an xpcshell invocation. I'm not sure why and will investigate it. - The time & date format code hasn't been upgraded yet and still relies on mozL10n.get(). Mostly because I don't know how to handle it yet and it's probably material for a separate bug.

Comment on attachment 8608218 [details] [gaia] gabrielesvelto:bug-1104667-callscreen-l10n > mozilla-b2g:master I've pushed an updated patch and replaced the old one; I didn't push it on top of the previous one as there were a lot of changes and they replaced large parts of the code of the first patch so there was no benefit in seeing the incremental changes. Long story short this should be good to go (pending testing). Here's a high-level list of the changes I made: - I've ripped out LazyL10n and removed all remaining uses of it, good riddance. This also removed plenty of asynchronous bits that were highly race-prone. - I've refactored the HandledCall code to reduce redundancy and make certain operations clearer. - Practically all localized elements in the callscreen should now respond properly to language changes. - All code that passed around localized data is now using the approach described in https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs - Unit-tests were overhauled to check for data-l10n-* attributes and I've added a helper that checks all patterns described in the localization guidelines. I've got just one final important question regarding a specific localization use-case. In some places we need the localized string so that we can adjust the font size to make it fit into an element. I've explicitly used mozL10n.translateFragment() instead of mozL10n.get() to force synchronous translation while still using data-l10n-* attributes. I'm not sure if this is the best way to go but it seemed the only sensible one. One drawback of this approach is that if the language changes we won't re-adjust the font; I haven't found a way to do that yet and I'll leave it for another bug since we're trying to resize more strings anyway.

Ouch, I just realized that I pushed the wrong, unsquashed and incomplete patch. Though some of Zibi's comments are still spot on, I'll push the right one right away.

Hey :gsvelto. Great patch! I left some comments, but overall it looks great. One major comment is that as much as I want to start using .l20n files in production, our build system is not ready yet. We're waiting for bug 1158976 to land and until then, we don't want to use .l20n in production code. So in order to get this landed soon (which will help us test v3 API against communitcations/dialer!) I'd suggest switching that new file to .properties. I hope we'll have a green light to use .l20n in prod. soon!

One more thing. There are two files that use mozL10n.get in dialer. One is mmi.js and seems like a complex problem, another is mmi_ui.js which seems simple to fix. I wrote a patch: https://pastebin.mozilla.org/8835040 Do you want to add it to your patch or do you prefer to tackle that together with mmi.js later?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7) > Do you want to add it to your patch or do you prefer to tackle that together > with mmi.js later? I'd like to tackle it in bug 1165332. I've put that one on hold for now as it required me to rethink my approach a little bit. The code in mmi.js is pretty terrible and I'm writing a full-fledged refactor of it. Since we're still very early in the release cycle for 3.0 this is the right time for such large changes.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6) > One major comment is that as much as I want to start using .l20n files in > production, our build system is not ready yet. We're waiting for bug 1158976 > to land and until then, we don't want to use .l20n in production code. > > So in order to get this landed soon (which will help us test v3 API against > communitcations/dialer!) I'd suggest switching that new file to .properties. > I hope we'll have a green light to use .l20n in prod. soon! Will do, there's nothing here which depends on l20n-specific functionality anyway though I did this work with the intent of poking the brand new l20n stuff. BTW I've noticed an issue which might be a build system bug. In one of the early patches due to a type I had an entity reference which lacked the actual definition, something like this: <phone_type_mobile_and_carrier "Mobile, {{ carrier }}"> Were 'carrier' wasn't defined. This caused the gaia build to get stuck somewhere with 100% CPU usage. I haven't figured out what the exact program was.

(In reply to Gabriele Svelto [:gsvelto] from comment #9) > Will do, there's nothing here which depends on l20n-specific functionality > anyway though I did this work with the intent of poking the brand new l20n > stuff. BTW I've noticed an issue which might be a build system bug. In one > of the early patches due to a type I had an entity reference which lacked > the actual definition, something like this: > > <phone_type_mobile_and_carrier "Mobile, {{ carrier }}"> > > Were 'carrier' wasn't defined. This caused the gaia build to get stuck > somewhere with 100% CPU usage. I haven't figured out what the exact program > was. I tried to reproduce it and failed. I opened dev_apps/l20n-app and modified example.en-US.l20n's entity hello by adding {{ notExisting }} to the string. Then I `APP=l20n-app make profile` and I rebuild the app and pretranslated and concatenated locales.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10) > Then I `APP=l20n-app make profile` and I rebuild the app and pretranslated > and concatenated locales. I can't repro it either, it must have been some other issue. In the meantime I've addressed almost all of your review comments in a new patch save for the string we automatically resize. Here's a summary of the changes: - Removed the l20n file and used a regular properties one - Removed the last synchronous mozL10n.get calls in the call screen code - Added proper aria labels for the incoming/outgoing icons - Fixed a small issue in the mozL10n mock when using formatValue() without arguments

(In reply to Gabriele Svelto [:gsvelto] from comment #11) > (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10) > > Then I `APP=l20n-app make profile` and I rebuild the app and pretranslated > > and concatenated locales. > > I can't repro it either, it must have been some other issue. In the meantime > I've addressed almost all of your review comments in a new patch save for > the string we automatically resize. Cool! I like it :) What do you want to do about the automatic resize piece?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12) > Cool! I like it :) > > What do you want to do about the automatic resize piece? I'm fixing a similar issue in bug 1155901 which is smaller than this one. If that works then I'll try to use the same approach here. I've rebased the patches as they had bit-rotted in the meantime.

Comment on attachment 8608218 [details] [gaia] gabrielesvelto:bug-1104667-callscreen-l10n > mozilla-b2g:master Great! I love the patch, let's get it landed!

Rebased on top of bug 1155901 so we don't have to handle the string resizing here.

Bit-rotted and rebased again, basically all changes to build/webapp-optimize.js have been removed.

Re-based again. Doug will you have time to review this anytime soon?

Comment on attachment 8608218 [details] [gaia] gabrielesvelto:bug-1104667-callscreen-l10n > mozilla-b2g:master This patch needs more work after the recent rebasing, clearing the review flag for now.

Gabriele: I accidentally wrote an inferior version of your patch in bug 1104667. If you'll find anything in that patch that you'd like to use in yours, feel free :)

Ugh, in bug 1187556.

Thanks, I'll have a look. Unfortunately this is not working yet because some of the nodes we set attributes on have child elements. I forgot about the fact that this scenario didn't work and it took me a while to figure out why setting the attributes was eating all the sub-nodes of certain elements :-/ I'll try to finish this soon but it will require further refactoring in the HTML code too.

Ok! Let me know if I can help. I don't know if you need those sub-nodes or not. We do support limited DOMFragment localization with DOMOverlays and they're designed to help with scenarios where you want the localizer to localize whole DOMFragment, not just a node. But if it's about the bdiCount, then I'd suggest setting l10nId/l10nArgs on the bdiCount element.

Comment on attachment 8608218 [details] [gaia] gabrielesvelto:bug-1104667-callscreen-l10n > mozilla-b2g:master Updated, rebased patch that now works everywhere. I didn't push on top of the old one because stuff got really messy so I just squashed everything together, the changes are minimal though: - In the HandledCall object I've explicitly used Node.setAttribute() instead of mozL10n.setAttributes() for setting the aria label otherwise the l10n code would squash all the call's main node children. IIRC .ariaLabel support is coming in l10n so hopefully this can be changed at a later time. - The call group UI now asynchronously shrinks the 'call ended' string using a MutationObserver, this means that we don't need the mozL10n.translateFragment synchronous call anymore there. - I've fixed up a couple of mocks in the meantime - All remaining nits from :gandalf comments on the PR have been addressed The rest is still as described in comment 4.

Comment on attachment 8608218 [details] [gaia] gabrielesvelto:bug-1104667-callscreen-l10n > mozilla-b2g:master Sorry for the delay here. It's good. I left a few comments, but overall it's great to see these changes.

Thanks for the review! I've addressed the nits and added a whole bunch of unit-tests for the new Utils.* methods which were untested (silly me): this allowed me to spot and fix a couple of issues too.

Comment on attachment 8608218 [details] [gaia] gabrielesvelto:bug-1104667-callscreen-l10n > mozilla-b2g:master This is awesome! Land it pls! :)

Thanks for the reviews, there's some orange in the tests but it seems unrelated to my patch and I saw it in other PRs too. I also don't get failures locally. Merged to gaia/master 9002fdc0abc2e758c69e1ac48bea6395947a0afb https://github.com/mozilla-b2g/gaia/commit/9002fdc0abc2e758c69e1ac48bea6395947a0afb

I had to revert this for Gu failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2441174&repo=b2g-inbound https://github.com/mozilla-b2g/gaia/commit/a83366dc9f73182bb43d06b22337670aa5d71cb6

In shared/test/unit/mocks/mock_l10n.js: - return Promise.resolve(stringify(id, args || '')); + return Promise.resolve(stringify(id, args)); This shouldn't be part of the patch. Drs, do you want to fix it and reland, or do you want me to do that?

I'm taking it from gsvelto. It's a minor test bustage and I was about to make the same change in bug 1187627, so I'll fix it here.

Sorry for trouble Wes! Updated commit: https://github.com/mozilla-b2g/gaia/commit/d9b9b68ac28e06bd949f2e6a444df8a68396ea11

While fixing a minor regression (bug 1190771), I noticed one thing. We replace the number in the STR and when we disconnect it displays the new number instead of the one we're calling. STR: 1) Dial 10010 2) Open dialpad 3) type "9123" 4) End call Current result: It seems like we're ending a call with "9123" Expected result: It should show "10010" as a number we've been connected to I'm NI'ing :gsvelto for when he's back from PTO, in case this is a bug.

It's a known bug and one that we should really be fixing, see bug 1134829.