Opened 6 years ago

Closed 6 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)

bugtest.html (1.2 KB) - added by lukevenn 6 years ago.
Test case

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by lukevenn

Attachment: bugtest.html added

Test case

comment:1 Changed 6 years ago by bill

Component: GeneralHTML
Owner: set to Douglas Hays
Status: newassigned

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 6 years ago by 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.

Last edited 6 years ago by lukevenn (previous) (diff)

comment:3 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by 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.

comment:5 Changed 6 years ago by Douglas Hays

Owner: Douglas Hays deleted
Status: assignedopen

comment:6 Changed 6 years ago by bill

Owner: set to Eugene Lazutkin
Status: openassigned
Summary: [Regression]Using dom-construct place with a position 'only' destroys all child nodes of any nodes it replacesdom-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:7 Changed 6 years ago by freddefisk

Related to #16957?

comment:8 Changed 6 years ago by bill

Resolution: duplicate
Status: assignedclosed

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

Note: See TracTickets for help on using tickets.