Opened 6 years ago

Last modified 5 years ago

#16957 new defect

dom-construct::empty also empties child nodes

Reported by: freddefisk Owned by: Eugene Lazutkin
Priority: undecided Milestone: 2.0
Component: HTML Version: 1.8.3
Keywords: Cc: Douglas Hays
Blocked By: Blocking:

Description

This example shows the problem. Also available here: http://dojo-sandbox.net/public/2e712/1

dom-construct::place(..., "only") also suffers from the same issue since it uses empty internaly.

HTML:

<div id="parent"></div>

JS:

require(["dojo/dom", "dojo/dom-construct", "dojo/domReady!"], function(dom, domConstruct) {
  var parent = dom.byId("parent");
  var child = domConstruct.create("div",{innerHTML: "I'm the child"});
  
  domConstruct.place(child, parent);
  
  // "I'm the child" is visible
  console.debug(parent.innerHTML); // => "<DIV>I'm the child</DIV>"
  console.debug(child.innerHTML); // => "I'm the child"
  
  domConstruct.empty(parent);
  domConstruct.place(child, parent);
  
  // "I'm the child" is NOT visible
  console.debug(parent.innerHTML); // => "<DIV></DIV>"
  console.debug(child.innerHTML); // => ""
});

I only have IE 8 so I have not tested any other versions.

Change History (10)

comment:1 Changed 6 years ago by bill

Cc: Douglas Hays added
Component: CoreHTML
Owner: set to Eugene Lazutkin

What makes you think the current behavior is incorrect? Admittedly the API doc for empty() contradicts itself: first it talks about "removing" the children but then it talks about "destroying" them:

 // summary:
 //		safely removes all children of the node.
 // node: DOMNode|String
 //		a reference to a DOM node or an id.
 // example:
 //		Destroy node's children byId:
 //	|	dojo.empty("someId");
 //
 // example:
 //		Destroy all nodes' children in a list by reference:
 //	|	dojo.query(".someNode").forEach(dojo.empty);

I always assumed the latter behavior was expected, that all children should be destroyed, since that's the way it's coded. Probably we just need to update the doc.

comment:2 Changed 6 years ago by freddefisk

The behaviour of empty() is not consistent between browsers. All other browsers I have tested will just remove the child nodes, not destroy them. What is the benifit with destroying all child nodes compared to just remove them?

It might not be relevant, but jQuery's empty does not destroy the child nodes in IE. In our project we ended up using that function instead.

comment:3 Changed 6 years ago by freddefisk

Is the destroying of child nodes done to prevent some memory leak in IE? I have not tested this after [31197], but I guess that it's innerHTML = "" that is causing this problem in IE.

Will there be a decision on this?

comment:4 Changed 6 years ago by bill

Yes, that code is to prevent memory leaks and also for speed, see [30096]. Note that prior to using innerHTML it was using another technique with the _destroyContainer DOMNode that presumably also destroyed all descendants, so the behavior on IE has presumably been this way from the beginning.

comment:5 Changed 5 years ago by bill

#17306 is a duplicate of this ticket.

comment:6 in reply to:  2 Changed 5 years ago by bill

Milestone: tbd2.0
Summary: dom-construct::empty also empties child nodes in IE8dom-construct::empty also empties child nodes

Replying to freddefisk:

The behaviour of empty() is not consistent between browsers.

Since 1.8, the behavior across browsers is consistent: all descendant nodes are orphaned, and event handlers are also removed. While this behavior is non-intuitive, at least it's now consistent.

As I wrote in the other ticket though, I assume 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 presumably the bug will be fixed (or rather, this behavior will be changed) in 2.0.

comment:7 Changed 5 years ago by bill

Milestone: 2.01.10

Actually I ended up fixing this in ffbb6d4d9718aac1e8df5e7b4d3aac6351404e2c, for 1.10.

comment:8 Changed 5 years ago by bill

Woops, actually on IE (even IE11) it still destroys the grandchildren. Because that's how node.innerHTML = "" works on IE. So to fix this we'd need to get rid of that innerHTML setting completely and simplify empty() to always do just:

for(var c; c = node.lastChild;){ // intentional assignment
	node.removeChild(c);
}

Maybe that's a good idea now that we've desupported IE6 and IE7, so memory leaks aren't as much of an issue as before?

According to http://jsperf.com/clear-dom-node the removeChild() approach is much faster. From my tests (using that page) it's 10x faster on IE8 and 100x faster on iOS7.

PS: Have to be careful about memory leaks on IE8, see #16833.

Last edited 5 years ago by bill (previous) (diff)

comment:9 Changed 5 years ago by bill

Milestone: 1.102.0

After looking at this more, it seems that fixing this ticket will likely cause significant memory leaks on IE8 for code like ContentPane? that calls domConstruct.empty() to clear its contents. See #16833 for a useful test case.

So, this seems too dangerous to fix until 2.0, when we desupport IE8. Or at least, not worth the effort of fixing in 1.10.

comment:10 Changed 5 years ago by Bill Keese <bill@…>

In 4870048a8a88133e145c89250781e64b3275b757/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.