Side navigation
#5832 closed bug (duplicate)
Opened January 15, 2010 10:09PM UTC
Closed October 19, 2010 01:11AM UTC
Last modified October 19, 2010 01:11AM UTC
Webkit radio input Node.cloneNode-related regression in jQuery 1.4 (patch and test included)
| Reported by: | mmonteleone | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | 1.4.1 |
| Component: | core | Version: | 1.4 |
| Keywords: | cache cloneNode webkit radio input | Cc: | |
| Blocked by: | Blocking: |
Description
I am truly impressed with the quality of significant optimizations in the recent jQuery 1.4 release. The use of fragment caching in conjunction with browser-native Node.cloneNode for small strings passed to $('...') is quite clever.
I apologize for not noticing this earlier a, but it seems perhaps the use of Node.cloneNode has exposed at least one browser-specific implementation bug causing a regression.
Unlike other engines, Webkit's Node.cloneNode seems to not clone the 'checked' attribute of radio inputs. It does clone the checked attribute of checkbox inputs. This means that code that used to work fine, like...
$('form').append('<input type="radio" checked="checked" />');
...no longer checks the input in Webkit for jQuery 1.4, since this fragment is otherwise small enough to hit the fragment cache and be subsequently cloned, and Webkit does not clone the checked attribute on radio inputs.
Here is code to reproduce in Webkit:
<!-- load jquery 1.3 -->
<script type="text/javascript" src="jquery-1.3.2.js"></script>
<script type="text/javascript">jq13 = jQuery.noConflict()</script>
<!-- load jquery 1.4 -->
<script type="text/javascript" src="jquery-1.4.js"></script>
<script type="text/javascript">jq14 = jQuery.noConflict()</script>
<script type="text/javascript">
jq14(function(){
// in jQuery 1.3, this works fine
jq13('form').append('<input type="radio" id="r1" name="r1" value="r1val" checked="checked" />');
alert('1.3 - should be true: ' + jq13('#r1').is(':checked'));
in jQuery 1.4, the fragment cache means that the new node gets cached, and then cloned
via Node.cloneNode. Webkit will not honor the 'checked' attribute during cloning
// Oddly enough, it would have honored a checkbox's
jq14('form').append('<input type="radio" id="r2" name="r2" value="r1val" checked="checked" />');
alert('1.4 - this is now false: ' + jq14('#r2').is(':checked'));
});
</script>
<body>
<form></form>
</body>
And here is a live running sample: http://michaelmonteleone.net/files/jq14webkit/
So, this isn't technically a jQuery issue as much it is a Webkit one, except for the fact that these kinds of operations always worked before in jQuery before the advent of the fragment cache/Node.cloneNode.
I have prepared a small patch to address this within the buildFragment function by adding another clause to the end of the if branch where it determines whether the fragment should be cacheable (and thus cloned). It's inspecting the fragment via a regex for the presence of various forms of checked="checked", and considering if matches exist. Unit tests are included.
http://github.com/mmonteleone/jquery/commit/60c28596fb1dbce44bf464435d31cc2faa2c1e09
To be fully correct, the if statement should probably also check for jQuery.browser.webkit and a more precise regular expression that specifically matches only radio inputs, but I was concerned this would add too much overhead for such a commonly used function.
I recognize there is probably a much more elegant and efficient way of solving this problem!
Thank you for another fantastic release of a fantastic framework.
Michael Monteleone
(michael@michaelmonteleone.net)
Re-mentioned and resolved in #5929