Opened 8 years ago
Closed 8 years ago
#17306 closed defect (duplicate)
dom-construct#place with position 'only' orphans all descendants, not just direct children
Reported by: | lukevenn | Owned by: | Eugene Lazutkin |
---|---|---|---|
Priority: | undecided | Milestone: | tbd |
Component: | HTML | Version: | 1.9.1 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
If you are using widgets and want to persist their domNode when switching widgets in and out of a container then using position 'only' with the 'placeAt' method will completely destroy any nodes you are replacing in anything other than IE. This is caused by the '_empty' method in dom-construct using a test against the node having a 'canHaveChildren' property which fails in anything other than IE.
function _empty(/*DomNode*/ node){ if(node.canHaveChildren){ try{ // fast path node.innerHTML = ""; return; }catch(e){ // innerHTML is readOnly (e.g. TABLE (sub)elements in quirks mode) // Fall through (saves bytes) } } // SVG/strict elements don't support innerHTML/canHaveChildren, and OBJECT/APPLET elements in quirks node have canHaveChildren=false for(var c; c = node.lastChild;){ // intentional assignment _destroy(c, node); // destroy is better than removeChild so TABLE subelements are removed in proper order } }
A simple change to add a test for ie fixes the issue -
function _empty(/*DomNode*/ node){ if(!has("ie") || node.canHaveChildren){ try{ // fast path node.innerHTML = ""; return; }catch(e){ // innerHTML is readOnly (e.g. TABLE (sub)elements in quirks mode) // Fall through (saves bytes) } } // SVG/strict elements don't support innerHTML/canHaveChildren, and OBJECT/APPLET elements in quirks node have canHaveChildren=false for(var c; c = node.lastChild;){ // intentional assignment _destroy(c, node); // destroy is better than removeChild so TABLE subelements are removed in proper order } }
Attachments (1)
Change History (9)
Changed 8 years ago by
Attachment: | bugtest.html added |
---|
comment:1 Changed 8 years ago by
Component: | General → HTML |
---|---|
Owner: | set to Douglas Hays |
Status: | new → assigned |
Assigning to Doug since he wrote that code.
So, you are saying that domConstruct.place(node, refNode, "only) will completely destroy the old children of refNode even if you maintain a reference to them?
I'm not sure what the correct behavior is and whether or not this constitutes a bug. You could equally argue that empty() is broken because it destroys the DOMNodes instead of just orphaning them.
Also, even with lukevenn's suggested patch, won't SVG elements still be completely destroyed?
comment:2 Changed 8 years ago by
It doesn't destroy the children (apologies for the confusing title) but detaches them and then detaches their children which means that although maintaining a reference to the node will persist it there will be no child nodes. If you look in the console while running the test you will see it log the node that is persisted for 'widget1' and that it is empty.
It looks like it is actually related to the following in '_destroy':
if(node.firstChild){ _empty(node); }
When '_destroy' is called from '_empty' it passes in the child nodes one at a time so will lead to a recursive detachment of all children (at least that's the way it appears from reading the code). FYI to test this I tried commenting out the above line and the problem did not occur so I assume it prevented the recursion.
comment:3 Changed 8 years ago by
That makes sense, so the replaced DOM trees are blown apart into their individual nodes, all detached from each other. And analogous problem with empty.
comment:4 Changed 8 years ago by
The previous implementation would destroy children in IE and orphan them in other browsers so I consolidated them to always destroy for consistency and to avoid memory leaks. The doc for empty could be less ambiguous.
comment:5 Changed 8 years ago by
Owner: | Douglas Hays deleted |
---|---|
Status: | assigned → open |
comment:6 Changed 8 years ago by
Owner: | set to Eugene Lazutkin |
---|---|
Status: | open → assigned |
Summary: | [Regression]Using dom-construct place with a position 'only' destroys all child nodes of any nodes it replaces → dom-construct#place with position 'only' orphans all descendants, not just direct children |
Since the behavior of empty(node) and place(node, refNode, "only") was never well documented, and since their behavior on IE hasn't changed, I don't think of this ticket as a bug.
I assume though that in 2.0, since we won't support browsers with memory leaks, that we'll change empty() and place() to just orphan the direct children, and not do anything recursively. So we could change this to a task to be done in 2.0.
comment:8 Changed 8 years ago by
Resolution: | → duplicate |
---|---|
Status: | assigned → closed |
Duplicate of #16957.
Oh you are right, OK I'll close this as a duplicate.
Test case