From: Richard Gibson <richard.gibson@gmail.com>
Date: Mon, 4 Apr 2016 13:58:14 +0000 (-0400)
Subject: CSS: Toggle detached elements as visible unless they have display: none
X-Git-Tag: 3.0.0-rc1~42
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=755e7ccf018eb150eddefe78063a9ec58b3229e3;p=jquery.git

CSS: Toggle detached elements as visible unless they have display: none

Fixes gh-2863
Closes gh-3037
---

diff --git a/src/css/showHide.js b/src/css/showHide.js
index 45f8f0696..7e9002ca5 100644
--- a/src/css/showHide.js
+++ b/src/css/showHide.js
@@ -54,12 +54,7 @@ function showHide( elements, show ) {
 					elem.style.display = "";
 				}
 			}
-			if ( elem.style.display === "" && jQuery.css( elem, "display" ) === "none" &&
-
-					// Support: Firefox 43+
-					// Don't set inline display on disconnected elements with computed display: none
-					jQuery.contains( elem.ownerDocument, elem ) ) {
-
+			if ( elem.style.display === "" && isHiddenWithinTree( elem ) ) {
 				values[ index ] = getDefaultDisplay( elem );
 			}
 		} else {
diff --git a/src/css/var/isHiddenWithinTree.js b/src/css/var/isHiddenWithinTree.js
index 1c81f6480..c01d228d8 100644
--- a/src/css/var/isHiddenWithinTree.js
+++ b/src/css/var/isHiddenWithinTree.js
@@ -5,14 +5,29 @@ define( [
 	// css is assumed
 ], function( jQuery ) {
 
-	// This function differs from the :hidden selector
-	// in that it intentionally ignores hidden ancestors (gh-2404)
+	// isHiddenWithinTree reports if an element has a non-"none" display style (inline and/or
+	// through the CSS cascade), which is useful in deciding whether or not to make it visible.
+	// It differs from the :hidden selector (jQuery.expr.pseudos.hidden) in two important ways:
+	// * A hidden ancestor does not force an element to be classified as hidden.
+	// * Being disconnected from the document does not force an element to be classified as hidden.
+	// These differences improve the behavior of .toggle() et al. when applied to elements that are
+	// detached or contained within hidden ancestors (gh-2404, gh-2863).
 	return function( elem, el ) {
 
 		// isHiddenWithinTree might be called from jQuery#filter function;
 		// in that case, element will be second argument
 		elem = el || elem;
-		return jQuery.css( elem, "display" ) === "none" ||
-			!jQuery.contains( elem.ownerDocument, elem );
+
+		// Inline style trumps all
+		return elem.style.display === "none" ||
+			elem.style.display === "" &&
+
+			// Otherwise, check computed style
+			// Support: Firefox <=43 - 45
+			// Disconnected elements can have computed display: none, so first confirm that elem is
+			// in the document.
+			jQuery.contains( elem.ownerDocument, elem ) &&
+
+			jQuery.css( elem, "display" ) === "none";
 	};
 } );
diff --git a/test/unit/css.js b/test/unit/css.js
index 26660ab4f..e04ad5dea 100644
--- a/test/unit/css.js
+++ b/test/unit/css.js
@@ -926,6 +926,30 @@ QUnit[ jQuery.find.compile && jQuery.fn.toggle ? "test" : "skip" ]( "toggle()",
 	jQuery.fn.hide = oldHide;
 } );
 
+QUnit[ jQuery.find.compile && jQuery.fn.toggle ? "test" : "skip" ]( "detached toggle()", function( assert ) {
+	assert.expect( 6 );
+	var detached = jQuery( "<p><a/><p>" ).find( "*" ).addBack(),
+		hiddenDetached = jQuery( "<p><a/></p>" ).find( "*" ).addBack().css( "display", "none" ),
+		cascadeHiddenDetached = jQuery( "<p><a/></p>" ).find( "*" ).addBack().addClass( "hidden" );
+
+	detached.toggle();
+	detached.appendTo( "#qunit-fixture" );
+	assert.equal( detached[ 0 ].style.display, "none", "detached element" );
+	assert.equal( detached[ 1 ].style.display, "none", "element in detached tree" );
+
+	hiddenDetached.toggle();
+	hiddenDetached.appendTo( "#qunit-fixture" );
+	assert.equal( hiddenDetached[ 0 ].style.display, "", "detached, hidden element" );
+	assert.equal( hiddenDetached[ 1 ].style.display, "", "hidden element in detached tree" );
+
+	cascadeHiddenDetached.toggle();
+	cascadeHiddenDetached.appendTo( "#qunit-fixture" );
+	assert.equal( cascadeHiddenDetached[ 0 ].style.display, "none",
+		"detached, cascade-hidden element" );
+	assert.equal( cascadeHiddenDetached[ 1 ].style.display, "none",
+		"cascade-hidden element in detached tree" );
+} );
+
 QUnit.test( "jQuery.css(elem, 'height') doesn't clear radio buttons (bug #1095)", function( assert ) {
 	assert.expect( 4 );
 
diff --git a/test/unit/effects.js b/test/unit/effects.js
index 0f5eaa415..5f1913575 100644
--- a/test/unit/effects.js
+++ b/test/unit/effects.js
@@ -1547,15 +1547,17 @@ QUnit.test( "animate should set display for disconnected nodes", function( asser
 	assert.expect( 20 );
 
 	var env = this,
-		methods = {
-			toggle: [ 1 ],
-			slideToggle: [],
+		showMethods = {
 			fadeIn: [],
 			fadeTo: [ "fast", 0.5 ],
 			slideDown: [ "fast" ],
 			show: [ 1 ],
 			animate: [ { width: "show" } ]
 		},
+		toggleMethods = {
+			toggle: [ 1 ],
+			slideToggle: []
+		},
 		$divEmpty = jQuery( "<div/>" ),
 		$divTest = jQuery( "<div>test</div>" ),
 		$divNone = jQuery( "<div style='display: none;'/>" ),
@@ -1578,7 +1580,7 @@ QUnit.test( "animate should set display for disconnected nodes", function( asser
 
 	assert.expectJqData( env, $divNone[ 0 ], "olddisplay" );
 
-	jQuery.each( methods, function( name, opt ) {
+	jQuery.each( showMethods, function( name, opt ) {
 		jQuery.fn[ name ].apply( jQuery( "<div/>" ), opt.concat( [ function() {
 			assert.strictEqual( jQuery( this ).css( "display" ), nullParentDisplay,
 				"." + name + " block with null parentNode" );
@@ -1589,6 +1591,17 @@ QUnit.test( "animate should set display for disconnected nodes", function( asser
 				"." + name + " block under fragment" );
 		} ] ) );
 	} );
+	jQuery.each( toggleMethods, function( name, opt ) {
+		jQuery.fn[ name ].apply( jQuery( "<div/>" ), opt.concat( [ function() {
+			assert.strictEqual( jQuery( this ).css( "display" ), "none",
+				"." + name + " block with null parentNode" );
+		} ] ) );
+
+		jQuery.fn[ name ].apply( jQuery( "<div>test</div>" ), opt.concat( [ function() {
+			assert.strictEqual( jQuery( this ).css( "display" ), "none",
+				"." + name + " block under fragment" );
+		} ] ) );
+	} );
 	clock.tick( 400 );
 } );