Closed Bug 907851 Opened 11 years ago Closed 11 years ago

Replace contents of overlayed nodes instead of the full nodes

Categories

(L20n :: JS Library, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 921169

People

(Reporter: mgol, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

When using L20n DOM overlays, the source node gets replaced using: node.parentNode.replaceChild(l10nNode, node); This can cause problems for some frameworks expecting to keep handle of their respective nodes, this can break bound event handlers etc., especially in a scenario where L20n document.l10n.localizeNode is fired like in my ng-l20n module: https://github.com/EE/ng-l20n This problem doesn't exist when not using DOM overlays as the node is not then completely replaced. A simple fix would be to replace the line with the following one: node.innerHTML = l10nNode.innerHTML; Obviously, this has the drawback of forcing the browser to convert the node to a string value and re-parse it on innerHTML substitution. A better approach would be to use a DocumentFragmet instead of a Node but it won't work for an arbitrary entity.value returned by L20n. Thoughts?
OS: Mac OS X → All
Hardware: x86 → All
On the second thought, DocumentFragment won't work since elements can have text contents together with sub-nodes. So we're left with the innerHTML approach unless I'm missing something.
OTH we don't have to clone the whole source node if we just replace innerHTML which saves the browser some parsing so it might not be so bad after all. Pull request: https://github.com/l20n/l20n.js/pull/21
I'd love to add some tests for that but I'd need localizeNode available which is on document.l10n and I don't have access to document in Mocha tests. How could I workaround this issue?
Attachment #795422 - Flags: review?(stas)
Attached patch The patch rebased to the latest master (obsolete) (deleted) — Splinter Review
Attachment #795422 - Attachment is obsolete: true
Attachment #795422 - Flags: review?(stas)
Attachment #802452 - Flags: review?(stas)
Attached patch Patch after a hotfix (deleted) — Splinter Review
Attachment #802452 - Attachment is obsolete: true
Attachment #802452 - Flags: review?(stas)
I want to fix this as part of bug 921169.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: