1053629 - Reduce complexity of setTextContent

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect, P2)

Description

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

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

Comment 1

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

Attachment: pull request

Comment 2

[Staś Małolepszy :stas]

(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?

Comment 3

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

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

Comment 4

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

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.

Comment 5

[Staś Małolepszy :stas]

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.

Comment 6

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

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.

Comment 7

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

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.

Comment 8

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

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.

Comment 9

[Staś Małolepszy :stas]

(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).

Comment 10

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

Attachment: patch

Comment 11

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

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.

Comment 12

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

Attachment: patch

updated patch

Comment 13

[Staś Małolepszy :stas]

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!

Comment 14

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

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! :)

Comment 15

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

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