From 49cd9c66bf6097ced133971a82fa9bd7b2f73e1a Mon Sep 17 00:00:00 2001 From: rwldrn Date: Thu, 20 Jan 2011 14:25:56 -0500 Subject: [PATCH 1/3] cloneCopyEvent; jQuery.clone() review --- src/manipulation.js | 134 +++++++++++++++++++++++--------------------- 1 file changed, 71 insertions(+), 63 deletions(-) diff --git a/src/manipulation.js b/src/manipulation.js index 596a45736..af71ff62c 100644 --- a/src/manipulation.js +++ b/src/manipulation.js @@ -183,43 +183,10 @@ jQuery.fn.extend({ return this; }, - clone: function( events ) { - // Do the clone - var ret = this.map(function() { - var clone = this.cloneNode(true); - if ( !jQuery.support.noCloneEvent && (this.nodeType === 1 || this.nodeType === 11) && !jQuery.isXMLDoc(this) ) { - // IE copies events bound via attachEvent when using cloneNode. - // Calling detachEvent on the clone will also remove the events - // from the original. In order to get around this, we use some - // proprietary methods to clear the events. Thanks to MooTools - // guys for this hotness. - - // Using Sizzle here is crazy slow, so we use getElementsByTagName - // instead - var srcElements = this.getElementsByTagName("*"), - destElements = clone.getElementsByTagName("*"); - - // Weird iteration because IE will replace the length property - // with an element if you are cloning the body and one of the - // elements on the page has a name or id of "length" - for ( var i = 0; srcElements[i]; ++i ) { - cloneFixAttributes( srcElements[i], destElements[i] ); - } - - cloneFixAttributes( this, clone ); - } - - return clone; + clone: function( events, deepData ) { + return this.map( function () { + return jQuery.clone( this, events == null ? true : events, deepData == null ? true : deepData ); }); - - // Copy the events from the original to the clone - if ( events === true ) { - cloneCopyEvent( this, ret ); - cloneCopyEvent( this.find("*"), ret.find("*") ); - } - - // Return the cloned set - return ret; }, html: function( value ) { @@ -347,7 +314,7 @@ jQuery.fn.extend({ root(this[i], first) : this[i], i > 0 || results.cacheable || (this.length > 1 && i > 0) ? - jQuery(fragment).clone(true)[0] : + jQuery.clone( fragment, true, true ) : fragment ); } @@ -369,40 +336,33 @@ function root( elem, cur ) { elem; } -function cloneCopyEvent(orig, ret) { - ret.each(function (nodeIndex) { - if ( this.nodeType !== 1 || !jQuery.hasData(orig[nodeIndex]) ) { - return; - } +function cloneCopyEvent( src, dest ) { - // XXX remove for 1.5 RC or merge back in if there is actually a reason for this check that has been - // unexposed by unit tests - if ( this.nodeName !== (orig[nodeIndex] && orig[nodeIndex].nodeName) ) { - throw "Cloned data mismatch"; - } + if ( dest.nodeType !== 1 || !jQuery.hasData( src ) ) { + return; + } - var internalKey = jQuery.expando, - oldData = jQuery.data( orig[nodeIndex] ), - curData = jQuery.data( this, oldData ); + var internalKey = jQuery.expando, + oldData = jQuery.data( src ), + curData = jQuery.data( dest, oldData ); - // Switch to use the internal data object, if it exists, for the next - // stage of data copying - if ( (oldData = oldData[ internalKey ]) ) { - var events = oldData.events; - curData = curData[ internalKey ] = jQuery.extend({}, oldData); + // Switch to use the internal data object, if it exists, for the next + // stage of data copying + if ( (oldData = oldData[ internalKey ]) ) { + var events = oldData.events; + curData = curData[ internalKey ] = jQuery.extend({}, oldData); - if ( events ) { - delete curData.handle; - curData.events = {}; + if ( events ) { + delete curData.handle; + curData.events = {}; - for ( var type in events ) { - for ( var i = 0, l = events[ type ].length; i < l; i++ ) { - jQuery.event.add( this, type, events[ type ][ i ], events[ type ][ i ].data ); - } + for ( var type in events ) { + for ( var i = 0, l = events[ type ].length; i < l; i++ ) { + jQuery.event.add( dest, type, events[ type ][ i ], events[ type ][ i ].data ); } } } - }); + } } function cloneFixAttributes(src, dest) { @@ -520,6 +480,54 @@ jQuery.each({ }); jQuery.extend({ + clone: function( elem, dataAndEvents, deepCloneDataAndEvents ) { + + var clone = elem.cloneNode(true), + srcElements, + destElements; + + if ( !jQuery.support.noCloneEvent && ( elem.nodeType === 1 || elem.nodeType === 11) && !jQuery.isXMLDoc(elem) ) { + // IE copies events bound via attachEvent when using cloneNode. + // Calling detachEvent on the clone will also remove the events + // from the original. In order to get around this, we use some + // proprietary methods to clear the events. Thanks to MooTools + // guys for this hotness. + + // Using Sizzle here is crazy slow, so we use getElementsByTagName + // instead + srcElements = elem.getElementsByTagName("*"); + destElements = clone.getElementsByTagName("*"); + + // Weird iteration because IE will replace the length property + // with an element if you are cloning the body and one of the + // elements on the page has a name or id of "length" + for ( var i = 0; srcElements[i]; ++i ) { + cloneFixAttributes( srcElements[i], destElements[i] ); + } + + cloneFixAttributes( elem, clone ); + } + + // Copy the events from the original to the clone + if ( dataAndEvents === true ) { + + cloneCopyEvent( elem , clone ); + + if ( deepCloneDataAndEvents === true && "getElementsByTagName" in elem ) { + + srcElements = elem.getElementsByTagName("*"); + destElements = clone.getElementsByTagName("*"); + + if ( srcElements.length ) { + for ( var i = 0; i < srcElements.length; ++i ) { + cloneCopyEvent( srcElements[i], destElements[i] ); + } + } + } + } + // Return the cloned set + return clone; + }, clean: function( elems, context, fragment, scripts ) { context = context || document; From 6458885881e7badb99c70c24843c31d13064a167 Mon Sep 17 00:00:00 2001 From: rwldrn Date: Thu, 20 Jan 2011 15:25:04 -0500 Subject: [PATCH 2/3] Cleaned up; fixes per review --- src/core.js | 2 +- src/manipulation.js | 20 +++++++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/core.js b/src/core.js index f116ef4d1..9af2882d7 100644 --- a/src/core.js +++ b/src/core.js @@ -130,7 +130,7 @@ jQuery.fn = jQuery.prototype = { } else { ret = jQuery.buildFragment( [ match[1] ], [ doc ] ); - selector = (ret.cacheable ? jQuery(ret.fragment).clone()[0] : ret.fragment).childNodes; + selector = (ret.cacheable ? jQuery.clone(ret.fragment) : ret.fragment).childNodes; } return jQuery.merge( this, selector ); diff --git a/src/manipulation.js b/src/manipulation.js index af71ff62c..d758d803f 100644 --- a/src/manipulation.js +++ b/src/manipulation.js @@ -183,9 +183,12 @@ jQuery.fn.extend({ return this; }, - clone: function( events, deepData ) { + clone: function( dataAndEvents, deepDataAndEvents ) { + dataAndEvents = dataAndEvents == null ? true : dataAndEvents; + deepDataAndEvents = deepDataAndEvents == null ? dataAndEvents : deepDataAndEvents; + return this.map( function () { - return jQuery.clone( this, events == null ? true : events, deepData == null ? true : deepData ); + return jQuery.clone( this, dataAndEvents, deepDataAndEvents ); }); }, @@ -480,13 +483,12 @@ jQuery.each({ }); jQuery.extend({ - clone: function( elem, dataAndEvents, deepCloneDataAndEvents ) { - + clone: function( elem, dataAndEvents, deepDataAndEvents ) { var clone = elem.cloneNode(true), srcElements, destElements; - if ( !jQuery.support.noCloneEvent && ( elem.nodeType === 1 || elem.nodeType === 11) && !jQuery.isXMLDoc(elem) ) { + if ( !jQuery.support.noCloneEvent && (elem.nodeType === 1 || elem.nodeType === 11) && !jQuery.isXMLDoc(elem) ) { // IE copies events bound via attachEvent when using cloneNode. // Calling detachEvent on the clone will also remove the events // from the original. In order to get around this, we use some @@ -509,17 +511,17 @@ jQuery.extend({ } // Copy the events from the original to the clone - if ( dataAndEvents === true ) { + if ( dataAndEvents ) { - cloneCopyEvent( elem , clone ); + cloneCopyEvent( elem, clone ); - if ( deepCloneDataAndEvents === true && "getElementsByTagName" in elem ) { + if ( deepDataAndEvents && "getElementsByTagName" in elem ) { srcElements = elem.getElementsByTagName("*"); destElements = clone.getElementsByTagName("*"); if ( srcElements.length ) { - for ( var i = 0; i < srcElements.length; ++i ) { + for ( var i = 0; srcElements[i]; ++i ) { cloneCopyEvent( srcElements[i], destElements[i] ); } } From 33a67ffa9d85450f1edf1b1645b57f7641e2e2d2 Mon Sep 17 00:00:00 2001 From: rwldrn Date: Fri, 21 Jan 2011 11:08:15 -0500 Subject: [PATCH 3/3] Basic unit tests; This patch relies on the 51 existing clone() tests --- test/unit/manipulation.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/unit/manipulation.js b/test/unit/manipulation.js index 37234d86d..a68c214ba 100644 --- a/test/unit/manipulation.js +++ b/test/unit/manipulation.js @@ -871,6 +871,18 @@ test("replaceAll(String|Element|Array<Element>|jQuery)", function() { ok( !jQuery("#yahoo")[0], 'Verify that original element is gone, after set of elements' ); }); +test("jQuery.clone() (#8017)", function() { + + expect(2); + + ok( jQuery.clone && jQuery.isFunction( jQuery.clone ) , "jQuery.clone() utility exists and is a function."); + + var main = jQuery("#main")[0], + clone = jQuery.clone( main ); + + equals( main.children.length, clone.children.length, "Simple child length to ensure a large dom tree copies correctly" ); +}); + test("clone()", function() { expect(37); equals( 'This is a normal link: Yahoo', jQuery('#en').text(), 'Assert text for #en' );