Closed Bug 1053629 Opened 10 years ago Closed 10 years ago

Reduce complexity of setTextContent

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(2 files, 1 obsolete file)

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
Attached file pull request (deleted) —
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(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?
Priority: -- → P2
Depends on: 1062602
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.
Depends on: 1059087
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.
Depends on: 1064561
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).
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #8493436 - Flags: review?(stas)
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.
Attached patch patch (deleted) — Splinter Review
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+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: