Bug Tracker

Opened 13 years ago

Closed 12 years ago

#2226 closed bug (duplicate)

serious appendTo bug

Reported by: seven Owned by:
Priority: major Milestone: 1.2.4
Component: core Version: 1.2.2
Keywords: appendto Cc: damir@…
Blocked by: Blocking:


Some time ago I reported a bug, which got bluntly closed. :( http://dev.jquery.com/ticket/1719

To optimize page loading time, what I am doing here is loading a banner from ad server in hidden div on the bottom of the page, and afterwards appending that to placeholder on top of the page.

I tried searching trac for any kind of reference that would help me but no luck. I must emphasize that I classified this as a jQuery bug, because this works in v1.2 but not in any higher version. This could however be normal behavior due to ad server banner invocation code. Since banner invocation code is something I can't influence and it is generally bad thing in the world of semantics and clean code, I am here asking for your help. :)

jQuery v1.2 works well, but 1.2.1 and 1.2.2 don't.

This time I enclosed example files:

Attachments (2)

jquery_append_to.zip (80.4 KB) - added by seven 13 years ago.
2226.diff (1.4 KB) - added by davidserduke 13 years ago.
new possible patch

Download all attachments as: .zip

Change History (11)

Changed 13 years ago by seven

Attachment: jquery_append_to.zip added

comment:1 Changed 13 years ago by seven

Firefox reports this error after appendTo replaces my page content with 5 (!!) banners:

$ is not defined
[Break on this error] $('#advertisement_holder').css('display', 'block');
jquery-pagespecif... (line 8)
missing ; before statement
[Break on this error] on error resume next\n

there is no error on line 8. it seems that dom gets completely overwritten, and jquery object is lost.

IE7 on other hand does not overwrite whole dom, but he pops following error:

Line: 14309141
Char: 4
Error: Expected ';'
Code: 0

comment:2 Changed 13 years ago by davidserduke

This appears to be happening because the div has a script tag in it. Then the div is moved with domManip (called from appendTo). The domManip function evaluates scripts when they are moved (typically scripts are only moved when they are first placed in the dom after being received from an ajax call). This of course causes all sorts of problems especially since this particular script calls document.write which starts stomping on everything.

I'm not sure what the solution is. Perhaps only call evalScripts if the argument was an html string? Or if it is a jQuery object then the script has already been run?

I'll attach a possible patch you can test out.

comment:3 Changed 13 years ago by seven

Hi David, thanks for the patch. It didn't work. :(

scripts.each( evalScript ) gets called anyhow and in that moment hell breaks loose.

However I don't understand how this works in jQuery 1.2.

comment:4 Changed 13 years ago by davidserduke

Ack sorry, I thought I tested it successfully and it worked but apparently not. By the time it gets to domManip you can't tell that way. An earlier function can already have transformed the string in to dom elements. I'll have to think some more about how I can tell the difference.

As for 1.2, I believe this section of code was rewritten to fix a number of other bugs to do with the dom not being ready for an eval of a script when it is eval'd. Unfortunately it seems to have caused this problem in the process.

Changed 13 years ago by davidserduke

Attachment: 2226.diff added

new possible patch

comment:5 Changed 13 years ago by davidserduke

Ok, try out this new patch and see if that works. Thanks for the help testing.

comment:6 Changed 13 years ago by seven

YES! YES! This is it! :) Pack it, wrap it, charge it! :)

Thank you David!

Best regards Neven

comment:7 Changed 13 years ago by seven

Kmeee.. this didn't make into 1.2.3 release and I was really hoping for that to happen. Do you have plans to patch the trunk?

comment:8 Changed 13 years ago by davidserduke


Unfortunately 1.2.3 was largely a compatibility patch for AIR and Drupal with only 2 bugs fixed outside of that which were show stoppers. Hopefully this will make it in to 1.2.4 or whatever the next release will be if it is deemed ok (since it deals with some low level code that could cause backwards compatibility issues).

comment:9 Changed 12 years ago by dmethvin

Resolution: duplicate
Status: newclosed

I've been consolodating these under #3105, it's a tough one but it isn't being ignored.

Note: See TracTickets for help on using tickets.