Reduce complexity of setTextContent

RESOLVED FIXED

Status

Firefox OS
Gaia::L10n
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
Created attachment 8472790 [details] [review]
pull request
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
See Also: → bug 994357
(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?
(Assignee)

Updated

3 years ago
Priority: -- → P2
(Assignee)

Updated

3 years ago
Depends on: 1062602
(Assignee)

Comment 3

3 years ago
The list of Gij errors that blow when throw new Error added: https://pastebin.mozilla.org/6302483
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1059087
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1064561
(Assignee)

Comment 8

3 years ago
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).
(Assignee)

Comment 10

3 years ago
Created attachment 8493436 [details] [diff] [review]
patch
Attachment #8493436 - Flags: review?(stas)
(Assignee)

Comment 11

3 years ago
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.
(Assignee)

Comment 12

3 years ago
Created attachment 8496059 [details] [diff] [review]
patch

updated patch
Attachment #8493436 - Attachment is obsolete: true
Attachment #8493436 - Flags: review?(stas)
Attachment #8496059 - Flags: review?(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!
Attachment #8496059 - Flags: review?(stas) → review+
(Assignee)

Comment 14

3 years ago
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! :)
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

3 years ago
L20n: https://github.com/l20n/l20n.js/commit/df786d4ca772401ca062a1672fda93b183e6731c
You need to log in before you can comment on or make changes to this bug.