[1.9] make replaceWith() clone elements where required #920

Closed
wants to merge 2 commits into from

3 participants

@rvagg

I thought I'd try jQuery over the new comprehensive test suite we have for the various insertion methods in Bonzo and discovered that jQuery's replaceWith() wasn't properly cloning where required.

Inside replaceWith() this.each() is used which then passes a singular collection for each element in this to before() or append() which then invokes domManip(), the clone check inside domManip() (see iNoClone = results.cacheable || l - 1 -- l always == 1).

This change pulls the call to domManip() up into replaceWith() with the basic logic that used to be performed by before() and append() used where appropriate.

With love from @Ender!

@rvagg

Sorry, here's a quick test for you on current jQuery to see what on earth I'm talking about:

jQuery('<div class="replaceable">foo</div><div class="replaceable">bar</div>')
  .appendTo(document.body)
  .replaceWith(jQuery('<span class="replacement">foo</span><span class="replacement">bar</span>'));

console.log(jQuery('.replaceable, .replacement').map(function () { return this.tagName + '[' + this.className + ']' }));

You should see 4 spans rather than 2.

@rwaldron
jQuery Foundation member

Is there a ticket for this? http://bugs.jquery.com

@rvagg
@mikesherov mikesherov and 1 other commented on an outdated diff Sep 8, 2012
src/manipulation.js
- jQuery( this ).remove();
@mikesherov
jQuery Foundation member

Why did this move to after the manipulation?

@rvagg
rvagg added a note Sep 9, 2012

Good question. (1) because I had a bad call to next.parentNode.insertBefore( elem, this ); (should have been insertBefore( elem, next )) which has been fixed in the latest commit. Also because of the nodeType === 1 check, it seemed appropriate to remove regardless of whether the insertion happened or not so doing it below was easiest. Anyway, the nodeType issue is for you guys to sort out so I'll leave that one with you. In the meantime I've fixed the insertBefore() and moved the remove() up in the latest commit: f698ccbb9a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mikesherov mikesherov and 3 others commented on an outdated diff Sep 8, 2012
src/manipulation.js
- jQuery( this ).remove();
+ if ( this.nodeType === 1 ) {
@mikesherov
jQuery Foundation member

Why are we not allowing replaceWith on documentFragments (or other nodeTypes for that matter?)

@mikesherov
jQuery Foundation member

More specifically, the jQuery( this ) .remove() needs to come before appending for the other node types that replaceWith can be called on. The fact that this doesn't break a unit test only shows that our unit tests aren't covering enough cases for replaceWith... I only know this because of the work done in #924

@dmethvin, @rwldrn, thoughts on correct behavior here?

@dmethvin
jQuery Foundation member
dmethvin added a note Sep 8, 2012

I did a quick look when i was trying to solve the "$('<div>abc</div>') has a parentNode" problem in 1.8.1 and just decided the whole method was too far gone to clean it up until 1.9. Since the isDisconnected() method doesn't return false for html conjured from strings, it doesn't have its intended non-effect there--but why should it be a no-op for that case anyway? Also, it's one of the few methods that do a conditional pushStack() which seems totally wrong because you can't reliably do $(selector).find(something).replaceWith(content).end() and be assured you're in the right place. Finally, it only accepts its first arg as content and not all its args like the other ap/pre-penders, although that could be fixed by passing arguments instead of [ value ] in this patch. Plus Rod's bug. So yeah we need much better test coverage here.

In general we don't reliably support anything but nodeType===1 in collections do we? If it works on other node types that's great, but if there's extra work required to support replacing comment and text nodes I'd prefer not to do it.

@rwaldron
jQuery Foundation member
rwaldron added a note Sep 8, 2012

I had a similar feeling when I was looking at the same bug. The function is FUBAR'ed and I'm inclined to halt any further changes.

@mikesherov
jQuery Foundation member

@dmethvin, for the record, prepend and append work on nodeType 11 (documentFragment)...

@rvagg
rvagg added a note Sep 9, 2012

fwiw, I was following the lead of append() which is currently used in replaceWith() although I missed the fact that it was also checking for 11, so I've fixed that in f698ccb, however, before() doesn't do such a check. I'm not sure what the reasoning with these is, if there's a good reason to restrict to 1 and 11 in appendChild() cases but not in insertBefore() then perhaps the 1 & 11 check should be moved to the else above the appendChild() call?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmethvin
jQuery Foundation member

This may not be landed as-is since there are some other cleanups and fixes to be done in 1.9, but the discussion is useful. This isn't a critical enough bug to risk a fix in 1.8.2, it's been around for a while.

@dmethvin
jQuery Foundation member

Hey @rvagg can you rebase this? I'd like to land it in 1.9.

@rvagg

np @dmethvin, done

rvagg added some commits Sep 3, 2012
@rvagg rvagg make replaceWith() clone elements where required
Bypassing domManip() for the whole collection skips the check that
determines whether the element needs to be cloned or not.
Unless the replacement is a non-node (e.g. '<foo>'), it needs to
be cloned where it's used >1 times.
257cb0e
@rvagg rvagg remove old before inserting new, allow nodeType 11 7e72368
@rwaldron
jQuery Foundation member
@rwaldron rwaldron closed this in 551c2c9 Dec 5, 2012
@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@rvagg rvagg Fixes #12449. make replaceWith() clone elements where required. Closes b720c8a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment