#8500 and #8060 - Appending disconnected radio or checkbox inputs and keeping checked setting #332

Closed
wants to merge 5 commits into
from

2 participants

@timmywil
jQuery Foundation member

IE6/7 can't handle holding on to the checked property when attaching to the DOM, this was apparent mostly when attaching newly-made checkboxes to the DOM or doing dom manipulation that involved reattaching radios or checkboxes.

  • Thanks to gnarf for helping out with this one. Any review is appreciated.

  • I didn't see a way to use an existing input in support.js and needed to create another one that would actually be attached to our mock body.

  • Also, many of the lines are simple formatting changes as I was working on this. The important lines are 512-522 and 644-655

@gnarf gnarf commented on an outdated diff Apr 17, 2011
src/manipulation.js
+ // Resets defaultChecked for any radios and checkboxes about to be appended to the DOM in IE 6/7 (#8060)
+ if ( !jQuery.support.appendChecked ) {
+ fixChecked = function( elem ) {
+ if ( jQuery.nodeName( elem, "input" ) && (elem.type === "checkbox" || elem.type === "radio") ) {
+ elem.defaultChecked = elem.checked;
+ }
+ };
+
+ if ( elem[0] && typeof elem.length === "number" ) {
+ for ( i = 0, len = elem.length; i < len; i++ ) {
+ if ( elem[i].getElementsByTagName ) {
+ fixChecked( elem[i] );
+ jQuery.grep( elem[i].getElementsByTagName("input"), fixChecked );
+ }
+ }
+ } else {
@gnarf
jQuery Foundation member
gnarf added a line comment Apr 17, 2011

not a huge deal, but couldn't just just be an else if ( elem.getElementsByTagName )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gnarf gnarf commented on an outdated diff Apr 17, 2011
src/manipulation.js
+ if ( jQuery.nodeName( elem, "input" ) && (elem.type === "checkbox" || elem.type === "radio") ) {
+ elem.defaultChecked = elem.checked;
+ }
+ };
+
+ if ( elem[0] && typeof elem.length === "number" ) {
+ for ( i = 0, len = elem.length; i < len; i++ ) {
+ if ( elem[i].getElementsByTagName ) {
+ fixChecked( elem[i] );
+ jQuery.grep( elem[i].getElementsByTagName("input"), fixChecked );
+ }
+ }
+ } else {
+ if ( elem.getElementsByTagName ) {
+ fixChecked( elem );
+ jQuery.grep( elem.getElementsByTagName("input"), fixChecked );
@gnarf
jQuery Foundation member
gnarf added a line comment Apr 17, 2011

Since both if statements already call fixChecked, perhaps it would be better to do the jQuery.grep in fixChecked() if the element passed in isn't an input...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timmywil
jQuery Foundation member

Thanks gnarf! Changes added.

@jeresig jeresig commented on the diff Apr 17, 2011
src/manipulation.js
- elem = div.childNodes;
+ // Resets defaultChecked for any radios and checkboxes
+ // about to be appended to the DOM in IE 6/7 (#8060)
+ var len;
+ if ( !jQuery.support.appendChecked ) {
+ if ( elem[0] && typeof (len = elem.length) === "number" ) {
@jeresig
jQuery Foundation member
jeresig added a line comment Apr 17, 2011

This is a bit too cute - can this be broken out and declared inside the for loop?

@timmywil
jQuery Foundation member
timmywil added a line comment Apr 17, 2011

declaring here saves accessing length twice, and declaring in the if only defines len if elem[0] passes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeresig jeresig and 1 other commented on an outdated diff Apr 17, 2011
src/manipulation.js
- elem = div.childNodes;
+ // Resets defaultChecked for any radios and checkboxes
+ // about to be appended to the DOM in IE 6/7 (#8060)
+ var len;
+ if ( !jQuery.support.appendChecked ) {
+ if ( elem[0] && typeof (len = elem.length) === "number" ) {
+ for ( i = 0; i < len; i++ ) {
+ fixChecked( elem[i] );
+ }
+ } else {
@jeresig
jQuery Foundation member
jeresig added a line comment Apr 17, 2011

When is this branch arrived at? Does it need to exist?

@timmywil
jQuery Foundation member
timmywil added a line comment Apr 17, 2011

Apparently clean can receive a nodelist or a single element, so this would be reached in the latter case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeresig jeresig and 2 others commented on an outdated diff Apr 17, 2011
src/manipulation.js
@@ -508,6 +509,18 @@ function getAll( elem ) {
}
}
+// Used in clean, fixes the defaultChecked property
+// on all inputs and checkboxes within html recursively
+function fixChecked( elem ) {
+ if ( jQuery.nodeName( elem, "input" ) ) {
@jeresig
jQuery Foundation member
jeresig added a line comment Apr 17, 2011

It seems like this might be overkill, given that we're doing getElementsByTagName("input"). Perhaps this check can be moved to the for loop that wraps this?

@timmywil
jQuery Foundation member
timmywil added a line comment Apr 17, 2011

Looking into this.

@timmywil
jQuery Foundation member
timmywil added a line comment Apr 17, 2011

Unfortunately I don't think it can be moved. I don't only want to pass inputs to fixChecked since I need to do get any inputs within any element passed to clean. Putting this if statement in clean would require duplicating most of this function within clean twice.

@gnarf
jQuery Foundation member
gnarf added a line comment Apr 17, 2011

Its basically turning it into two functions... fixChecked() can be passed any element (which may contain inputs, hence the later grep) or it can be passed an input element, which if a checkbox gets the checked property fixed.

Maybe it could be 2 functions to avoid calling nodeName on every found "input" tag...

// elem is assumed to be an input
 function fixDefaultChecked( elem ) {
   if ( elem.type == 'radio' || elem.type == 'checkbox') {
     elem.defaultChecked = elem.checked;
   }
 }

 function findInputs( elem ) {
    if (jQuery.nodeName( elem, "input" )) {
      fixDefaultChecked(elem);
    } else if (elem.getElementByTagName) { 
      jQuery.grep( elem.getElementsByTagName( "input "), fixDefaultChecked );
    }
 }
@timmywil
jQuery Foundation member
timmywil added a line comment Apr 18, 2011

Added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeresig
jQuery Foundation member

Landed in commit d274b7b.

@jeresig jeresig closed this Apr 22, 2011
@davidpenuelab davidpenuelab pushed a commit that referenced this pull request Dec 3, 2013
timmywil Landing pull request 332. Appending disconnected radio or checkbox in…
…puts and keeping checked setting Fixes #8060, #8500.

More Details:
 - #332
 - http://bugs.jquery.com/ticket/8060
 - http://bugs.jquery.com/ticket/8500
d274b7b
@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
timmywil Landing pull request 332. Appending disconnected radio or checkbox in…
…puts and keeping checked setting Fixes #8060, #8500.

More Details:
 - jquery#332
 - http://bugs.jquery.com/ticket/8060
 - http://bugs.jquery.com/ticket/8500
d7d56d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment