Skip to main content

Bug Tracker

Side navigation

#12120 closed bug (fixed)

Opened July 22, 2012 10:11AM UTC

Closed October 22, 2012 04:06PM UTC

Inconsistency of .end() with respect to .after()

Reported by: anonymous Owned by:
Priority: blocker Milestone: 1.9
Component: manipulation Version: git
Keywords: Cc:
Blocked by: Blocking:
Description

.after() only changes the stack if the first element in the set is disconnected. This is unfortunate, because .end() cannot be used with the same result if .after() is called on an ''empty'' set vs a ''non-empty'' set. You don't always know whether there are elements in the set, and more importantly, it should not matter.

Fiddle: http://jsfiddle.net/3qqXG/

The second div doesn't turn red because it doesn't have any children. The fiddle is contrived, of course, but in real cases you don't know whether there are any children - I'd expect both .end() calls to select the div again.

I did note that the docs state that .after() acts differently on a set of disconnected nodes. However, jQuery thinks an empty set is disconnected as well, causing this issue.

Attachments (0)
Change History (17)

Changed July 22, 2012 02:43PM UTC by dmethvin comment:1

_comment0: I agree this limits the utility of chaining. \ \ The easiest way out would be to remove support for using before/after to add something to an empty set, which was added without a lot of thought for how it might affect these situations and has been a thorn in our side ever since. It seems potentially like quite a breaking change to some code, though. \ \ To fix this specific case, if the div was always stacked you'd end up with the `.after` adding to an empty set 1342968309737845
component: unfiledmanipulation
milestone: None1.8
priority: undecidedlow
status: newopen
type: featurebug

I agree this limits the utility of chaining.

The easiest way out would be to remove support for using before/after to add something to an empty set, which was added without a lot of thought for how it might affect these situations and has been a thorn in our side ever since. It seems potentially like quite a breaking change to some code, though.

To fix this specific case, if the div was always stacked you'd end up with the .after adding to an empty set and then being thrown away by the .end, which shouldn't break anything.

Changed July 22, 2012 04:01PM UTC by dmethvin comment:2

#12121 is a duplicate of this ticket.

Changed August 10, 2012 05:42PM UTC by anonymous comment:3

Sorry for possibly being rude, but 1.8 has been released without this fix, although the milestone on this issue is set to 1.8. This issue isn't urgent, but do you happen to know if it will be included in 1.8.1? Thanks!

Changed August 10, 2012 05:44PM UTC by dmethvin comment:4

milestone: 1.81.8.1
priority: lowblocker

I meant to reset the milestones on the handful of tickets that didn't make it.

Changed August 24, 2012 03:28AM UTC by Dave Methvin comment:5

resolution: → fixed
status: openclosed

Fix #12120. Always stack .before/.after, and fix disconnected nodes.

Changeset: e2eac3f4d2e7f47b67635704df8e3b6675a91ed6

Changed August 24, 2012 09:01AM UTC by Lafriks <lauris@nix.lv> comment:6

This change does throw an error ("Invalid argument") when calling prepend and html string that is being prepended does contain space before/after first tag. It does not without this change.

Try with IE9:

http://jsfiddle.net/xhZJz/7/

Changed August 24, 2012 10:27AM UTC by anonymous comment:7

Hmm, the issue is still reproducible in the fiddle, unfortunately.

The problem doesn't lie in arguments.length (in the fiddle it's always 1), but rather in the length of the set. The isDisconnected(this[0]) call yields false for an empty set, and consequently .after adds one stack too many for the empty set case.

Changed August 24, 2012 12:53PM UTC by dmethvin comment:8

resolution: fixed
status: closedreopened

Needs a followup commit to fix.

Changed August 24, 2012 02:18PM UTC by dmethvin comment:9

status: reopenedopen

Ignore the IE issues for now, let's address the title of the ticket, which is "Inconsistency of .end() with respect to .after()"

We need to do something consistent with .after() to make it behave predictably. The two choices that I saw were that we can either *always* pushStack regardless of the jQuery object contents and incoming arguments, or *never* pushStack.

By our internal convention, any jQuery method that may change the current set always does a pushStack. We're shoehorning two different things into this method: 1) put content after the elements in a set, and 2) put content **into** the set. So the second is a modifying behavior, and whether we do 1 or 2 depended on whether the first element in the set is disconnected, or the set is empty, or there were no arguments, which isn't very consistent at all.

I was proposing that we always pushStack. The original test case is not expecting this. It wants an empty set to be seen as connected and essentially have a no-op effect, but it also wants the sometimes-pushStack behavior to be preserved. That just leaves us differently inconsistent doesn't it? **I'd be totally okay with never-pushStack as well,** but to me some sort of consistency here is important like the title of the ticket says.

Perhaps I've been looking at this too long. Thoughts welcome.

Changed August 24, 2012 04:01PM UTC by Dave Methvin comment:10

resolution: → fixed
status: openclosed

Revert "Fix #12120. Always stack .before/.after, and fix disconnected nodes."

This reverts commit e2eac3f4d2e7f47b67635704df8e3b6675a91ed6.

There is a 1.7 regression with isDisconnected() that we should fix before tackling this.

Changeset: a5be98620665c8af4d21a043a871e6a965de4fbc

Changed August 24, 2012 07:10PM UTC by anonymous comment:11

Thanks for sharing your thoughts. I believe that never pushing the stack would be the most consistent, given that e.g. .append exhibits the same behaviour.

It should be noted that alot of jQuery's functions are actually inconsistent with respect to pushing the stack. For example, appendTo etc. always push the stack except for specific cases (see the source). This causes a different stack between:

$("p").appendTo("body"); prevObject: empty set

$("<p>").appendTo("body"); prevObject: undefined

Also, it should be noted that doing disconnected.after("foo").end() still includes the "foo" text node. This is because jQuery.merge modifies the current set (this) **and** returns the modified set. That set is passed to pushStack which causes two same sets on the stack.

Here's a jsFiddle illustrating append vs after: http://jsfiddle.net/3qqXG/5/

Changed August 24, 2012 07:12PM UTC by anonymous comment:12

Whoops, sorry for the formatting:

$("p").appendTo("body");   // prevObject: empty set
$("<p>").appendTo("body"); // prevObject: undefined

Changed August 24, 2012 08:06PM UTC by dmethvin comment:13

milestone: 1.8.11.9
resolution: fixed
status: closedreopened

Most of the stack inconsistencies you're mentioning shouldn't exist IMO and are probably bugs, although a few are explicit like the one in .replaceWith that I haven't explored.

Thinking about it further today, I'm inclined to never stack .before or .after.

A complicating factor is that #12392 is impacting .before, .after, .replaceWith and .add in some cases. The patch I backed out tried to fix that problem but there were regressions in all the IEs.

The ability of jQuery.merge to modify its first arg is great, until you don't want it.

Changed August 24, 2012 08:07PM UTC by dmethvin comment:14

status: reopenedopen

Changed October 22, 2012 03:24PM UTC by saiwong comment:15

The after() method has been refactored to be more consistent. The after() method is now pure DOM manipulation and does not affect the stack. #12664

Recommend this ticket be closed.

Changed October 22, 2012 04:02PM UTC by mikesherov comment:16

I agree with close, but I'll leave it to dmethvin.

Changed October 22, 2012 04:06PM UTC by mikesherov comment:17

resolution: → fixed
status: openclosed

fixed in #12664.