jquery.makeCollapsible: Clean up issues caused by wrong nesting

The way it was done - switching first on action (expand/collapse), then on
elements - caused the logic to be split all over the file.

This caused:
* code duplication (e.g. computing the elements to be acted upon in
  the same way for expanding and collapsing, repeated same comments)
* regressions when the logic was changed for one but not for the other
  (this was the case e.g. with table expanding/collapsing).

As for the second, I fixed all spotted inconsistencies; as for the
first, I'll let the diffstat speak for itself.

Change-Id: I2d2592b4d00424f0c23c493f6de6c824d0714dfc

diff --git a/resources/jquery/jquery.makeCollapsible.js b/resources/jquery/jquery.makeCollapsible.js
index 67bec23..b369584 100644 (file)
--- a/resources/jquery/jquery.makeCollapsible.js
+++ b/resources/jquery/jquery.makeCollapsible.js
@@ -65,53 +65,67 @@ $.fn.makeCollapsible = function () {
                                return;
                        }
 
-                       if ( action === 'collapse' ) {
+                       // Handle different kinds of elements
 
-                               // Collapse the element
-                               if ( $collapsible.is( 'table' ) ) {
+                       if ( $collapsible.is( 'table' ) ) {
+                               // Tables
+                               $containers = $collapsible.find( '> tbody > tr' );
+                               if ( $defaultToggle ) {
+                                       // Exclude table row containing togglelink
+                                       $containers = $containers.not( $defaultToggle.closest( 'tr' ) );
+                               }
+
+                               if ( action === 'collapse' ) {
                                        // Hide all table rows of this table
-                                       // Slide doens't work with tables, but fade does as of jQuery 1.1.3
+                                       // Slide doesn't work with tables, but fade does as of jQuery 1.1.3
                                        // http://stackoverflow.com/questions/467336#920480
-                                       $containers = $collapsible.find( '> tbody > tr' );
-                                       if ( $defaultToggle ) {
-                                               // Exclude tablerow containing togglelink
-                                               $containers = $containers.not( $defaultToggle.closest( 'tr' ) );
-                                       }
-
                                        if ( options.instantHide ) {
                                                $containers.hide();
                                        } else {
                                                $containers.stop( true, true ).fadeOut();
                                        }
+                               } else {
+                                       $containers.stop( true, true ).fadeIn();
+                               }
 
-                               } else if ( $collapsible.is( 'ul' ) || $collapsible.is( 'ol' ) ) {
-                                       $containers = $collapsible.find( '> li' );
-                                       if ( $defaultToggle ) {
-                                               // Exclude list-item containing togglelink
-                                               $containers = $containers.not( $defaultToggle.parent() );
-                                       }
+                       } else if ( $collapsible.is( 'ul' ) || $collapsible.is( 'ol' ) ) {
+                               // Lists
+                               $containers = $collapsible.find( '> li' );
+                               if ( $defaultToggle ) {
+                                       // Exclude list-item containing togglelink
+                                       $containers = $containers.not( $defaultToggle.parent() );
+                               }
 
+                               if ( action === 'collapse' ) {
                                        if ( options.instantHide ) {
                                                $containers.hide();
                                        } else {
                                                $containers.stop( true, true ).slideUp();
                                        }
-
                                } else {
-                                       // <div>, <p> etc.
-                                       $collapsibleContent = $collapsible.find( '> .mw-collapsible-content' );
+                                       $containers.stop( true, true ).slideDown();
+                               }
+
+                       } else {
+                               // Everything else: <div>, <p> etc.
+                               $collapsibleContent = $collapsible.find( '> .mw-collapsible-content' );
 
-                                       // If a collapsible-content is defined, collapse it
-                                       if ( $collapsibleContent.length ) {
+                               // If a collapsible-content is defined, act on it
+                               if ( $collapsibleContent.length ) {
+                                       if ( action === 'collapse' ) {
                                                if ( options.instantHide ) {
                                                        $collapsibleContent.hide();
                                                } else {
                                                        $collapsibleContent.slideUp();
                                                }
-
-                                       // Otherwise assume this is a customcollapse with a remote toggle
-                                       // .. and there is no collapsible-content because the entire element should be toggled
                                        } else {
+                                               $collapsibleContent.slideDown();
+                                       }
+
+                               // Otherwise assume this is a customcollapse with a remote toggle
+                               // .. and there is no collapsible-content because the entire element should be toggled
+                               } else {
+                                       if ( action === 'collapse' ) {
                                                if ( options.instantHide ) {
                                                        $collapsible.hide();
                                                } else {
@@ -121,40 +135,6 @@ $.fn.makeCollapsible = function () {
                                                                $collapsible.slideUp();
                                                        }
                                                }
-                                       }
-                               }
-
-                       } else {
-
-                               // Expand the element
-                               if ( $collapsible.is( 'table' ) ) {
-                                       $containers = $collapsible.find( '>tbody>tr' );
-                                       if ( $defaultToggle ) {
-                                               // Exclude tablerow containing togglelink
-                                               $containers.not( $defaultToggle.parent().parent() ).stop(true, true).fadeIn();
-                                       } else {
-                                               $containers.stop( true, true ).fadeIn();
-                                       }
-
-                               } else if ( $collapsible.is( 'ul' ) || $collapsible.is( 'ol' ) ) {
-                                       $containers = $collapsible.find( '> li' );
-                                       if ( $defaultToggle ) {
-                                               // Exclude list-item containing togglelink
-                                               $containers.not( $defaultToggle.parent() ).stop( true, true ).slideDown();
-                                       } else {
-                                               $containers.stop( true, true ).slideDown();
-                                       }
-
-                               } else {
-                                       // <div>, <p> etc.
-                                       $collapsibleContent = $collapsible.find( '> .mw-collapsible-content' );
-
-                                       // If a collapsible-content is defined, collapse it
-                                       if ( $collapsibleContent.length ) {
-                                               $collapsibleContent.slideDown();
-
-                                       // Otherwise assume this is a customcollapse with a remote toggle
-                                       // .. and there is no collapsible-content because the entire element should be toggled
                                        } else {
                                                if ( $collapsible.is( 'tr' ) || $collapsible.is( 'td' ) || $collapsible.is( 'th' ) ) {
                                                        $collapsible.fadeIn();