Bug Tracker

Opened 12 years ago

Closed 11 years ago

Last modified 8 years ago

#2973 closed bug (fixed)

IE6 clone(true) bug when cloning a cloned element with events

Reported by: futileohm Owned by:
Priority: major Milestone: 1.3.2
Component: core Version: 1.3.1
Keywords: clone ie6 Cc:
Blocked by: Blocking:

Description

I seem to have discovered an obscure bug in IE (6, I don't have a copy of 7 to test with) when using clone(true) that occurs when you clone an element with certain nested elements and events, and then clone it again. This is a regression from 1.2.3, which seems to have worked fine.

I had a hard time generalizing this bug as it seems to only happen in very specific circumstances, but I have attached a test case to demonstrate the behavior.

As far as I can tell, the most generic way to make the bug occur is to take a DOM node structured like so:

<div class='a'>
	<span>
		<span class='b'>abc</span>
	</span>
	<span>
		<span class='c'>abc</span>
	</span>
</div>

and bind an event to span.b and another event to span.c. Then, clone div.a using clone(true), and then clone the clone using clone(true) again. Removing any of the surrounding span elements causes the bug to disappear, although swapping the class designation on the first set of nested span elements demonstrates the bug as well.

The specific error reported by IE is "'nodeType' is null or not an object" and occurs on the first line of the event.add function: if ( elem.nodeType == 3 || elem.nodeType == 8 ).

I tried replacing the event.add function in 1.2.3 with the event.add function from the trunk, and 1.2.3 still doesn't demonstrate the bug, leading me to believe it's elsewhere that the issue's occurring. I compared the clone functions from 1.2.3 and the trunk and didn't see any major changes that may have caused this sort of an issue, but at that point I was out of my league.

I'm not sure that this is a major regression, but it's a regression nonetheless, so I would imagine it should probably be fixed to maintain backwards compatibility.

Attachments (3)

clone-testcase.html (1.1 KB) - added by futileohm 12 years ago.
Test case demonstrating the bug.
prism.exe (104.0 KB) - added by futileohm 12 years ago.
Patch with potential fix.
ie6-clone-events-patch.diff (1.2 KB) - added by futileohm 12 years ago.
Actual patch with potential fix.

Download all attachments as: .zip

Change History (9)

Changed 12 years ago by futileohm

Attachment: clone-testcase.html added

Test case demonstrating the bug.

comment:1 Changed 12 years ago by flesler

You're trying this with 1.2.5 or 1.2.6 ?

comment:2 Changed 12 years ago by futileohm

It doesn't work in 1.2.5, 1.2.6, or the latest trunk (r5712). (I think I chose version 1.2.5 because it was the latest listed at the time).

I can also confirm that the same bug occurs in IE7.

Changed 12 years ago by futileohm

Attachment: prism.exe added

Patch with potential fix.

Changed 12 years ago by futileohm

Attachment: ie6-clone-events-patch.diff added

Actual patch with potential fix.

comment:3 Changed 12 years ago by futileohm

(Apologies for the prism.exe attachment, I have no clue how that got selected instead of the patch itself, please delete it.)

I tracked down the source of the bug, and the actual cause is due to the way expandos are handled and stripped within the clone function. The actual change in behavior between 1.2.3 and the current trunk is due to a unique() call within the add() function. Within the clone function, the following is used to strip expandos from the newly cloned object:

var clone = ret.find("*").andSelf().each(function(){
	if ( this[ expando ] != undefined )
		this[ expando ] = null;
});

However, the andSelf() call, which then invokes add(), barfs because of the yet-to-be-cleaned expandos on the newly cloned elements. Thus, until the expandos are cleaned, unique() will return an incorrect set of elements because the copied expandos are inconsistent.

I attached a patch (against r5713) with a fix, which breaks the expando cleaning out into two separate calls to avoid using andSelf(). There's probably a more efficient way of doing it, but I couldn't find one that wasn't incredibly tedious in my few minutes of tinkering. The new code looks like this:

ret.find('*').each(function(){
	if ( this[ expando ] != undefined )
		this[ expando ] = null;
});

ret.each(function(){
	if ( this[ expando ] != undefined )
		this[ expando ] = null;
});

var clone = ret.find('*').andSelf();

As such, it seems this may not apply only to elements with events, but any data carrying nodes that are cloned within IE could be susceptible to this bug, which could in theory remove elements from the cloned set.

comment:4 in reply to:  3 Changed 12 years ago by num

Replying to futileohm:

Applied your patch to 1.2.6 and works well in prelim testing with http://plugins.jquery.com/project/jCal plugin which suffered from this issue.

comment:5 Changed 11 years ago by dmethvin

Keywords: clone ie6 added

comment:6 Changed 11 years ago by john

Milestone: 1.3.2
Resolution: fixed
Status: newclosed
Version: 1.2.51.3.1

This should be fixed now in SVN trunk.

Note: See TracTickets for help on using tickets.