Bug Tracker

Ticket #12120 (closed bug: fixed)

Opened 2 years ago

Last modified 2 years ago

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

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

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.

Change History

comment:1 Changed 2 years ago by dmethvin

  • Priority changed from undecided to low
  • Status changed from new to open
  • Type changed from feature to bug
  • Component changed from unfiled to manipulation
  • Milestone changed from None to 1.8

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.

Last edited 2 years ago by dmethvin (previous) (diff)

comment:2 Changed 2 years ago by dmethvin

#12121 is a duplicate of this ticket.

comment:3 Changed 2 years ago by anonymous

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!

comment:4 Changed 2 years ago by dmethvin

  • Priority changed from low to blocker
  • Milestone changed from 1.8 to 1.8.1

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

comment:5 Changed 2 years ago by Dave Methvin

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

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

Changeset: e2eac3f4d2e7f47b67635704df8e3b6675a91ed6

comment:6 Changed 2 years ago by Lafriks <lauris@…>

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/

comment:7 Changed 2 years ago by anonymous

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.

comment:8 Changed 2 years ago by dmethvin

  • Status changed from closed to reopened
  • Resolution fixed deleted

Needs a followup commit to fix.

comment:9 Changed 2 years ago by dmethvin

  • Status changed from reopened to open

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.

comment:10 Changed 2 years ago by Dave Methvin

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

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

comment:11 Changed 2 years ago by anonymous

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/

comment:12 Changed 2 years ago by anonymous

Whoops, sorry for the formatting:

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

comment:13 Changed 2 years ago by dmethvin

  • Status changed from closed to reopened
  • Resolution fixed deleted
  • Milestone changed from 1.8.1 to 1.9

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.

comment:14 Changed 2 years ago by dmethvin

  • Status changed from reopened to open

comment:15 Changed 2 years ago by saiwong

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.

comment:16 Changed 2 years ago by mikesherov

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

comment:17 Changed 2 years ago by mikesherov

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

fixed in #12664.

Note: See TracTickets for help on using tickets.