jquery / jquery

Source
Commits
Network (149)
Graphs
Tree: bed759c

jQuery JavaScript Library — Read more

This URL has Read+Write access

Make sure that checked state is cloned properly. Based upon the patch 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.

mmonteleone (author)

Mon Jan 25 15:43:33 -0800 2010

jeresig (committer)

Mon Jan 25 15:43:33 -0800 2010

commit bed759c95ca6d796125653b540e8611dc63b38bb
tree bc8fcc3811466fd18913fd2604586b8c6a96bb04
parent 390186b902c4c1ac13e23754d33ed4d8b3d5fa38

M	src/manipulation.js	15 •••••
M	src/support.js	11 ••••
M	test/unit/manipulation.js	20 ••••

src/manipulation.js

0
@@ -5,6 +5,7 @@ var rinlinejQuery = / jQuery\d+="(?:\d+|null)"/g,
0
   rtagName = /<([\w:]+)/,
0
   rtbody = /<tbody/i,
0
   rhtml = /<|&\w+;/,
0
+  rchecked = /checked\s*(?:[^=]|=\s*.checked.)/i,  // checked="checked" or checked (html5)
0
   fcloseTag = function( all, front, tag ) {
0
     return rselfClosing.test( tag ) ?
0
       all :
0
@@ -35,7 +36,7 @@ jQuery.fn.extend({
0
     if ( jQuery.isFunction(text) ) {
0
       return this.each(function(i) {
0
         var self = jQuery(this);
0
-        return self.text( text.call(this, i, self.text()) );
0
+        self.text( text.call(this, i, self.text()) );
0
       });
0
     }
0
 
0
@@ -251,11 +252,18 @@ jQuery.fn.extend({
0
   domManip: function( args, table, callback ) {
0
     var results, first, value = args[0], scripts = [];
0
 
0
+    // We can't cloneNode fragments that contain checked, in WebKit
0
+    if ( !jQuery.support.checkClone && arguments.length === 3 && typeof value === "string" && rchecked.test( value ) ) {
0
+      return this.each(function() {
0
+        jQuery(this).domManip( args, table, callback, true );
0
+      });
0
+    }
0
+
0
     if ( jQuery.isFunction(value) ) {
0
       return this.each(function(i) {
0
         var self = jQuery(this);
0
         args[0] = value.call(this, i, table ? self.html() : undefined);
0
-        return self.domManip( args, table, callback );
0
+        self.domManip( args, table, callback );
0
       });
0
     }
0
 
0
@@ -326,7 +334,8 @@ function cloneCopyEvent(orig, ret) {
0
 function buildFragment( args, nodes, scripts ) {
0
   var fragment, cacheable, cacheresults, doc;
0
 
0
-  if ( args.length === 1 && typeof args[0] === "string" && args[0].length < 512 && args[0].indexOf("<option") < 0 ) {
0
+  // webkit does not clone 'checked' attribute of radio inputs on cloneNode, so don't cache if string has a checked
0
+  if ( args.length === 1 && typeof args[0] === "string" && args[0].length < 512 && args[0].indexOf("<option") < 0 && (jQuery.support.checkClone || !rchecked.test( args[0] )) ) {
0
     cacheable = true;
0
     cacheresults = jQuery.fragments[ args[0] ];
0
     if ( cacheresults ) {

src/support.js

0
@@ -57,6 +57,7 @@
0
     optSelected: document.createElement("select").appendChild( document.createElement("option") ).selected,
0
 
0
     // Will be defined later
0
+    checkClone: false,
0
     scriptEval: false,
0
     noCloneEvent: true,
0
     boxModel: null
0
@@ -89,9 +90,17 @@
0
     div.cloneNode(true).fireEvent("onclick");
0
   }
0
 
0
+  div = document.createElement("div");
0
+  div.innerHTML = "<input type='radio' name='radiotest' checked='checked'/>";
0
+
0
+  var fragment = document.createDocumentFragment();
0
+  fragment.appendChild( div.firstChild );
0
+
0
+  // WebKit doesn't clone checked state correctly in fragments
0
+  jQuery.support.checkClone = fragment.cloneNode(true).cloneNode(true).lastChild.checked;
0
+
0
   // Figure out if the W3C box model works as expected
0
   // document.body must exist before we can do this
0
-  // TODO: This timeout is temporary until I move ready into core.js.
0
   jQuery(function() {
0
     var div = document.createElement("div");
0
     div.style.width = div.style.paddingLeft = "1px";

test/unit/manipulation.js

0
@@ -197,7 +197,7 @@ test("unwrap()", function() {
0
 });
0
 
0
 var testAppend = function(valueObj) {
0
-  expect(22);
0
+  expect(37);
0
   var defaultText = 'Try them out:'
0
   var result = jQuery('#first').append(valueObj('<b>buga</b>'));
0
   equals( result.text(), defaultText + 'buga', 'Check if text appending works' );
0
@@ -230,6 +230,24 @@ var testAppend = function(valueObj) {
0
   ok( jQuery("#sap").append(valueObj( [] )), "Check for appending an empty array." );
0
   ok( jQuery("#sap").append(valueObj( "" )), "Check for appending an empty string." );
0
   ok( jQuery("#sap").append(valueObj( document.getElementsByTagName("foo") )), "Check for appending an empty nodelist." );
0
+  
0
+  reset();
0
+  jQuery("form").append(valueObj('<input name="radiotest" type="radio" checked="checked" />'));
0
+  jQuery("form input[name=radiotest]").each(function(){
0
+    ok( jQuery(this).is(':checked'), "Append checked radio");
0
+  }).remove();
0
+
0
+  reset();
0
+  jQuery("form").append(valueObj('<input name="radiotest" type="radio" checked    =   \'checked\' />'));
0
+  jQuery("form input[name=radiotest]").each(function(){
0
+    ok( jQuery(this).is(':checked'), "Append alternately formated checked radio");
0
+  }).remove();
0
+
0
+  reset();
0
+  jQuery("form").append(valueObj('<input name="radiotest" type="radio" checked />'));
0
+  jQuery("form input[name=radiotest]").each(function(){
0
+    ok( jQuery(this).is(':checked'), "Append HTML5-formated checked radio");
0
+  }).remove();
0
 
0
   reset();
0
   jQuery("#sap").append(valueObj( document.getElementById('form') ));

Comments

mmonteleone Mon Jan 25 18:25:04 -0800 2010

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 Mon Jan 25 19:34:36 -0800 2010

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 Mon Jan 25 22:44:01 -0800 2010

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 Mon Jan 25 22:46:51 -0800 2010

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

jquery / jquery

Pledgie Donations

Comments