Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix #13019. Disconnected nodes with .replaceWith are a noop. #1062

Closed
wants to merge 1 commit into from

2 participants

@dmethvin
Owner

We should just do nothing with disconnected elements here, as we do with .before() and .after(). It's actually more compatible with older versions I think, although some of that may be due to bugs in those versions. In any case it's -19 gzip so there's that. :smiley_cat:

Compare this (1.8.3) to current -git:

http://jsfiddle.net/dmethvin/raR93/

@gibson042
Collaborator

The behavior is documented ("As of jQuery 1.4, .replaceWith() can also work on disconnected DOM nodes"), but I think abandoning it without deprecation can be justified by the same logic we're using with .before and .after.

The case in your fiddle (set containing disconnected elements but first element connected) appears never to have worked, but I suspect that if anyone ever took advantage of this logic in .replaceWith it was on sets containing only disconnected elements. Still, that's what jquery-compat is for, right?

@dmethvin
Owner

Ugh. Yeah, it seems strange to allow this one exception for disconnected nodes when we've eliminated the others. With disconnected elements .replaceWith() works more like the Array .splice() method and it would seem to make more sense to provide Array-like methods in a plugin to do those things.

Also, we're saying "disconnected" in this discussion but we really mean "either has no parent or has a fragment as a parent". I wonder if we should just remove isDisconnected() and look for elem.parentNode instead, allowing someone to treat fragments as containers for manipulation.

@dmethvin dmethvin closed this in f8f52cf
@tp9 tp9 referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@tp9 tp9 referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@mescoda mescoda referenced this pull request from a commit in mescoda/jquery
@dmethvin dmethvin Ref #13019 and gh-1062. Use parentNode check instead of isDisconnecte…
…d().
ecfa312
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 53 deletions.
  1. +3 −11 src/manipulation.js
  2. +3 −42 test/unit/manipulation.js
View
14 src/manipulation.js
@@ -249,8 +249,7 @@ jQuery.fn.extend({
},
replaceWith: function( value ) {
- var self = this,
- isFunc = jQuery.isFunction( value );
+ var isFunc = jQuery.isFunction( value );
// Make sure that the elements are removed from the DOM before they are inserted
// this can help fix replacing a parent with child elements
@@ -258,17 +257,10 @@ jQuery.fn.extend({
value = jQuery( value ).detach();
}
- return this.domManip( [ value ], true, function( elem, i ) {
+ return this.domManip( [ value ], true, function( elem ) {
var next, parent;
- if ( isDisconnected( this ) ) {
- // for disconnected elements, we simply replace
- // with the new content in the set
- self[ i ] = elem;
- return;
- }
-
- if ( this.nodeType === 1 || this.nodeType === 11 ) {
+ if ( !isDisconnected( this ) && this.nodeType === 1 || this.nodeType === 11 ) {
next = this.nextSibling;
parent = this.parentNode;
View
45 test/unit/manipulation.js
@@ -1230,11 +1230,9 @@ var testReplaceWith = function( val ) {
QUnit.reset();
set = jQuery("<div/>").replaceWith(val("<span>test</span>"));
- equal( set[0].nodeName.toLowerCase(), "span", "Replace the disconnected node." );
- equal( set.length, 1, "Replace the disconnected node." );
-
- // #11338
- ok( jQuery("<div>1</div>").replaceWith( val("<span/>") ).is("span"), "#11338, Make sure disconnected node with content is replaced" );
+ equal( set[0].nodeName.toLowerCase(), "div", "No effect on a disconnected node." );
+ equal( set.length, 1, "No effect on a disconnected node." );
+ equal( set[0].childNodes.length, 0, "No effect on a disconnected node." );
non_existant = jQuery("#does-not-exist").replaceWith( val("<b>should not throw an error</b>") );
equal( non_existant.length, 0, "Length of non existant element." );
@@ -1288,43 +1286,6 @@ test( "replaceWith(string) for more than one element", function() {
equal(jQuery("#foo p").length, 0, "verify that all the three original element have been replaced");
});
-test( "replaceWith(string) for collection with disconnected element", function() {
-
- expect( 18 );
-
- var testSet, newSet,
- elem = jQuery("<div />");
-
-
- QUnit.reset();
- testSet = jQuery("#foo p").add( elem );
- equal( testSet.length, 4, "ensuring that test data has not changed" );
-
- newSet = testSet.replaceWith("<span>bar</span>");
- equal( testSet.length, 4, "ensure that we still have the same number of elements" );
- equal( jQuery("#foo span").length, 3, "verify that all the three original elements have been replaced" );
- equal( jQuery("#foo p").length, 0, "verify that all the three original elements have been replaced" );
- equal( testSet.filter("p").length, 3, "ensure we still have the original set of attached elements" );
- equal( testSet.filter("div").length, 0, "ensure the detached element is not in the original set" );
- equal( newSet.filter("p").length, 3, "ensure we still have the original set of attached elements in new set" );
- equal( newSet.filter("div").length, 0, "ensure the detached element has been replaced in the new set" );
- equal( newSet.filter("span").length, 1, "ensure the new element is in the new set" );
-
- QUnit.reset();
- testSet = elem.add( jQuery("#foo p") );
- equal( testSet.length, 4, "ensuring that test data has not changed" );
-
- testSet.replaceWith("<span>bar</span>");
- equal( testSet.length, 4, "ensure that we still have the same number of elements" );
- equal( jQuery("#foo span").length, 3, "verify that all the three original elements have been replaced" );
- equal( jQuery("#foo p").length, 0, "verify that all the three original elements have been replaced" );
- equal( testSet.filter("p").length, 3, "ensure we still have the original set of attached elements" );
- equal( testSet.filter("div").length, 0, "ensure the detached element is not in the original set" );
- equal( newSet.filter("p").length, 3, "ensure we still have the original set of attached elements in new set" );
- equal( newSet.filter("div").length, 0, "ensure the detached element has been replaced in the new set" );
- equal( newSet.filter("span").length, 1, "ensure the new element is in the new set" );
-});
-
test( "replaceAll(String|Element|Array<Element>|jQuery)", function() {
expect( 10 );
Something went wrong with that request. Please try again.