Bug Tracker

Ticket #13505 (closed bug: fixed)

Opened 22 months ago

Last modified 19 months ago

jquery#add: seems to get items in collection out of order on larger lists

Reported by: getify Owned by: getify
Priority: high Milestone: 1.10/2.0
Component: selector Version: 1.9.1
Keywords: Cc: mkwst, timmywil
Blocking: Blocked by:

Description

Test case (in chrome, canary on mac):  http://jsfiddle.net/HHL3m/

Notice that the JS list has "Rhonda" as the first item, but the DOM output (in Chrome) is "Clayton", then "Victor", etc (at least for me). Rhonda shows up somewhere like 10 items down the list. And it doesn't appear to be anything about the alphabetical sorting or anything like that.

Tested on Firefox (aurora, mac) worked fine.

Tested on Saf6 on mac, also broken, but in a different order ("Keely", "Sloane", etc) than Chrome.

NOTE: The third example in the "Examples" section of the add() documentation page suggests that you should be able to add() a new element (not yet in a document) to an existing collection, and expect the collection order to be kept in the sensible appending order (not silently re-ordered under the covers).

I realize there are other ways to accomplish what I'm trying to do here in this simple test, but my more complex code called for this approach more naturally (keeping a simple collection around instead of, say, a dummy container element like a <div> with children() in it).

Change History

comment:1 Changed 22 months ago by rwaldron

Confirmed. Seems to appear in 1.9.x and up

comment:2 Changed 22 months ago by dmethvin

  • Owner set to getify
  • Status changed from new to pending

Did you try jquery-git? gibson042 put in a fix for this, which is caused by Chrome screwing up its document position operators for disconnected elements.

comment:3 Changed 22 months ago by rwaldron

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

@dmethvin, I tested against jsfiddle's 2.0.x and 1.9.x... Just looked at edge and its fixed.  http://jsfiddle.net/rwaldron/HHL3m/1/

...I couldn't find any other tickets filed.

comment:4 Changed 22 months ago by gibson042

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:5 Changed 22 months ago by gibson042

  • Status changed from reopened to closed
  • Resolution set to duplicate

Duplicate of #13331.

comment:6 Changed 22 months ago by rwaldron

Haha, I didn't look on the second page because I thought that was too far back. Go me.

comment:7 Changed 22 months ago by getify

Is this supposed to be fixed in 1.9.2-pre (from git)? I just tried that version in my app, and I'm still getting bad ordering behavior in chrome.

In addition, I just tried this  http://jsfiddle.net/rwaldron/HHL3m/1/ (which says it's using jquery-edge) in Chrome (canary, mac), and it's broken.

comment:8 Changed 22 months ago by dmethvin

Honestly I'd prefer to put this into the "unsupported behavior" category.

If you want a jQuery object in that order, push the elements onto an array and $(array).

comment:9 Changed 22 months ago by getify

Well, I wish that wasn't the case. But... if you're going to move this into unsupported behavior, please change the docs to remove the example that makes it seem like it is supported behavior. A special note that this is what's happened would be nice, too.

comment:10 follow-up: ↓ 11 Changed 22 months ago by getify

Also, I just tried what you suggested, @dmethvin, here:  http://jsfiddle.net/HHL3m/2/ Unless I misunderstood what you meant, that throws a JS error. :/

comment:11 in reply to: ↑ 10 Changed 22 months ago by gibson042

Replying to getify:

Also, I just tried what you suggested, @dmethvin, here:  http://jsfiddle.net/HHL3m/2/ Unless I misunderstood what you meant, that throws a JS error. :/

Close, but don't wrap the array before sending it to .append:  http://jsfiddle.net/HHL3m/3/

comment:12 Changed 22 months ago by getify

thanks for the clarification.

comment:13 follow-up: ↓ 16 Changed 22 months ago by gibson042

  • Status changed from closed to reopened
  • Cc mkwst, timmywil added
  • Component changed from unfiled to selector
  • Priority changed from undecided to high
  • Milestone changed from None to 1.9.2
  • Resolution duplicate deleted

Replying to getify:

Is this supposed to be fixed in 1.9.2-pre (from git)? I just tried that version in my app, and I'm still getting bad ordering behavior in chrome.

In addition, I just tried this  http://jsfiddle.net/rwaldron/HHL3m/1/ (which says it's using jquery-edge) in Chrome (canary, mac), and it's broken.

Oh, brother. I'm betting that mkwst's  fix has exposed us to some unstable sorting, but just to dot the is and cross the tts, can you report both the WebKit version of your browser and the results from pointing it at  http://swarm.jquery.org/build/jquery/5e29ff7e59a0c97bae02b7512b40486df0f44022/test/index.html?module=Sizzle?

As for support, I really would like to fix this for good. Something like  https://github.com/gibson042/sizzle/blob/83284b9729f0bbe5138f831f7150db495f796413/sizzle.js#L621-L625 oughtta do it. Collections containing disconnected elements will incur indexOf calls, basically making the WebKit fix universal.

...And all because we fixed .add for  mixed connectedness.

comment:14 Changed 22 months ago by getify

Running the test suite, as requested, in my Chrome canary mac, 2 tests failed:

 http://gyazo.com/7dfffd67b07e120ec9c6fed7530bfdf6

Running it in Saf6, works fine. Also, running the actual test-case of this bug ( http://jsfiddle.net/rwaldron/HHL3m/1/) with jquery-edge works in Saf6 but not Chrome.

Here's my chrome info:

Google Chrome 27.0.1421.0 (Official Build 184293) canary OS Mac OS X WebKit 537.32 (@143784) JavaScript V8 3.17.2 Flash 11.6.602.171 User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.32 (KHTML, like Gecko) Chrome/27.0.1421.0 Safari/537.32

Last edited 22 months ago by getify (previous) (diff)

comment:15 Changed 22 months ago by gibson042

 r143784, huh? That'd do it. At least our unit test is good.

Last edited 22 months ago by gibson042 (previous) (diff)

comment:16 in reply to: ↑ 13 Changed 22 months ago by timmywil

  • Status changed from reopened to open

Replying to gibson042:

As for support, I really would like to fix this for good. Something like  https://github.com/gibson042/sizzle/blob/83284b9729f0bbe5138f831f7150db495f796413/sizzle.js#L621-L625 oughtta do it. Collections containing disconnected elements will incur indexOf calls, basically making the WebKit fix universal.

We already landed a version of this. Doesn't seem to have done the job.

comment:17 Changed 22 months ago by timmywil

Unless...is the difference that you want to remove the support test and open up the fix to all browsers?

comment:18 Changed 22 months ago by gibson042

@timmywil exactly. Note that @getify is using Chrome Canary, which has @mkwst's hot-off-the-presses compareDocumentPosition fix. So we're now back to returning 0 for disconnected nodes, which exposes us to unstable native sort algorithms. Forcing stability with the indexOf checks works, so I'd like to do it every time we see a disconnected node, regardless of user agent.

Any objections?

comment:19 Changed 22 months ago by gibson042

For the record, this is the  unstable sorting I'm talking about:  http://jsfiddle.net/vU2uB/

comment:20 Changed 22 months ago by timmywil

Yea, sounds like we don't have much of a choice. My concern here is performance, but this will likely be shorter.

comment:21 Changed 22 months ago by gibson042

WebKit ticket:  https://bugs.webkit.org/show_bug.cgi?id=110718

Edit: disregard; as @getify points out, this is fundamentally a V8 issue, not WebKit.

Last edited 22 months ago by gibson042 (previous) (diff)

comment:22 Changed 22 months ago by getify

It'd be nice if they did fix it. But I doubt it's gonna get fixed, if this thread is any indication:  http://code.google.com/p/v8/issues/detail?id=90

FWIW, I have, temporarily, fixed my own "dammit this is not stable" sorting issues in webkit/chrome by using this JS merge-sort algorithm instead of the built-in sort, so maybe jquery could consider the same (even if just temporarily)?

 http://stackoverflow.com/a/7730507/228852

comment:23 Changed 22 months ago by dmethvin

It still frustrates me that people are using .add() and expecting it to work like a push() operator. We work hard to keep the set in document order; if someone needs a specific order for cases like this $(array) or $(selector).append(array) works great.

comment:24 Changed 22 months ago by gibson042

That's fair, but this is another bed we made for ourselves... multiple times, in fact. :\

comment:25 Changed 22 months ago by Richard Gibson

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

Fix #13505: Yet another detached add patch

Changeset: 44340c889bbf6d78511f27ebef5786c639010a52

comment:26 Changed 22 months ago by Richard Gibson

Fix #13505: Yet another detached add patch

Changeset: 6256c614fd36596debbf52ddcc2c9660705b3d8f

comment:27 Changed 22 months ago by getify

@dmethvin--

with all due respect, the docs (as I mentioned above) make it seem like you can add a new element *after* an existing element, in a collection, and have it be added to the DOM in that collection order. Nothing about that example says "be careful, these might not end up in the order you expect, depending on which browser you use."

` <body> <p>Hello</p> <script>$("p").clone().add("<span>Again</span>").appendTo(document.body);</script> </body> `

If it's not intended that we should be able to rely on the order of items in a collection made partially or entirely by add(), then please just consider changing the documentation (removing that example) so we don't keep getting the wrong idea.

For my part, I had no idea there was any back-story (as you clearly share) to add(), and I just showed up and tried to use it (for the first time) based on the docs.

Last edited 22 months ago by getify (previous) (diff)

comment:28 Changed 22 months ago by dmethvin

Sorry if it came off as whining...I guess it was. The issue is that there are multiple conflicting goals here. Intuitively I suspect everyone would like the disconnected element case to have .add() just tack things on the end. But for similar intuitive reasons most people want connected elements to be in document order.

The problem is that the .add() method has no information about which case to expect (or more pathological cases such as a mix of connected and disconnected elements, even elements from different documents). So doing things "the right way" for every case means ugly workarounds and still may not meet everyone's definition of "right".

If we can declare some cases out of bounds, jQuery can stay faster and smaller. There are still several good ways to accomplish the goal, so we haven't made it any worse for the jQuery user. In fact, I suspect the revised code is faster and easier for most people to follow.

I agree the docs are misleading about several of these issues, we've fallen behind a bit on the docs and now that the release is out I'm planning on getting things in sync. We may yet still support set ordering as described there on all browsers, but I think it's a misuse of .add() and again the revised code is better.

comment:29 Changed 19 months ago by dmethvin

  • Milestone changed from 1.9.2 to 1.10/2.0

Bulk update to milestone 1.10/2.0

Note: See TracTickets for help on using tickets.