Merge pull request #141 from ckeditor/t/139a

Fix: getOptimalPosition() utility does not work when the parent element has a scroll. Closes #139.
ckeditor · Mar 22, 2017 · b878949 · b878949
2 parents d4f016d + 64ba251
commit b878949
Show file tree

Hide file tree

Showing 5 changed files with 249 additions and 65 deletions.
diff --git a/src/dom/position.js b/src/dom/position.js
@@ -99,14 +99,32 @@ export function getOptimalPosition( { element, target, positions, limiter, fitIn
 
 	let { left, top } = getAbsoluteRectCoordinates( bestPosition );
 
-	// (#126) If there's some positioned ancestor of the panel, then its rect must be taken into
-	// consideration. `Rect` is always relative to the viewport while `position: absolute` works
-	// with respect to that positioned ancestor.
 	if ( positionedElementAncestor ) {
 		const ancestorPosition = getAbsoluteRectCoordinates( new Rect( positionedElementAncestor ) );
+		const ancestorComputedStyles = global.window.getComputedStyle( positionedElementAncestor );
 
+		// (https://github.com/ckeditor/ckeditor5-ui-default/issues/126)
+		// If there's some positioned ancestor of the panel, then its `Rect` must be taken into
+		// consideration. `Rect` is always relative to the viewport while `position: absolute` works
+		// with respect to that positioned ancestor.
 		left -= ancestorPosition.left;
 		top -= ancestorPosition.top;
+
+		// (https://github.com/ckeditor/ckeditor5-utils/issues/139)
+		// If there's some positioned ancestor of the panel, not only its position must be taken into
+		// consideration (see above) but also its internal scrolls. Scroll have an impact here because `Rect`
+		// is relative to the viewport (it doesn't care about scrolling), while `position: absolute`
+		// must compensate that scrolling.
+		left += positionedElementAncestor.scrollLeft;
+		top += positionedElementAncestor.scrollTop;
+
+		// (https://github.com/ckeditor/ckeditor5-utils/issues/139)
+		// If there's some positioned ancestor of the panel, then its `Rect` includes its CSS `borderWidth`
+		// while `position: absolute` positioning does not consider it.
+		// E.g. `{ position: absolute, top: 0, left: 0 }` means upper left corner of the element,
+		// not upper-left corner of its border.
+		left -= parseInt( ancestorComputedStyles.borderLeftWidth, 10 );
+		top -= parseInt( ancestorComputedStyles.borderTopWidth, 10 );
 	}
 
 	return { left, top, name };

diff --git a/tests/dom/position.js b/tests/dom/position.js
@@ -3,26 +3,19 @@
  * For licensing, see LICENSE.md.
  */
 
-/* global document, window */
-
 import global from '../../src/dom/global';
 import { getOptimalPosition } from '../../src/dom/position';
-import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
-
-testUtils.createSinonSandbox();
 
-let element, target, limiter, windowStub;
+let element, target, limiter;
 
-describe( 'getOptimalPosition', () => {
+describe( 'getOptimalPosition()', () => {
 	beforeEach( () => {
-		windowStub = {
+		stubWindow( {
 			innerWidth: 10000,
 			innerHeight: 10000,
 			scrollX: 0,
 			scrollY: 0
-		};
-
-		testUtils.sinon.stub( global, 'window', windowStub );
+		} );
 	} );
 
 	describe( 'for single position', () => {
@@ -37,7 +30,7 @@ describe( 'getOptimalPosition', () => {
 		} );
 
 		it( 'should return coordinates (window scroll)', () => {
-			Object.assign( windowStub, {
+			stubWindow( {
 				innerWidth: 10000,
 				innerHeight: 10000,
 				scrollX: 100,
@@ -51,41 +44,67 @@ describe( 'getOptimalPosition', () => {
 			} );
 		} );
 
-		it( 'should return coordinates (positioned element parent)', () => {
-			const positionedParent = document.createElement( 'div' );
-
-			Object.assign( windowStub, {
-				innerWidth: 10000,
-				innerHeight: 10000,
-				scrollX: 1000,
-				scrollY: 1000,
-				getComputedStyle: ( el ) => {
-					return window.getComputedStyle( el );
-				}
-			} );
-
-			Object.assign( positionedParent.style, {
-				position: 'absolute',
-				top: '1000px',
-				left: '1000px'
+		describe( 'positioned element parent', () => {
+			let parent;
+
+			it( 'should return coordinates', () => {
+				stubWindow( {
+					innerWidth: 10000,
+					innerHeight: 10000,
+					scrollX: 1000,
+					scrollY: 1000
+				} );
+
+				parent = getElement( {
+					top: 1000,
+					right: 1010,
+					bottom: 1010,
+					left: 1000,
+					width: 10,
+					height: 10
+				}, {
+					position: 'absolute'
+				} );
+
+				element.parentElement = parent;
+
+				assertPosition( { element, target, positions: [ attachLeft ] }, {
+					top: -900,
+					left: -920,
+					name: 'left'
+				} );
 			} );
 
-			document.body.appendChild( positionedParent );
-			positionedParent.appendChild( element );
-
-			stubElementRect( positionedParent, {
-				top: 1000,
-				right: 1010,
-				bottom: 1010,
-				left: 1000,
-				width: 10,
-				height: 10
-			} );
-
-			assertPosition( { element, target, positions: [ attachLeft ] }, {
-				top: -900,
-				left: -920,
-				name: 'left'
+			it( 'should return coordinates (scroll and border)', () => {
+				stubWindow( {
+					innerWidth: 10000,
+					innerHeight: 10000,
+					scrollX: 1000,
+					scrollY: 1000
+				} );
+
+				parent = getElement( {
+					top: 0,
+					right: 10,
+					bottom: 10,
+					left: 0,
+					width: 10,
+					height: 10,
+					scrollTop: 100,
+					scrollLeft: 200
+				}, {
+					position: 'absolute',
+					borderLeftWidth: '20px',
+					borderTopWidth: '40px',
+				} );
+
+				element.parentElement = parent;
+
+				assertPosition( { element, target, positions: [ attachLeft ] }, {
+					top: 160,
+					left: 260,
+					name: 'left'
+				} );
 			} );
 		} );
 	} );
@@ -252,7 +271,7 @@ describe( 'getOptimalPosition', () => {
 		} );
 
 		it( 'should return the very first coordinates if limiter does not fit into the viewport', () => {
-			stubElementRect( limiter, {
+			limiter = getElement( {
 				top: -100,
 				right: -80,
 				bottom: -80,
@@ -320,12 +339,41 @@ const attachTop = ( targetRect, elementRect ) => ( {
 	name: 'bottom'
 } );
 
-function stubElementRect( element, rect ) {
-	if ( element.getBoundingClientRect.restore ) {
-		element.getBoundingClientRect.restore();
+// Returns a synthetic element.
+//
+// @private
+// @param {Object} properties A set of properties for the element.
+// @param {Object} styles A set of styles in `window.getComputedStyle()` format.
+function getElement( properties = {}, styles = {} ) {
+	const element = {
+		tagName: 'div',
+		scrollLeft: 0,
+		scrollTop: 0
+	};
+
+	Object.assign( element, properties );
+
+	if ( !styles.borderLeftWidth ) {
+		styles.borderLeftWidth = '0px';
+	}
+
+	if ( !styles.borderTopWidth ) {
+		styles.borderTopWidth = '0px';
 	}
 
-	testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( rect );
+	global.window.getComputedStyle.withArgs( element ).returns( styles );
+
+	return element;
+}
+
+// Stubs the window.
+//
+// @private
+// @param {Object} properties A set of properties the window should have.
+function stubWindow( properties ) {
+	global.window = Object.assign( {
+		getComputedStyle: sinon.stub()
+	}, properties );
 }
 
 //        <-- 100px ->
@@ -343,10 +391,7 @@ function stubElementRect( element, rect ) {
 //	      |
 //
 function setElementTargetPlayground() {
-	element = document.createElement( 'div' );
-	target = document.createElement( 'div' );
-
-	stubElementRect( element, {
+	element = getElement( {
 		top: 0,
 		right: 20,
 		bottom: 20,
@@ -355,7 +400,7 @@ function setElementTargetPlayground() {
 		height: 20
 	} );
 
-	stubElementRect( target, {
+	target = getElement( {
 		top: 100,
 		right: 110,
 		bottom: 110,
@@ -387,11 +432,7 @@ function setElementTargetPlayground() {
 //
 //
 function setElementTargetLimiterPlayground() {
-	element = document.createElement( 'div' );
-	target = document.createElement( 'div' );
-	limiter = document.createElement( 'div' );
-
-	stubElementRect( element, {
+	element = getElement( {
 		top: 0,
 		right: 20,
 		bottom: 20,
@@ -400,7 +441,7 @@ function setElementTargetLimiterPlayground() {
 		height: 20
 	} );
 
-	stubElementRect( limiter, {
+	limiter = getElement( {
 		top: 100,
 		right: 10,
 		bottom: 120,
@@ -409,7 +450,7 @@ function setElementTargetLimiterPlayground() {
 		height: 20
 	} );
 
-	stubElementRect( target, {
+	target = getElement( {
 		top: 100,
 		right: 10,
 		bottom: 110,

diff --git a/tests/manual/position/position.html b/tests/manual/position/position.html
@@ -0,0 +1,56 @@
+<div class="test-box" style="height: 200px; width: 200px">
+	<div class="test-box" style="height: 400px; width: 400px">
+		<div class="test-box" style="height: 800px; width: 800px">
+			<div class="target"></div>
+			<div class="source source-se"></div>
+		</div>
+		<div class="source source-sw"></div>
+	</div>
+	<div class="source source-ne"></div>
+</div>
+<div class="source source-nw"></div>
+
+<style>
+	body {
+		padding-top: 2000px;
+	}
+
+	.test-box {
+		padding: 15px;
+		border: 25px solid #000;
+		overflow: scroll;
+		position: relative;
+	}
+
+	.source {
+		width: 40px;
+		height: 40px;
+		background: red;
+		position: absolute;
+	}
+
+	.source-nw {
+		background: cyan;
+	}
+
+	.source-ne {
+		background: magenta;
+	}
+
+	.source-sw {
+		background: yellow;
+	}
+
+	.source-se {
+		background: black;
+	}
+
+	.target {
+		width: 80px;
+		height: 80px;
+		background: blue;
+		position: absolute;
+		bottom: 0;
+		right: 0;
+	}
+</style>