Bug Tracker

Modify

Ticket #8070 (closed bug: fixed)

Opened 3 years ago

Last modified 3 years ago

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:
Blocking: Blocked by:

Description (last modified by rwaldron) (diff)

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>");

Change History

comment:2 Changed 3 years ago by rwaldron

  • Component changed from unfiled to manipulation

comment:6 Changed 3 years ago by rwaldron

#8072 is a duplicate of this ticket.

comment:7 Changed 3 years ago by jitter

  • Owner set to rwaldron
  • Priority changed from undecided to blocker
  • Status changed from new to assigned
  • Milestone changed from 1.next to 1.5

 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.

comment:8 follow-up: ↓ 9 Changed 3 years ago by rwaldron

  • Description modified (diff)

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.

comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 3 years ago by jitter

Replying to 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.

comment:10 in reply to: ↑ 9 Changed 3 years ago by rwaldron

Replying to jitter:

Replying to 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.

comment:11 Changed 3 years ago by rwaldron

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/

Last edited 3 years ago by rwaldron (previous) (diff)

comment:12 Changed 3 years ago by rwaldron

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.png

Version 2, edited 3 years ago by rwaldron (previous) (next) (diff)

comment:13 Changed 3 years ago by jitter

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.

Last edited 3 years ago by jitter (previous) (diff)

comment:14 Changed 3 years ago by rwaldron

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)"

Last edited 3 years ago by rwaldron (previous) (diff)

comment:11 Changed 3 years ago by Colin Snover

  • Status changed from assigned to closed
  • Resolution set to fixed

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

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.