[1.9] Fixes #9960, Append function not working for parent nodes that are nodeType === 9 #924

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

First time pull request!

Running "compare_size:files" (compare_size) task
Sizes - compared to master
    260028       (+46)  dist/jquery.js                                         
     92823       (+38)  dist/jquery.min.js                                     
     33142        (+6)  dist/jquery.min.js.gz 
test/unit/manipulation.js
@@ -530,6 +530,22 @@ test("append(Function) with incoming value", function() {
QUnit.reset();
});
+test("Append not working on XML document, see #9960", function () {
@rwaldron

rwaldron Sep 7, 2012

Member

.append() never gets tested?

Also, the test description should read as "what it should do"

test/unit/manipulation.js
+
+ newNode = jQuery( ":first>state[id='provisioning3']", xml1 );
+
+ equal( newNode.length, 1, "ReplaceWith not working on document nodes." );
@rwaldron

rwaldron Sep 7, 2012

Member

There should be coverage for all manip methods

Updated the pull request, new filesize diff below:

Running "compare_size:files" (compare_size) task
Sizes - compared to master
    260377      (+395)  dist/jquery.js                                         
     92869       (+84)  dist/jquery.min.js                                     
     33148       (+12)  dist/jquery.min.js.gz 

P.S. Appending and prepending to XMLDocument nodes is crazy.

@rwldrn can you recheck this pull request?

Member

rwaldron commented Sep 7, 2012

I wonder if it's worth putting that condition into a function? Can you give it a try and see what the filesize looks like?

Member

mikesherov commented Sep 8, 2012

So, I tested this with @coopersemantics as well, and of our supported browsers, only Safari 5.0 (not 5.1) has the "detached" requirement. That's why we only put it into the test, and not the code. Perhaps we can say something to that effect in the comments. Feel free to amend the test comments if this gets merged.

Owner

dmethvin commented Nov 2, 2012

This looks good to land, sorry it took so long. @coopersemantics can you sign the CLA at http://jquery.github.com/cla.html ? Also can you set your git user name and email to the one you use when you sign the CLA?

Cool. Will do.

@dmethvin dmethvin closed this in 78c1560 Nov 24, 2012

Owner

dmethvin commented Nov 24, 2012

🌟

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment