Permalink
Browse files

Manipulation: privatize internal domManip() function

Fixes gh-2225
  • Loading branch information...
timmywil committed May 5, 2015
1 parent a74320f commit 62d5579578109f1468a37e44f76af06f283a46ab
Showing with 93 additions and 94 deletions.
  1. +93 −94 src/manipulation.js
View
@@ -108,6 +108,94 @@ function fixInput( src, dest ) {
}
}
function domManip( collection, args, callback, ignored ) {
// Flatten any nested arrays
args = concat.apply( [], args );
var fragment, first, scripts, hasScripts, node, doc,
i = 0,
l = collection.length,
iNoClone = l - 1,
value = args[ 0 ],
isFunction = jQuery.isFunction( value );
// We can't cloneNode fragments that contain checked, in WebKit
if ( isFunction ||
( l > 1 && typeof value === "string" &&
!support.checkClone && rchecked.test( value ) ) ) {
return collection.each(function( index ) {
var self = collection.eq( index );
if ( isFunction ) {
args[ 0 ] = value.call( this, index, self.html() );
}
domManip( self, args, callback, ignored );
});
}
if ( l ) {
fragment = buildFragment( args, collection[ 0 ].ownerDocument, false, collection, ignored );
first = fragment.firstChild;
if ( fragment.childNodes.length === 1 ) {
fragment = first;
}
// Require either new content or an interest in ignored elements to invoke the callback
if ( first || ignored ) {
scripts = jQuery.map( getAll( fragment, "script" ), disableScript );
hasScripts = scripts.length;
// Use the original fragment for the last item
// instead of the first because it can end up
// being emptied incorrectly in certain situations (#8070).
for ( ; i < l; i++ ) {
node = fragment;
if ( i !== iNoClone ) {
node = jQuery.clone( node, true, true );
// Keep references to cloned scripts for later restoration
if ( hasScripts ) {
// Support: Android<4.1, PhantomJS<2
// push.apply(_, arraylike) throws on ancient WebKit
jQuery.merge( scripts, getAll( node, "script" ) );
}
}
callback.call( collection[ i ], node, i );
}
if ( hasScripts ) {
doc = scripts[ scripts.length - 1 ].ownerDocument;
// Reenable scripts
jQuery.map( scripts, restoreScript );
// Evaluate executable scripts on first document insertion
for ( i = 0; i < hasScripts; i++ ) {
node = scripts[ i ];
if ( rscriptType.test( node.type || "" ) &&
!dataPriv.access( node, "globalEval" ) &&
jQuery.contains( doc, node ) ) {
if ( node.src ) {
// Optional AJAX dependency, but won't run scripts if not present
if ( jQuery._evalUrl ) {
jQuery._evalUrl( node.src );
}
} else {
jQuery.globalEval( node.textContent.replace( rcleanScript, "" ) );
}
}
}
}
}
}
return collection;
}
jQuery.extend({
htmlPrefilter: function( html ) {
return html.replace( rxhtmlTag, "<$1></$2>" );
@@ -193,7 +281,7 @@ jQuery.fn.extend({
},
append: function() {
return this.domManip( arguments, function( elem ) {
return domManip( this, arguments, function( elem ) {
if ( this.nodeType === 1 || this.nodeType === 11 || this.nodeType === 9 ) {
var target = manipulationTarget( this, elem );
target.appendChild( elem );
@@ -202,7 +290,7 @@ jQuery.fn.extend({
},
prepend: function() {
return this.domManip( arguments, function( elem ) {
return domManip( this, arguments, function( elem ) {
if ( this.nodeType === 1 || this.nodeType === 11 || this.nodeType === 9 ) {
var target = manipulationTarget( this, elem );
target.insertBefore( elem, target.firstChild );
@@ -211,15 +299,15 @@ jQuery.fn.extend({
},
before: function() {
return this.domManip( arguments, function( elem ) {
return domManip( this, arguments, function( elem ) {
if ( this.parentNode ) {
this.parentNode.insertBefore( elem, this );
}
});
},
after: function() {
return this.domManip( arguments, function( elem ) {
return domManip( this, arguments, function( elem ) {
if ( this.parentNode ) {
this.parentNode.insertBefore( elem, this.nextSibling );
}
@@ -317,7 +405,7 @@ jQuery.fn.extend({
var ignored = [];
// Make the changes, replacing each non-ignored context element with the new content
return this.domManip( arguments, function( elem ) {
return domManip( this, arguments, function( elem ) {
var parent = this.parentNode;
if ( jQuery.inArray( this, ignored ) < 0 ) {
@@ -333,95 +421,6 @@ jQuery.fn.extend({
detach: function( selector ) {
return this.remove( selector, true );
},
domManip: function( args, callback, ignored ) {
// Flatten any nested arrays
args = concat.apply( [], args );
var fragment, first, scripts, hasScripts, node, doc,
i = 0,
l = this.length,
set = this,
iNoClone = l - 1,
value = args[ 0 ],
isFunction = jQuery.isFunction( value );
// We can't cloneNode fragments that contain checked, in WebKit
if ( isFunction ||
( l > 1 && typeof value === "string" &&
!support.checkClone && rchecked.test( value ) ) ) {
return this.each(function( index ) {
var self = set.eq( index );
if ( isFunction ) {
args[ 0 ] = value.call( this, index, self.html() );
}
self.domManip( args, callback, ignored );
});
}
if ( l ) {
fragment = buildFragment( args, this[ 0 ].ownerDocument, false, this, ignored );
first = fragment.firstChild;
if ( fragment.childNodes.length === 1 ) {
fragment = first;
}
// Require either new content or an interest in ignored elements to invoke the callback
if ( first || ignored ) {
scripts = jQuery.map( getAll( fragment, "script" ), disableScript );
hasScripts = scripts.length;
// Use the original fragment for the last item
// instead of the first because it can end up
// being emptied incorrectly in certain situations (#8070).
for ( ; i < l; i++ ) {
node = fragment;
if ( i !== iNoClone ) {
node = jQuery.clone( node, true, true );
// Keep references to cloned scripts for later restoration
if ( hasScripts ) {
// Support: Android<4.1, PhantomJS<2
// push.apply(_, arraylike) throws on ancient WebKit
jQuery.merge( scripts, getAll( node, "script" ) );
}
}
callback.call( this[ i ], node, i );
}
if ( hasScripts ) {
doc = scripts[ scripts.length - 1 ].ownerDocument;
// Reenable scripts
jQuery.map( scripts, restoreScript );
// Evaluate executable scripts on first document insertion
for ( i = 0; i < hasScripts; i++ ) {
node = scripts[ i ];
if ( rscriptType.test( node.type || "" ) &&
!dataPriv.access( node, "globalEval" ) &&
jQuery.contains( doc, node ) ) {
if ( node.src ) {
// Optional AJAX dependency, but won't run scripts if not present
if ( jQuery._evalUrl ) {
jQuery._evalUrl( node.src );
}
} else {
jQuery.globalEval( node.textContent.replace( rcleanScript, "" ) );
}
}
}
}
}
}
return this;
}
});

8 comments on commit 62d5579

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 5, 2015

Member

I'd say changes like that should go through pr first

Member

markelog replied May 5, 2015

I'd say changes like that should go through pr first

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 5, 2015

Member

I'd say changes like that should go throw pr first

I strongly agree. We have no ability to evaluate style, size, etc. when changes just land out of the blue.

Member

gibson042 replied May 5, 2015

I'd say changes like that should go throw pr first

I strongly agree. We have no ability to evaluate style, size, etc. when changes just land out of the blue.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 5, 2015

Member

This function is quite large, couldn't it be moved to a separate module?

Member

mgol replied May 5, 2015

This function is quite large, couldn't it be moved to a separate module?

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 5, 2015

Member

I'd say changes like that should go throw pr first

I'm happy to do this, but mistakenly thought it was simple enough to commit.

We have no ability to evaluate style, size, etc. when changes just land out of the blue.

That said, I think this is an overstatement. I agree the forum is not as convenient, but that can still be done after the fact. Also, this reduced size.

Member

timmywil replied May 5, 2015

I'd say changes like that should go throw pr first

I'm happy to do this, but mistakenly thought it was simple enough to commit.

We have no ability to evaluate style, size, etc. when changes just land out of the blue.

That said, I think this is an overstatement. I agree the forum is not as convenient, but that can still be done after the fact. Also, this reduced size.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 5, 2015

Member

This function is quite large, couldn't it be moved to a separate module?

Historically, we have pretty much only separated functions into their own modules if they are needed in more than one file, but I am not opposed to this.

Member

timmywil replied May 5, 2015

This function is quite large, couldn't it be moved to a separate module?

Historically, we have pretty much only separated functions into their own modules if they are needed in more than one file, but I am not opposed to this.

@PhoenixFnX

This comment has been minimized.

Show comment
Hide comment
@PhoenixFnX

PhoenixFnX Dec 2, 2016

@timmywil Can you explain what this means ?

line 179 : !dataPriv.access(node, "globalEval" )

Having multiple <script> in a server response, this makes only the first one to be executed.

PhoenixFnX replied Dec 2, 2016

@timmywil Can you explain what this means ?

line 179 : !dataPriv.access(node, "globalEval" )

Having multiple <script> in a server response, this makes only the first one to be executed.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 2, 2016

Member

That line checks for the presence of the "globalEval" property in private data. Basically, this is used to determine if the script has already been executed by jQuery. Users shouldn't need to worry about this line. If you think there's an issue, feel free to file an issue with a test case.

Member

timmywil replied Dec 2, 2016

That line checks for the presence of the "globalEval" property in private data. Basically, this is used to determine if the script has already been executed by jQuery. Users shouldn't need to worry about this line. If you think there's an issue, feel free to file an issue with a test case.

@PhoenixFnX

This comment has been minimized.

Show comment
Hide comment
@PhoenixFnX

PhoenixFnX Dec 2, 2016

Yes @timmywil , I think there is a problem or I don't understand.
I'm using jquery and jqueryui and a post request that appends a result in a dom element.
I didn't manage to get it simpler

<script type="application/javascript">
	$(function () {
		$.post("test2", function (data) {
			$(data).appendTo($("#container"));
		});
	})
</script>
<div id="container"></div>

When the post requests returns :

<div id="the_dialog" style="display: none;">
	<script type="text/javascript">
		$(function () {
			$('div#the_dialog.ui-dialog-content').dialog("destroy").remove();
			$("div#the_dialog").dialog();
		});
		alert(1);
	</script>
	<script type="application/javascript">
		alert(2);
	</script>
	Hello
</div>

There is no alert(2).

When I debug after the alert(1), I'm having this screenshot:
image

You can see there are 2 scripts but the second one gets dataPriv.access( node, "globalEval" ) == true

PhoenixFnX replied Dec 2, 2016

Yes @timmywil , I think there is a problem or I don't understand.
I'm using jquery and jqueryui and a post request that appends a result in a dom element.
I didn't manage to get it simpler

<script type="application/javascript">
	$(function () {
		$.post("test2", function (data) {
			$(data).appendTo($("#container"));
		});
	})
</script>
<div id="container"></div>

When the post requests returns :

<div id="the_dialog" style="display: none;">
	<script type="text/javascript">
		$(function () {
			$('div#the_dialog.ui-dialog-content').dialog("destroy").remove();
			$("div#the_dialog").dialog();
		});
		alert(1);
	</script>
	<script type="application/javascript">
		alert(2);
	</script>
	Hello
</div>

There is no alert(2).

When I debug after the alert(1), I'm having this screenshot:
image

You can see there are 2 scripts but the second one gets dataPriv.access( node, "globalEval" ) == true

Please sign in to comment.