dom-construct#place with position 'only' orphans all descendants, not just direct children

[lukevenn]

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
		}
	}

[lukevenn]

Test case

[bill]

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?

[lukevenn]

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.

[bill]

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.

[Douglas Hays]

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.

[bill]

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.

[freddefisk]

Related to #16957?

[bill]

Duplicate of #16957.
Oh you are right, OK I'll close this as a duplicate.