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)
L20n
JS Library
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 921169
People
(Reporter: mgol, Unassigned)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Reporter | ||
Comment 1•11 years ago
|
||
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.
Reporter | ||
Comment 2•11 years ago
|
||
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
Reporter | ||
Comment 3•11 years ago
|
||
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?
Reporter | ||
Updated•11 years ago
|
Attachment #795422 -
Flags: review?(stas)
Reporter | ||
Comment 4•11 years ago
|
||
Attachment #795422 -
Attachment is obsolete: true
Attachment #795422 -
Flags: review?(stas)
Reporter | ||
Updated•11 years ago
|
Attachment #802452 -
Flags: review?(stas)
Reporter | ||
Comment 5•11 years ago
|
||
Attachment #802452 -
Attachment is obsolete: true
Attachment #802452 -
Flags: review?(stas)
Comment 6•11 years ago
|
||
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.
Description
•