From 2ebbdbebec347c4f029b90e578f8d03034478794 Mon Sep 17 00:00:00 2001
From: MatmaRex <matma.rex@gmail.com>
Date: Sun, 10 Mar 2013 21:19:06 +0100
Subject: [PATCH] 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
---
 resources/jquery/jquery.makeCollapsible.js | 96 +++++++++-------------
 1 file changed, 38 insertions(+), 58 deletions(-)

diff --git a/resources/jquery/jquery.makeCollapsible.js b/resources/jquery/jquery.makeCollapsible.js
index 67bec23ceb..b369584acb 100644
--- 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();
-- 
2.20.1