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 comment:1
Changed February 24, 2013 12:41AM UTC by comment:2
owner: | → getify |
---|---|
status: | new → 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.
Changed February 24, 2013 12:48AM UTC by comment:3
resolution: | → fixed |
---|---|
status: | pending → closed |
@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 comment:4
resolution: | fixed |
---|---|
status: | closed → reopened |
Changed February 24, 2013 12:53AM UTC by comment:5
resolution: | → duplicate |
---|---|
status: | reopened → closed |
Duplicate of #13331.
Changed February 24, 2013 01:01AM UTC by 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 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 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 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 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 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 comment:12
thanks for the clarification.
Changed February 24, 2013 04:06AM UTC by comment:13
cc: | → mkwst, timmywil |
---|---|
component: | unfiled → selector |
milestone: | None → 1.9.2 |
priority: | undecided → high |
resolution: | duplicate |
status: | closed → reopened |
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 i
s and cross the tt
s, 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 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 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 comment:16
status: | reopened → open |
---|
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 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 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 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 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 comment:21
_comment0: | WebKit ticket: https://bugs.webkit.org/show_bug.cgi?id=110718 → 1361760505280334 |
---|
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 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)?
Changed February 25, 2013 03:33AM UTC by 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 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 comment:25
resolution: | → fixed |
---|---|
status: | open → closed |
Fix #13505: Yet another detached add patch
Changeset: 44340c889bbf6d78511f27ebef5786c639010a52
Changed February 25, 2013 04:24AM UTC by comment:26
Fix #13505: Yet another detached add patch
Changeset: 6256c614fd36596debbf52ddcc2c9660705b3d8f
Changed February 25, 2013 04:35AM UTC by 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 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 comment:29
milestone: | 1.9.2 → 1.10/2.0 |
---|
Bulk update to milestone 1.10/2.0
Confirmed. Seems to appear in 1.9.x and up