Bug Tracker

Opened 13 years ago

Closed 12 years ago

Last modified 8 years ago

#1698 closed bug (fixed)

Scripts should be executed after HTML is inserted to DOM

Reported by: diz Owned by:
Priority: major Milestone: 1.2.2
Component: core Version: 1.2.1
Keywords: Cc:
Blocked by: Blocking:

Description

Hello!

I previously posted this ticket: http://dev.jquery.com/ticket/1598, but by solving that problem for v1.2.1, another one has been created:

When HTML code is added to the DOM (by domManip function), the <SCRIPT> elements it contains should be automatically executed. The problem is that this execution now takes place before the HTML code is inserted!

I corrected this behavior within jQuery 1.2.1, so that the JavaScript code is executed after the DOM manipulation, here is my code from line 385 of the full version:

	domManip: function(args, table, dir, fn) {
		var clone = this.length > 1, a; 

		return this.each(function(){
			if ( !a ) {
				a = jQuery.clean(args, this.ownerDocument);
				if ( dir < 0 )
					a.reverse();
			}

			var obj = this;

			if ( table && jQuery.nodeName(this, "table") && jQuery.nodeName(a[0], "tr") )
				obj = this.getElementsByTagName("tbody")[0] || this.appendChild(document.createElement("tbody"));

			jQuery.each( a, function(){
				var elem = clone ? this.cloneNode(true) : this;
				
				// Changes start from here ========================================
				var script = extractScript(-1, elem);
				fn.call( obj, elem );
				if (script !== false)
				    evalScript(script);
			});
		});
	}
};

function extractScript(i, elem) {
	if ( jQuery.nodeName(elem, "script") ) {
		if ( elem.parentNode )
			elem.parentNode.removeChild(elem);
		
		// Don't need to return a jQuery object if it was a recursive call
		if (i == -1)
			return jQuery(elem);
		else
		    return;
	
	} else if ( elem.nodeType == 1 )
		return jQuery("script", elem).each(extractScript);
	
	return false;
}

function evalScript(script) {
	script.each(function() {
		if ( this.src )
			jQuery.ajax({ url: this.src, async: false, dataType: "script" });
		else
			jQuery.globalEval( this.text || this.textContent || this.innerHTML || "" );
	});
}

What I did, was to split the current evalScript function into two function: one that extracts (and removes) the scripts and one that evaluates what has been extracted. This way, the DOM manipulation can be done after the scripts have been extracted (else Firefox will execute the script twice), but before the scripts are executed.

I didn't extensively test my correction. It solves the differences with JavaScript execution for the different browsers in my case. But I tried to make it in the jQuery logic, so that it can be added easily to the next build after having been reviewed by you. ;-)

Thanks for your work!

Gabriel

Attachments (2)

jquery.patch (1.4 KB) - added by diz 13 years ago.
Patch
jquery-20071012.patch (2.1 KB) - added by diz 12 years ago.
New patch that should correct kennydee's test case.

Download all attachments as: .zip

Change History (14)

Changed 13 years ago by diz

Attachment: jquery.patch added

Patch

comment:1 Changed 13 years ago by c_t

Thanks Gabriel for that patch! I was having the same problem you described and your patch fixed it for me! I hope it gets integrated to the jquery-source soon since this issue probably brakes a lot of ajax-applications!

Cheers, Christoph

comment:2 Changed 13 years ago by diz

A reduced test case has been submitted by Carlos in this ticket: http://dev.jquery.com/ticket/1725

comment:3 Changed 13 years ago by diz

This ticket probably also describes the same bug: http://dev.jquery.com/ticket/1654

comment:4 Changed 13 years ago by kennydee

Hi, Thanks for your patch witch correct this issue but it create one another bug :

If we load $('#mydiv').load('test.html') And test.html contains something like this : <script src='file1.js' type='text/javascript'></script> <script src='file2.js' type='text/javascript'></script>

It seems like file1 and file2 are loaded twice ! One with a timestamp, one without, In firebug you will see something like this in "script" part :

file1.js file1.js?_=1154545454545 file2.js file2.js?_=1154545454599

If you know how to correct this issue ! :) Hope this will integrated into a new jquery release soon !

comment:5 Changed 13 years ago by rrjanbiah

Are you sure that it will work for external sourced scripts like Google AdSense?

comment:6 Changed 12 years ago by diz

Ok kennydee, I corrected the script to behave correctly for your test case.

I have also integrated the extractScript and evalScript function to jQuery and made them a bit easier to use.

Here is the modified jQuery code, again from line 385:

	domManip: function(args, table, dir, fn) {
		var clone = this.length > 1, a; 

		return this.each(function(){
			if ( !a ) {
				a = jQuery.clean(args, this.ownerDocument);
				if ( dir < 0 )
					a.reverse();
			}

			var obj = this;

			if ( table && jQuery.nodeName(this, "table") && jQuery.nodeName(a[0], "tr") )
				obj = this.getElementsByTagName("tbody")[0] || this.appendChild(document.createElement("tbody"));

			jQuery.each(a, function() {
				var elem = clone ? jQuery.extractScript(this.cloneNode(true)) : jQuery.extractScript(this);
				if (elem[1])
					fn.call(obj, elem[1]);
				if (elem[0])
					jQuery.evalScript(elem[0]);
			});
		});
	}
};

// This function should be called with a single argument: the DOM elements in jQuery format.
// Returns an array containing at key 0 the extracted script elements and at key 1 the remaining DOM elements.
jQuery.extractScript = function() {
	var r = !arguments[1],
		elem = r ? arguments[0] : arguments[1];
	if ( jQuery.nodeName(elem, "script") ) {
		if ( elem.parentNode )
			elem.parentNode.removeChild(elem);
		if ( r )
			return [elem, null];
	} else if ( elem.nodeType == 1 )
		return [jQuery("script", elem).each(jQuery.extractScript), elem];
	else
		return [null, elem];
};

// This function should be called with a single argument: the script element alone or in a jQuery list.
jQuery.evalScript = function() {
	var scr = arguments[1] ? arguments[1] : arguments[0];
	if ( scr.each ) {
	    scr.each(jQuery.evalScript);
	} else {
		if ( scr.src )
			jQuery.ajax({ url: scr.src, async: false, dataType: "script" });
		else
			jQuery.globalEval( scr.text || scr.textContent || scr.innerHTML || "" );
	}
};

Thanks for your feedback!

Gabriel

Changed 12 years ago by diz

Attachment: jquery-20071012.patch added

New patch that should correct kennydee's test case.

comment:7 Changed 12 years ago by diz

And this ticket describes the same bug again, with a simple workaround using setTimeout: http://dev.jquery.com/ticket/1793

comment:8 Changed 12 years ago by Jebber007

Does anyone know if this solution will be integrated into the next release?

comment:9 Changed 12 years ago by diz

I've no idea, Jebber007. I'm asking myself the same question!

comment:10 Changed 12 years ago by Jebber007

Great solution, diz. Works like a champ. It's better than mine, because mine doesn't evaluate inline functions correctly. Hope the developers see this an implement it!

comment:11 Changed 12 years ago by john

Resolution: fixed
Status: newclosed

Fixed in SVN rev [3667]. I re-worked the evaluation order, pushing inline scripts onto a stack to be executed upon completion (except for scripts that are inline, before DOM elements - those are executed immediately). See the test case for an example.

comment:12 Changed 12 years ago by diz

Very elegant, how you did that! I tested the nightly build and everything seems to work perfectly here.

Thanks a lot!

Note: See TracTickets for help on using tickets.