Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#17869 closed defect (fixed)

function empty() in dom-construct.js doens't work reasonable in non-IE browser

Reported by: RickyShi Owned by: Eugene Lazutkin
Priority: undecided Milestone: 1.10
Component: HTML Version: 1.9.3
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

I saw something updated in Fixes #16975. Remove leak when calling empty() on SVG elements. https://github.com/dojo/dojo/commit/10ede66aa5d774604b789a6288c68241734990ca

e.g.

function _empty(/*DomNode?*/ node){

if(node.canHaveChildren){

try{

The node.canHaveChildren is always null in non-ie browsers. So all the process will path to _destroy:

	function _destroy(/*DomNode*/ node, /*DomNode*/ parent){
		// in IE quirks, node.canHaveChildren can be false but firstChild can be non-null (OBJECT/APPLET)
		if(node.firstChild){
			_empty(node);
		}
		if(parent){
			// removeNode(false) doesn't leak in IE 6+, but removeChild() and removeNode(true) are known to leak under IE 8- while 9+ is TBD.
			// In IE quirks mode, PARAM nodes as children of OBJECT/APPLET nodes have a removeNode method that does nothing and
			// the parent node has canHaveChildren=false even though removeChild correctly removes the PARAM children.
			// In IE, SVG/strict nodes don't have a removeNode method nor a canHaveChildren boolean.
			has("ie") && parent.canHaveChildren && "removeNode" in node ? node.removeNode(false) : '''parent.removeChild(node)''';
		}
	}

Because of the wrong logic in call stack the removeChild(node) will be always invoked no matter the browser is IE or Firefox. That's the reason why dojo.empty still works.

But in some cases it will take issue.

If I have a node with some child dijit widget, in elder versions, it do innerHtml = "", when I run empty on this node, the widget.domNode will still kept as a detached dom node. While in 1.9.3, if we use removeChild here, the domNode in all child widget will be cleaned.

On my opinion, the empty should just clean the dom tree, should not to clean the linked widget's properties. I think it should be rollback.

Thanks

Change History (5)

comment:1 Changed 6 years ago by bill

Component: CoreHTML
Description: modified (diff)
Owner: set to Eugene Lazutkin

The node.canHaveChildren is always null in non-ie browsers.

I agree that's a weird check, perhaps it should say this instead?

if("innerHTML" in node)

Actually the if() condition is unnecessary -- we can always run the code in the try/catch -- but it prevents spurious throws that are annoying when using the debugger.


If I have a node with some child dijit widget, in elder versions, it do innerHtml = "", when I run empty on this node, the widget.domNode will still kept as a detached dom node. While in 1.9.3, if we use removeChild here, the domNode in all child widget will be cleaned.

I guess so... actually the fallback for() loop seems strange, I see it has a comment that:

// destroy is better than removeChild so TABLE subelements are removed in proper order

...but I don't get it. Seems like removeChild() is fine. So probably this makes sense:

function _empty(/*DomNode*/ node){
	if("innerHTML" in node){
		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
	for(var c; c = node.lastChild;){ // intentional assignment
		node.removeChild(c);
	}
}

Comments?

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

comment:2 Changed 6 years ago by bill

I sent a PR for this in https://github.com/dojo/dojo/pull/79.

comment:3 Changed 6 years ago by Bill Keese <bill@…>

Resolution: fixed
Status: newclosed

In ffbb6d4d9718aac1e8df5e7b4d3aac6351404e2c/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:4 Changed 6 years ago by freddefisk

Will this change solve #16957?

comment:5 Changed 6 years ago by bill

Milestone: tbd1.10

Ah, thanks, you are right, I'll close that ticket.

Edit: Oops, not quite, but anyway I'll add comments to that ticket, and maybe work on it for 1.10.

Last edited 6 years ago by bill (previous) (diff)
Note: See TracTickets for help on using tickets.