Skip to content

Commit

Permalink
Make sure that checked state is cloned properly. Based upon the patch…
Browse files Browse the repository at this point in the history 
… by Michael, required better test cases and in doing so found more edge cases. Introduced a new check into jQuery.support as a result. Fixes #5929.
  • Loading branch information
@mmonteleone @jeresig
mmonteleone authored and jeresig committed Jan 25, 2010
1 parent 390186b commit bed759c
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 5 deletions.
15 changes: 12 additions & 3 deletions src/manipulation.js
View file
Expand Up @@ -5,6 +5,7 @@ var rinlinejQuery = / jQuery\d+="(?:\d+|null)"/g,
rtagName = /<([\w:]+)/,
rtbody = /<tbody/i,
rhtml = /<|&\w+;/,
rchecked = /checked\s*(?:[^=]|=\s*.checked.)/i, // checked="checked" or checked (html5)
fcloseTag = function( all, front, tag ) {
return rselfClosing.test( tag ) ?
all :
Expand Down Expand Up @@ -35,7 +36,7 @@ jQuery.fn.extend({
if ( jQuery.isFunction(text) ) {
return this.each(function(i) {
var self = jQuery(this);
return self.text( text.call(this, i, self.text()) );
self.text( text.call(this, i, self.text()) );
});
}

Expand Down Expand Up @@ -251,11 +252,18 @@ jQuery.fn.extend({
domManip: function( args, table, callback ) {
var results, first, value = args[0], scripts = [];

// We can't cloneNode fragments that contain checked, in WebKit
if ( !jQuery.support.checkClone && arguments.length === 3 && typeof value === "string" && rchecked.test( value ) ) {
return this.each(function() {
jQuery(this).domManip( args, table, callback, true );
});
}

if ( jQuery.isFunction(value) ) {
return this.each(function(i) {
var self = jQuery(this);
args[0] = value.call(this, i, table ? self.html() : undefined);
return self.domManip( args, table, callback );
self.domManip( args, table, callback );
});
}

Expand Down Expand Up @@ -326,7 +334,8 @@ function cloneCopyEvent(orig, ret) {
function buildFragment( args, nodes, scripts ) {
var fragment, cacheable, cacheresults, doc;

if ( args.length === 1 && typeof args[0] === "string" && args[0].length < 512 && args[0].indexOf("<option") < 0 ) {
// webkit does not clone 'checked' attribute of radio inputs on cloneNode, so don't cache if string has a checked
if ( args.length === 1 && typeof args[0] === "string" && args[0].length < 512 && args[0].indexOf("<option") < 0 && (jQuery.support.checkClone || !rchecked.test( args[0] )) ) {
cacheable = true;
cacheresults = jQuery.fragments[ args[0] ];
if ( cacheresults ) {
Expand Down
11 changes: 10 additions & 1 deletion src/support.js
View file
Expand Up @@ -57,6 +57,7 @@
optSelected: document.createElement("select").appendChild( document.createElement("option") ).selected,

// Will be defined later
checkClone: false,
scriptEval: false,
noCloneEvent: true,
boxModel: null
Expand Down Expand Up @@ -89,9 +90,17 @@
div.cloneNode(true).fireEvent("onclick");
}

div = document.createElement("div");
div.innerHTML = "<input type='radio' name='radiotest' checked='checked'/>";

var fragment = document.createDocumentFragment();
fragment.appendChild( div.firstChild );

// WebKit doesn't clone checked state correctly in fragments
jQuery.support.checkClone = fragment.cloneNode(true).cloneNode(true).lastChild.checked;

// Figure out if the W3C box model works as expected
// document.body must exist before we can do this
// TODO: This timeout is temporary until I move ready into core.js.
jQuery(function() {
var div = document.createElement("div");
div.style.width = div.style.paddingLeft = "1px";
Expand Down
20 changes: 19 additions & 1 deletion test/unit/manipulation.js
View file
Expand Up @@ -197,7 +197,7 @@ test("unwrap()", function() {
});

var testAppend = function(valueObj) {
expect(22);
expect(37);
var defaultText = 'Try them out:'
var result = jQuery('#first').append(valueObj('<b>buga</b>'));
equals( result.text(), defaultText + 'buga', 'Check if text appending works' );
Expand Down Expand Up @@ -230,6 +230,24 @@ var testAppend = function(valueObj) {
ok( jQuery("#sap").append(valueObj( [] )), "Check for appending an empty array." );
ok( jQuery("#sap").append(valueObj( "" )), "Check for appending an empty string." );
ok( jQuery("#sap").append(valueObj( document.getElementsByTagName("foo") )), "Check for appending an empty nodelist." );

reset();
jQuery("form").append(valueObj('<input name="radiotest" type="radio" checked="checked" />'));
jQuery("form input[name=radiotest]").each(function(){
ok( jQuery(this).is(':checked'), "Append checked radio");
}).remove();

reset();
jQuery("form").append(valueObj('<input name="radiotest" type="radio" checked = \'checked\' />'));
jQuery("form input[name=radiotest]").each(function(){
ok( jQuery(this).is(':checked'), "Append alternately formated checked radio");
}).remove();

reset();
jQuery("form").append(valueObj('<input name="radiotest" type="radio" checked />'));
jQuery("form input[name=radiotest]").each(function(){
ok( jQuery(this).is(':checked'), "Append HTML5-formated checked radio");
}).remove();

reset();
jQuery("#sap").append(valueObj( document.getElementById('form') ));
Expand Down

4 comments on commit bed759c

@mmonteleone
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing up my crummy regex and including this, John.

And good call on adding support.checkClone. I hadn't considered that in what I saw as an all-or-nothing decision to be dirty and explicitly check browser.webkit, or always test the regex and be slow.

@jeresig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Michael, Thanks for the patch. Note that I ended up having to change a bunch of things: I couldn't get the test cases to fail so I rewrote them into a failing state. In doing so I ran across another set of problems (buildFragment isn't the only place where a fragment can be cloned, it can also happen in domManip - hence the new block of code there). It ended up being a surprisingly complex problem - especially building support.checkClone - because it turns out this only happens when you clone an already cloned fragment (a strange case, to be sure). Anyway, thanks for your help, I appreciate it!

@mmonteleone
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right. I wrongly committed "cleaned up" tests at the last minute without re-verifying they still failed against unpatched 1.4.0. And I also totally missed domManip. Thanks for not blindly accepting patches from strangers! :)

Not that it's of much consequence now, but I do think Webkit's issue is not related to duplicate clones nor does jQuery require duplicate calls to cloneNode to establish support.checkClone. I can reproduce Webkit's issue with only a single call to cloneNode. http://michaelmonteleone.net/files/jq14Webkit/Webkitclone.html (Reproducible in Safari 4.0.4 and Chromium nightly).

var div = document.createElement("div");
div.innerHTML = "<input type='radio' name='radiotest' checked='checked'/>";
console.log("should be true: " + div.lastChild.checked);
console.log("should be true (but is false in Webkit): " + div.cloneNode(true).lastChild.checked);

which matches with my original experience in 1.4.0 where there is theoretically only a single clone occurring in the following http://michaelmonteleone.net/files/jq14Webkit/jq140clone.html:

// this is the only code present.  returns false in Webkit
var checked = $("<input type='radio' name='radiotest' checked='checked'/>").is(":checked");

So changing to jQuery.support.checkClone = fragment.cloneNode(true).lastChild.checked; still seems to do the job.

And just to add one more big of Webkit weirdness: Webkit only exhibits this issue when the input has a name attribute.

But yes, I clouded the issue with my broken tests. And I will be sure to help test more while still in pre-release next time. Thank you again for your thoroughness and improvements to the original patch.

@mmonteleone
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That second link without the HTTP 300 inducing trailing colon: http://michaelmonteleone.net/files/jq14Webkit/jq140clone.html

Please sign in to comment.