Bug Tracker

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#5832 closed bug (duplicate)

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@…)

Change History (3)

comment:1 Changed 8 years ago by mmonteleone

Re-mentioned and resolved in #5929

comment:2 Changed 7 years ago by snover

Resolution: duplicate
Status: newclosed

comment:3 Changed 7 years ago by snover

Duplicate of #5929.

Note: See TracTickets for help on using tickets.