#11230 closed bug (fixed)
.appendTo .prependTo .insertAfter .insertBefore returns incorrect data for $('not_existing_element')
| Reported by: | yurk | Owned by: | yurk |
|---|---|---|---|
| Priority: | low | Milestone: | 1.9 |
| Component: | manipulation | Version: | 1.7.1 |
| Keywords: | Cc: | ||
| Blocked by: | Blocking: |
Description
$('<h3>3</h3>').appendTo('not_existing_element');
$('<h3>3</h3>').prependTo('not_existing_element');
$('<h3>3</h3>').insertAfter('not_existing_element');
$('<h3>3</h3>').insertBefore('not_existing_element');
expected: return collection with <h3> element
actually: returns an empty collection
Maybe related with http://bugs.jquery.com/ticket/11226
Change History (9)
comment:1 Changed 12 years ago by
| Owner: | set to yurk |
|---|---|
| Status: | new → pending |
comment:3 Changed 12 years ago by
| Component: | unfiled → manipulation |
|---|---|
| Priority: | undecided → low |
| Status: | pending → open |
This one seems to have been around since the dawn of jQuery; I agree that it seems wrong for the set to disappear. Most likely it's *very* rare for anyone to want to continue a chain in that situation. Often I do var x = $("<html stuff>").appendTo("body") but of course the body is always there. :)
comment:4 Changed 11 years ago by
| Milestone: | None → 1.9 |
|---|
Okay, studying the docs for appendTo I am starting to think this is working as documented, even if it seems stupid:
If an element selected this way is inserted elsewhere, it will be moved into the target (not cloned) ... If there is more than one target element, however, cloned copies of the inserted element will be created for each target after the first, and that new set (the original element plus clones) is returned.
Reading between the lines on that, when there are *no* elements, no elements are returned. I would expect the original set to always be returned, but that's not what it's documented to do unless there is only 1 element.
Looking at the current code, it's more distressing that it does a pushStack unless there is just 1 element. That means it's impossible to tell whether to use .end() on an arbitrary set.
Because of the latter issue I could be convinced to change the documented behavior here so that it always returns the original set.
comment:5 Changed 11 years ago by
I spent way too much time looking at this, and am convincing myself we need a breaking change. Consider this case, what does it return?
$("<p>hello</p>").appendTo(".status");
It turns out the answer depends on whether .status selects 0, 1, or N elements, whether the incoming set has 0 or 1 elements, and whether the first element in the incoming set is connected to the document or not.
If .status selects nothing, we get a new set that is empty as this ticket shows. If .status selects one element, and there's only one element in the set, we return the original set and don't .pushStack(). Otherwise we return a new set consisting of the original plus all the clones.
And that's the simplified description ... I think I got that right.
I'm proposing that we always return the original set and document this as a breaking change. At least in my use, I am generally using .appendTo() for appending to one element which will have the same behavior in the new world order.
comment:6 Changed 11 years ago by
Should we always return the new set? Otherwise, you can't sanely chain methods. I think the only confusing thing would be that single element .appendTo() would need to .pushStack(), but at least it would be consistent and useful.
comment:7 Changed 11 years ago by
Scott and I talked in IRC ... I can see the case for stacking. My main concern is that we are consistent here, since we already got rid of the maybe-stack-who-knows behavior of .before(), .after() and .replaceWith().
The benefit of consistently stacking the appended elements is that it's somewhat like .clone() in that it selects the clones, although a case like the one here will get you NOTHING because nothing was appended to anything. It's is probably the least likely to break things because it will return the same elements as before, just stacking the one case it didn't used to stack.
The benefit of returning the current set is that it's smaller and faster, plus it won't create a useless stacked set for a case like var markup = $("<div>").appendTo("body") which we certainly use all over the place in unit tests at least.
I sure wish we knew what people were doing out there. It could turn out that these cases are pretty rare, given how inconsistently they currently behave.
comment:8 Changed 11 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | open → closed |
Fix #11230. .appendTo and pals should always stack.
Changeset: 0018f7700bf8004d573c752ecbf66f299769c4de
comment:9 Changed 11 years ago by
And just to be clear here, the behavior of returning a new empty set is correct, we need to update the docs to clarify this change which was to return a new set when there is only one element and several other special cases.
Thanks for taking the time to contribute to the jQuery project! Please provide a complete reduced test case on jsFiddle to help us assess your ticket.
Additionally, be sure to test against the jQuery Edge version to ensure the issue still exists. To get you started, use this boilerplate: http://jsfiddle.net/FrKyN/ Open the link and click to "Fork" (in the top menu) to get started.