Skip to main content

Bug Tracker

Side navigation

#13505 closed bug (fixed)

Opened February 24, 2013 12:26AM UTC

Closed February 25, 2013 04:24AM UTC

Last modified May 24, 2013 01:43PM UTC

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

Attachments (0)
Change History (29)

Changed February 24, 2013 12:33AM UTC by rwaldron comment:1

Confirmed. Seems to appear in 1.9.x and up

Changed February 24, 2013 12:41AM UTC by dmethvin comment:2

owner: → getify
status: newpending

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.

Changed February 24, 2013 12:48AM UTC by rwaldron comment:3

resolution: → fixed
status: pendingclosed

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

Changed February 24, 2013 12:52AM UTC by gibson042 comment:4

resolution: fixed
status: closedreopened

Changed February 24, 2013 12:53AM UTC by gibson042 comment:5

resolution: → duplicate
status: reopenedclosed

Duplicate of #13331.

Changed February 24, 2013 01:01AM UTC by rwaldron comment:6

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

Changed February 24, 2013 02:06AM UTC by getify comment:7

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.

Changed February 24, 2013 02:18AM UTC by dmethvin comment:8

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

Changed February 24, 2013 02:21AM UTC by getify comment:9

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.

Changed February 24, 2013 02:27AM UTC by getify comment:10

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. :/

Changed February 24, 2013 03:44AM UTC by gibson042 comment:11

Replying to [comment:10 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/

Changed February 24, 2013 03:59AM UTC by getify comment:12

thanks for the clarification.

Changed February 24, 2013 04:06AM UTC by gibson042 comment:13

cc: → mkwst, timmywil
component: unfiledselector
milestone: None1.9.2
priority: undecidedhigh
resolution: duplicate
status: closedreopened

Replying to [comment:7 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.

Changed February 24, 2013 04:14AM UTC by getify comment:14

_comment0: Running the test 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 \ \ 1361679313186349

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

Changed February 24, 2013 04:45AM UTC by gibson042 comment:15

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

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

Changed February 24, 2013 05:50PM UTC by timmywil comment:16

status: reopenedopen

Replying to [comment:13 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.

Changed February 24, 2013 05:52PM UTC by timmywil comment:17

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

Changed February 24, 2013 06:26PM UTC by gibson042 comment:18

@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?

Changed February 24, 2013 07:30PM UTC by gibson042 comment:19

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

Changed February 24, 2013 10:04PM UTC by timmywil comment:20

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

Changed February 25, 2013 01:14AM UTC by gibson042 comment:21

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

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.

Changed February 25, 2013 02:10AM UTC by getify comment:22

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

Changed February 25, 2013 03:33AM UTC by dmethvin comment:23

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.

Changed February 25, 2013 04:02AM UTC by gibson042 comment:24

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

Changed February 25, 2013 04:24AM UTC by Richard Gibson comment:25

resolution: → fixed
status: openclosed

Fix #13505: Yet another detached add patch

Changeset: 44340c889bbf6d78511f27ebef5786c639010a52

Changed February 25, 2013 04:24AM UTC by Richard Gibson comment:26

Fix #13505: Yet another detached add patch

Changeset: 6256c614fd36596debbf52ddcc2c9660705b3d8f

Changed February 25, 2013 04:35AM UTC by getify comment:27

_comment0: @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 if (for the first time) based on the docs.1361767118556246
_comment1: @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 ii (for the first time) based on the docs.1361767132355168

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

Changed February 25, 2013 09:30PM UTC by dmethvin comment:28

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.

Changed May 24, 2013 01:43PM UTC by dmethvin comment:29

milestone: 1.9.21.10/2.0

Bulk update to milestone 1.10/2.0