Skip to main content

Bug Tracker

Side navigation

#8070 closed bug (fixed)

Opened January 27, 2011 02:49AM UTC

Closed January 28, 2011 04:58PM UTC

jQuery 1.5rc1 issue with handling mutiple objects

Reported by: Shawn Owned by: rwaldron
Priority: blocker Milestone: 1.5
Component: manipulation Version: 1.5rc1
Keywords: Cc:
Blocked by: Blocking:
Description

Issue with 1.5rc1 on handling mutiple objects on change html and append function.

In sample

Both of followings are not working:

$("#list2,#list3").html("<OPTION>1</OPTION><OPTION>2</OPTION><OPTION>3</OPTION>");

$("#list2,#list3").append("<OPTION>1</OPTION><OPTION>2</OPTION><OPTION>3</OPTION>");

Attachments (0)
Change History (11)

Changed January 27, 2011 04:03AM UTC by rwaldron comment:1

component: unfiledmanipulation

Changed January 27, 2011 02:29PM UTC by rwaldron comment:2

#8072 is a duplicate of this ticket.

Changed January 27, 2011 06:18PM UTC by jitter comment:3

milestone: 1.next1.5
owner: → rwaldron
priority: undecidedblocker
status: newassigned

reduced test case happens in every browser.

The cause for this regression is similar to the one for #8052. Check my comment there for more information.

Basically a recent optimization to the clone method with this commit makes wrong assumptions about the existance of getElementsByTagName on DocumentFragments.

Changed January 27, 2011 08:43PM UTC by rwaldron comment:4

description: Issue with 1.5rc1 on handling mutiple objects on change html and append function. \ \ In sample http://jsfiddle.net/WcH8u/ \ \ Both of followings are not working: \ $("#list2,#list3").html("<OPTION>1</OPTION><OPTION>2</OPTION><OPTION>3</OPTION>"); \ $("#list2,#list3").append("<OPTION>1</OPTION><OPTION>2</OPTION><OPTION>3</OPTION>");Issue with 1.5rc1 on handling mutiple objects on change html and append function. \ \ In sample \ \ Both of followings are not working: \ $("#list2,#list3").html("<OPTION>1</OPTION><OPTION>2</OPTION><OPTION>3</OPTION>"); \ $("#list2,#list3").append("<OPTION>1</OPTION><OPTION>2</OPTION><OPTION>3</OPTION>");

jitter, thanks for digging through this one - I'll get this fixed as soon as possible. Unfortunately there were no test cases for this in the test suite.

Changed January 27, 2011 08:53PM UTC by jitter comment:5

Replying to [comment:8 rwaldron]:

jitter, thanks for digging through this one - I'll get this fixed as soon as possible. Unfortunately there were no test cases for this in the test suite.

No problem. Yes it's a shame. After figuring this one out (easy after debugging #8052) I too was thinking. "WTF, this didn't make the whole test suite trip over?".

Don't hurry, you only need to fix it before monday. :D Or just

hit the bail out button.

Changed January 27, 2011 09:24PM UTC by rwaldron comment:6

Replying to [comment:9 jitter]:

Replying to [comment:8 rwaldron]: > jitter, thanks for digging through this one - I'll get this fixed as soon as possible. Unfortunately there were no test cases for this in the test suite. No problem. Yes it's a shame. After figuring this one out (easy after debugging #8052) I too was thinking. "WTF, this didn't make the whole test suite trip over?". Don't hurry, you only need to fix it before monday. :D Or just hit the bail out button

Do you have a fix for this that doesn't compromise the perf optimizations? If you do - then we should move forward with it. I'm afk (answering from mobile phone) for the next few hours.

Changed January 28, 2011 02:01AM UTC by rwaldron comment:7

_comment0: Based on this test case I've produced, I'd like to propose that the issue is not based in the {{{jQuery.clone()}}} optimizations using {{{ getElementsbyTagName }}} \ \ \ http://jsfiddle.net/rwaldron/VQWR6/3/1296180263717340
_comment1: Based on this test case I've produced, I'd like to propose that the issue is not based in the {{{jQuery.clone()}}} optimizations using {{{ getElementsbyTagName }}} \ \ \ http://jsfiddle.net/rwaldron/VQWR6/5/1296181588381342

Based on this test case I've produced, I'd like to propose that the issue is not based in the

jQuery.clone()
optimizations using
 getElementsbyTagName 

http://jsfiddle.net/rwaldron/VQWR6/7/

Changed January 28, 2011 02:42AM UTC by rwaldron comment:8

_comment0: Inside jQuery.buildFragment(), a Document Fragment is created, in the case of {{{<option>}}} elements, the fragments appear empty (firstChild is null), except when you check fragment.firstChild, the option is there. \ \ I'll continue working on this tomorrow1296182757317319
_comment1: Inside jQuery.buildFragment(), a Document Fragment is created, in the case of {{{<option>}}} elements, the fragments appear empty (firstChild is null), except when you check fragment.firstChild, the option is there. fragment.childNodes IS empty. \ \ I'll continue working on this tomorrow1296182991018005
_comment2: Inside jQuery.buildFragment(), a Document Fragment is created, in the case of {{{<option>}}} elements, the fragments appear empty (firstChild is null), except when you check fragment.firstChild, the option is there. fragment.childNodes IS empty. \ \ I'll continue working on this tomorrow \ \ \ http://gyazo.com/e8b37703343988bd2a2c25724b44249d.png1296230971373016

Inside jQuery.buildFragment(), a Document Fragment is created, in the case of these

<option>
elements, the fragments appear empty (firstChild is null), except when you check fragment.firstChild, the option is there. fragment.childNodes IS empty.

I'll continue working on this tomorrow

http://gyazo.com/e8b37703343988bd2a2c25724b44249d.png

Changed January 28, 2011 12:23PM UTC by jitter comment:9

_comment0: I can't seem to reproduce your findings regarding the DocumentFragment just "looking" like it's empty. Neither in Opera 11 (http://i.imgur.com/o6m6P.jpg) nor in Chrome 8 (http://i.imgur.com/bRRSW.jpg). The Fragment is really empty to me on the 2nd iteration when inserting 2 options. \ \ The [http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-B63ED1A3 specification] is also pretty clear about this \ > [...] various operations [...] take DocumentFragment objects as arguments; this results in all the child nodes of the DocumentFragment being moved to the child list of this node. \ \ So you maybe you hit a browser bug there, Chrome 10 ;) I guess? \ \ I toyed some more with the test case and added some comments [http://jsfiddle.net/jitter/STMC3/ test case with comments]. No there is also a test showing this hasn't got "anything" to do with option elements but with bogus behavior if the fragment returned isn't cacheable and contains more then one element which needs to be appended. \ \ ----- \ \ Although this wouldn't fix the underlying issue, it would be interesting to see a proof for the behavior described in this [https://github.com/jquery/jquery/blob/1.5rc1/src/manipulation.js#L428 comment]. I can't seem to reproduce this problem.1296217452375414

I can't seem to reproduce your findings regarding the DocumentFragment just "looking" like it's empty. Neither in Opera 11 (http://i.imgur.com/o6m6P.jpg) nor in Chrome 8 (http://i.imgur.com/bRRSW.jpg). The Fragment is really empty to me on the 2nd iteration when inserting 2 options.

The specification is also pretty clear about this

[...] various operations [...] take DocumentFragment objects as arguments; this results in all the child nodes of the DocumentFragment being moved to the child list of this node.

So you maybe you hit a browser bug there, Chrome 10 ;) I guess?

I toyed some more with the test case and added some comments test case with comments. Now there also is a test showing this hasn't got "anything" to do with option elements but with bogus behavior if the fragment returned isn't cacheable and contains more then one element which needs to be appended. So something with that conditional isn't working right.


Although this wouldn't fix the underlying issue, it would be interesting to see a proof for the behavior described in this comment. I can't seem to reproduce this problem.

Changed January 28, 2011 04:07PM UTC by rwaldron comment:10

_comment0: I've tracked the issue down to this \ [https://github.com/jquery/jquery/blob/885d06c8ef906fa11d130d7d567c871d20ef9ba9/src/manipulation.js#L349 commit] \ \ Specifically: \ \ {{{ i > 0 || results.cacheable || (this.length > 1 && i > 0) }}} \ \ ... when this is reverted to its previous state, the regression is resolved, unfortunately it will also fail the unit tests at: \ \ test/unit/manipulation.js Line 411 "append the same fragment with events (Bug #6997, 5566)" \ \ 1296230912568838

I've tracked the issue down to this

commit

Specifically:

 i > 0 || results.cacheable || (this.length > 1 && i > 0) 

... when this is reverted to its previous state:

 i > 0 || results.cacheable || this.length > 1 

the regression is resolved, unfortunately it will also fail the unit tests at:

test/unit/manipulation.js Line 411 "append the same fragment with events (Bug #6997, 5566)"

Changed January 28, 2011 04:58PM UTC by Colin Snover comment:11

resolution: → fixed
status: assignedclosed

Use the original element/fragment as the last item to be appended to the document instead of the first in order to prevent missing elements when appending to multiple elements. Fixes #8070.

Changeset: 0a0cff9d29c4bd559da689d96c532d06c03fce09