The current algorithm that we use to set text content is overly complex. [1] I don't think we use it anywhere, and the sole fact that it checks firstElementChild is probably costing us enough to justify its removal. Ultimately, we're looking to introduce the whole DOM overlay mechanism (bug 994357), but as a first step, we could reduce complexity of setTextContent to ensure there are no regressions. [1] https://github.com/mozilla-b2g/gaia/blob/5e074831f9ddacdf6f622a6dffaecb626f740be8/shared/js/l10n.js#L1720-L1747

(In reply to Zibi Braniecki [:gandalf] from comment #0) > Ultimately, we're looking to introduce the whole DOM overlay mechanism (bug > 994357), but as a first step, we could reduce complexity of setTextContent > to ensure there are no regressions. Great approach, let's try to simplify this now and it will make landing bug 994357 much easier. It looks like the TBPL build is red. The Gu failure is related to your change, but I'm not sure about Gij. Can you investigate and fix the tests if needed?

The list of Gij errors that blow when throw new Error added: https://pastebin.mozilla.org/6302483

Actually, the list above is wrong. Most of those tests fail on my machine even on master. I fixed errors reported by Gij on tbpl and pushed new PR. I'm testing the build on my phone that logs any attempt to localize an element which has nodes inside and throws. So far didn't find any example using my branch, but would like to test more. Stas, one idea would be to land it with this console/throw combo so that if people find it somewhere, they can easily debug it. Close to FL we could remove the throw and logging and simplify the method.

I've had mixed experiences with running the whole Gij suite locally; usually ending in many failures which I didn't see on TBPL at the time. I'd like to try an alternative approach. At the time of this writing the latest master commit with a completely green TBPL is de59e0c: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=0672ce1bc5b5 I've created a pull request based on this commit in which I thrown an error in setTextContent: https://github.com/mozilla-b2g/gaia/pull/23709 This should hopefully give us a good list of tests which fail, which in turn should lead us to code which should be changed. Unfortunately, I don't think we'll be able to see console.log nor even the L10nError message, as most of the Gij failures are usually client.waitFor's timeouts.

I have now green builds and the patch contains all fixes to cases which will be altered by this change except of what's in bug 1059087 (which should land soon). I'm still not sure how to test this beyond what we have. I'll probably wait for bug 1059087 to get fixed and message dev.gaia asking for an opinion.

I created a simple DOMWalker script to look for nodes with data-l10n-id that have childNodes. Found 52 cases: https://pastebin.mozilla.org/6388495 Will file a separate bug for cleaning up and write patches per app.

The good news is that vast majority of the cases found by the script are l10nIds that only alter node attributes (in all cases - ariaLabel), so they never trigger setTextContent and thus will not be affected by this patch. It is still debatable wherever we should allow for such cases to work, but it is out of the scope of this bug. I filed bug 1064561 and will fix all cases where the l10n is expected to translate the content of the node.

(In reply to Zibi Braniecki [:gandalf] from comment #8) > The good news is that vast majority of the cases found by the script are > l10nIds that only alter node attributes (in all cases - ariaLabel), so they > never trigger setTextContent and thus will not be affected by this patch. Cool. I'm looking forward to L20n syntax which makes a distinction between <foo attr: "Attr"> (no value) and <foo "" attr: "Attr"> (an empty value).

Stas, I believe we should land it. I'm pretty sure that there are no HTML files in gaia that would assign data-l10n-id to DOMFragment, and we introduced the setAttribute/setAttributes quite recently and I believe most of the code that set data-l10n-id from JS has been going through one of the l10n-drivers. I'm using a build with this change for a few days now and didn't spot any regressions.

updated patch

Comment on attachment 8496059 [details] [diff] [review] patch Review of attachment 8496059 [details] [diff] [review]: ----------------------------------------------------------------- r=me, thank you for applying my comments from the IRC convo!

Commit: https://github.com/zbraniecki/gaia/commit/88da39a2b698dc2458867de2173a409e2d8987f3 Merge: https://github.com/mozilla-b2g/gaia/commit/c4657a4a69b937cc289e7b035a9499a1772cb344 Whoa! Landed. Thanks a lot for reviews and feedback Stas! :)

L20n: https://github.com/l20n/l20n.js/commit/df786d4ca772401ca062a1672fda93b183e6731c