Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

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

Closed
wants to merge 2 commits into from

4 participants

Rod Vagg Rick Waldron Dave Methvin Mike Sherov
Rod Vagg

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!

Rod Vagg

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.

Rick Waldron
Collaborator

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

Rod Vagg
src/manipulation.js
((9 lines not shown))
279 278
280   - jQuery( this ).remove();
2
Mike Sherov Collaborator

Why did this move to after the manipulation?

Rod Vagg
rvagg added a note

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
src/manipulation.js
((9 lines not shown))
279 278
280   - jQuery( this ).remove();
  279 + if ( this.nodeType === 1 ) {
6
Mike Sherov Collaborator

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

Mike Sherov Collaborator

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?

Dave Methvin Owner

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.

Rick Waldron Collaborator

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.

Mike Sherov Collaborator

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

Rod Vagg
rvagg added a note

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
Dave Methvin
Owner

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.

Dave Methvin
Owner

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

Rod Vagg

np @dmethvin, done

rvagg added some commits
Rod Vagg 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
Rod Vagg rvagg remove old before inserting new, allow nodeType 11 7e72368
tp9 referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 1 author.

Nov 09, 2012
Rod Vagg 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
Rod Vagg rvagg remove old before inserting new, allow nodeType 11 7e72368
This page is out of date. Refresh to see the latest.

Showing 2 changed files with 27 additions and 19 deletions. Show diff stats Hide diff stats

  1. +17 19 src/manipulation.js
  2. +10 0 test/unit/manipulation.js
36 src/manipulation.js
@@ -276,32 +276,29 @@ jQuery.fn.extend({
276 276 value = jQuery( value ).detach();
277 277 }
278 278
279   - this.each( function( i ) {
280   - var next = this.nextSibling,
281   - parent = this.parentNode,
282   - // HTML argument replaced by "this" element
283   - // 1. There were no supporting tests
284   - // 2. There was no internal code relying on this
285   - // 3. There was no documentation of an html argument
286   - val = !isFunc ? value : value.call( this, i, this );
  279 + return this.domManip( [ value ], true, function( elem, i ) {
  280 + var next, parent;
287 281
288 282 if ( isDisconnected( this ) ) {
289   - // for disconnected elements, we replace with the new content in the set. We use
290   - // clone here to ensure that each replaced instance is unique
291   - self[ i ] = jQuery( val ).clone()[ 0 ];
  283 + // for disconnected elements, we simply replace with the new content in the set
  284 + self[ i ] = elem;
292 285 return;
293 286 }
294 287
295   - jQuery( this ).remove();
  288 + if ( this.nodeType === 1 || this.nodeType === 11 ) {
  289 + next = this.nextSibling;
  290 + parent = this.parentNode;
296 291
297   - if ( next ) {
298   - jQuery( next ).before( val );
299   - } else {
300   - jQuery( parent ).append( val );
  292 + jQuery( this ).remove();
  293 +
  294 + if ( next ) {
  295 + next.parentNode.insertBefore( elem, next );
  296 + } else {
  297 + parent.appendChild( elem );
  298 + }
301 299 }
302   - });
303 300
304   - return this;
  301 + });
305 302 },
306 303
307 304 detach: function( selector ) {
@@ -356,7 +353,8 @@ jQuery.fn.extend({
356 353 this[i],
357 354 i === iNoClone ?
358 355 fragment :
359   - jQuery.clone( fragment, true, true )
  356 + jQuery.clone( fragment, true, true ),
  357 + i
360 358 );
361 359 }
362 360 }
10 test/unit/manipulation.js
@@ -1205,6 +1205,16 @@ test("replaceWith(string) for collection with disconnected element", function(){
1205 1205 equal(newSet.filter("span").length, 1, "ensure the new element is in the new set");
1206 1206 });
1207 1207
  1208 +// #12449
  1209 +test("replaceWith([]) where replacing element requires cloning", function () {
  1210 + expect(2);
  1211 + jQuery("#qunit-fixture").append("<div class='replaceable'></div><div class='replaceable'></div><div class='replaceable'></div>" );
  1212 + // replacing set needs to be cloned so it can cover 3 replacements
  1213 + jQuery("#qunit-fixture .replaceable").replaceWith( jQuery("<span class='replaced'></span><span class='replaced'></span>") );
  1214 + equal( jQuery("#qunit-fixture").find(".replaceable").length, 0, "Make sure replaced elements were removed" );
  1215 + equal( jQuery("#qunit-fixture").find(".replaced").length, 6, "Make sure replacing elements were cloned" );
  1216 +});
  1217 +
1208 1218 test("replaceAll(String|Element|Array<Element>|jQuery)", function() {
1209 1219 expect(10);
1210 1220 jQuery("<b id='replace'>buga</b>").replaceAll("#yahoo");

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.