Position: Fix scrollbar calculcation to correctly take overflow:scrol…

…l into account, along with unit tests
jquery · Apr 24, 2012 · 252352e · 252352e
1 parent 1a0f2e4
commit 252352e
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 7 deletions.
diff --git a/tests/unit/position/position.html b/tests/unit/position/position.html
@@ -43,7 +43,7 @@ <h2 id="qunit-userAgent"></h2>
 	<div id="parent" style="position: absolute; width: 6px; height: 6px; top: 4px; left: 4px; line-height: 6px;"></div>
 	<div id="within" style="position: absolute; width: 12px; height: 12px; top: 2px; left: 0px;"></div>
 
-	<div style="position: absolute; top: 0px; left: 0px">
+	<div id="scrollx" style="position: absolute; top: 0px; left: 0px">
 		<div id="elx" style="position: absolute; width: 10px; height: 10px; line-height: 10px;"></div>
 		<div id="parentx" style="position: absolute; width: 20px; height: 20px; top: 40px; left: 40px;"></div>
 	</div>

diff --git a/tests/unit/position/position_core.js b/tests/unit/position/position_core.js
@@ -591,6 +591,67 @@ test( "within", function() {
 	}, "flipfit - left top" );
 });
 
+test( "with scrollbars", function() {
+	expect( 4 );
+
+	$( "#scrollx" ).css({
+		width: 100,
+		height: 100,
+		left: 0,
+		top: 0
+	});
+
+	collisionTest({
+		of: "#scrollx",
+		collision: "fit",
+		within: "#scrollx"
+	}, {
+		top: 90,
+		left: 90
+	}, "visible" );
+
+	$( "#scrollx" ).css({
+		overflow: "scroll"
+	});
+
+	var scrollbarInfo = $.position.getScrollInfo( $.position.getWithinInfo( $( "#scrollx" ) ) );
+
+	collisionTest({
+		of: "#scrollx",
+		collision: "fit",
+		within: "#scrollx"
+	}, {
+		top: 90 - scrollbarInfo.height,
+		left: 90 - scrollbarInfo.width
+	}, "scroll" );
+
+	$( "#scrollx" ).css({
+		overflow: "auto"
+	});
+
+	collisionTest({
+		of: "#scrollx",
+		collision: "fit",
+		within: "#scrollx"
+	}, {
+		top: 90,
+		left: 90
+	}, "auto, no scroll" );
+
+	$( "#scrollx" ).css({
+		overflow: "auto"
+	}).append( $("<div>").height(300).width(300) );
+
+	collisionTest({
+		of: "#scrollx",
+		collision: "fit",
+		within: "#scrollx"
+	}, {
+		top: 90 - scrollbarInfo.height,
+		left: 90 - scrollbarInfo.width
+	}, "auto, with scroll" );
+});
+
 test( "fractions", function() {
 	expect( 1 );
 

diff --git a/tests/visual/position/position_within.html b/tests/visual/position/position_within.html
@@ -97,7 +97,7 @@
                     collision: $( "#collision_horizontal" ).val() + " " + $( "#collision_vertical" ).val()
                 });
             }
-            $( ".demo" ).append("<div style='width:5000px;height:5000px;' />").css("overflow","auto");
+            $( ".demo" ).css("overflow","scroll");
 
             $( ".positionable" ).css( "opacity", 0.5 );
 

diff --git a/ui/jquery.ui.position.js b/ui/jquery.ui.position.js
@@ -58,12 +58,13 @@ $.position = {
 	getScrollInfo: function( within ) {
 		var overflowX = within.isWindow ? "" : within.element.css( "overflow-x" ),
 			overflowY = within.isWindow ? "" : within.element.css( "overflow-y" ),
-			scrollbarWidth = overflowX === "auto" || overflowX === "scroll" ? $.position.scrollbarWidth() : 0,
-			scrollbarHeight = overflowY === "auto" || overflowY === "scroll" ? $.position.scrollbarWidth() : 0;
-
+			hasOverflowX = overflowX === "scroll" ||
+				( overflowX === "auto" && within.width < within.element[0].scrollWidth ),
+			hasOverflowY = overflowY === "scroll" ||
+				( overflowY === "auto" && within.height < within.element[0].scrollHeight );
 		return {
-			height: within.height < within.element[0].scrollHeight ? scrollbarHeight : 0,
-			width: within.width < within.element[0].scrollWidth ? scrollbarWidth : 0
+			width: hasOverflowX ? $.position.scrollbarWidth() : 0,
+			height: hasOverflowY ? $.position.scrollbarWidth() : 0
 		};
 	},
 	getWithinInfo: function( element ) {