Skip to content

Commit

Permalink
Workaround for Webkit's broken Node.cloneNode implementation where ra…
Browse files Browse the repository at this point in the history 
…dio inputs checked attributes are not copied.
  • Loading branch information
@mmonteleone
mmonteleone committed Jan 15, 2010
1 parent eb496f7 commit 60c2859
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 2 deletions.
4 changes: 3 additions & 1 deletion 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"|'checked'))/i, // checked="checked" or checked (html5)
fcloseTag = function( all, front, tag ) {
return rselfClosing.test( tag ) ?
all :
Expand Down Expand Up @@ -324,7 +325,8 @@ function cloneCopyEvent(orig, ret) {
function buildFragment( args, nodes, scripts ) {
var fragment, cacheable, cached, 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 && !rchecked.test( args[0] ) ) {
cacheable = true;
cacheresults = jQuery.fragments[ args[0] ];
if ( cacheresults ) {
Expand Down
14 changes: 13 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(25);
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,18 @@ 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:last').append('<input type="radio" id="checkedRadio" checked="checked" />');
ok( jQuery('#checkedRadio').is(':checked'), "Append checked radio");

reset();
jQuery('form:last').append('<input checked = "checked" id="checkedRadio" type= "radio" />');
ok( jQuery('#checkedRadio').is(':checked'), "Append alternately formated checked radio");

reset();
jQuery('form:last').append('<input checked id="checkedRadio" type= "radio" />');
ok( jQuery('#checkedRadio').is(':checked'), "Append HTML5-formated checked radio");

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

6 comments on commit 60c2859

@GarrettS
Copy link

Choose a reason for hiding this comment

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

Can you provide an example of the bug? Where does calling cloneNode on an INPUT whose checked attribute is set result in an element that does not have a checked attribute?

Can you explain what you mean by "Append HTML5-formated checked radio"? (I'm assuming you meant "formatted", not "formated").

Thanks!

@mmonteleone
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is a very old (18 months ago) issue, one that has already been accepted into jQuery.

I've recreated jQuery's own test from its commit in this gist.

In Safari 4.0.5 (the current version as of this pull request), the sample demonstrates Safari failing to clone the checked state. In more recent versions of Safari (just tried in 5.1), the test passes.

And yes, 'formated' was obviously a typo, and not related to the browser bug, but rather to jQuery's workaround involving detection of 'checked="checked"' or simply 'checked' (as is now valid in HTML5) within text passed to the jQuery() function in order to branch on using jQuery's own internal cache which relied on cloneNode, or to create a new node.

@GarrettS
Copy link

Choose a reason for hiding this comment

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

cloneNode is not cloneState. Attributes are not properties.

Boolean attributes, as simply checked, selected, disabled, is not a new thing in HTML5. See HTML4:
http://www.w3.org/TR/html4/intro/sgmltut.html#h-3.3.4.2

@mmonteleone
Copy link
Owner Author

Choose a reason for hiding this comment

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

You are correct that attributes are not properties. If cloneState existed, perhaps jQuery could have used that instead. Your point about checked vs defaultChecked is also valid, and this was not my own sample, but Resig's from jQuery's source. However, I've updated the gist to demonstrate that even when the cloned fragment is appended to the DOM that it is not checked in Safari 4, regardless of whether the test harness reads checked or defaultChecked.

The fact remains that this code failed in Safari 4, but passed in all other browsers (including Safari 5). The fact that it failed broke jQuery for a specific use case: creating checked radio nodes via var newNode = $('<input type="radio" checked />') which internally used a node cache and cloneNode when available. Resulting nodes from such a call would not retain their checked state. I understand you may have a negative opinion of jQuery's use of cloneNode at all (or of jQuery in general), and that's perfectly reasonable. But this was a fix for an issue in that library caused by Safari 4's different (even if it was potentially correct) handling of cloneNode that was breaking functionality in an app I was working on.

I also am aware that 'checked' is not a "new" thing in HTML5 and that HTML 4.0 allowed that as valid as well. XHTML technically did not, and I was simply being terse without going into a lengthy W3C apology.

I'm curious what the need for any of these points are, Garrett. Along with my earlier typo correction, it feels vaguely pedantic. Again, this is an 18 month old known, resolved, issue. I've been meaning to delete this branch for several months.

@GarrettS
Copy link

Choose a reason for hiding this comment

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

I would not mix cloneNode and innerHTML, especially in abstract view; not being added to and rendered in the document.

Thanks for the example.

Have you tried to see what happens not to the clone, but to what happens to the original fragment?

//[1] var clone = fragment.cloneNode(true);
document.body.appendChild(fragment);
alert(document.getElementsByName("radiotest")[0].checked);

Checked in Safari 3, yet when uncommenting out line 1, or "[1]", the result is false and the radio button appears not to be ticked-off. In Safari 3, calling cloneNode on a node that doesn't yet exist in the document and that was created with innerHTML seems to have a side effect of the node upon which it is called.

I don't care so much where DOM quirkiness appears, but under what sorts conditions it appears, so that I know what to avoid doing in code. General purpose innerHTML is definitely a sort of thing that will fail.

Don't get me wrong; innerHTML is great when it works, but it won't always work like Ian Hickson wants it to. I wouldn't ever want to use a general purpose innerHTML wrapper. Sure, maybe fora widget that I can control the HTML over, but not for any arbitrary HTML -- that's just asking for troubles. When HTML5 prevails with its innerHTML, I won't need a wrapper for it. An innerHTML wrapper isn't a good idea.

@mmonteleone
Copy link
Owner Author

Choose a reason for hiding this comment

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

Replied here, but just to add -- I agree cloneNode and innerHTML can be an odd match, but that's probably a discussion that should be taken up with the jQuery team. This was simply a bug fix to allow it to work consistently with Safari 4 since jQuery automatically uses cloneNode within its innerHTML wrapper, which regardless of whether either of us personally agree with it, is still the jQuery-recommended practice that very many use.

I will investigate what happens with the original fragment in Safari 4 later. That's an interesting side effect you've noted.

Please sign in to comment.