Bladeren bron

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

Fix: getOptimalPosition() utility does not work when the parent element has a scroll. Closes #139.
Oskar Wróbel 8 jaren geleden
bovenliggende
commit
fbb69f2581

+ 21 - 3
packages/ckeditor5-utils/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 };

+ 103 - 62
packages/ckeditor5-utils/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,

+ 56 - 0
packages/ckeditor5-utils/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>

+ 59 - 0
packages/ckeditor5-utils/tests/manual/position/position.js

@@ -0,0 +1,59 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* global document, setTimeout */
+
+import { getOptimalPosition } from '@ckeditor/ckeditor5-utils/src/dom/position';
+
+const boxes = document.querySelectorAll( '.test-box' );
+const sources = document.querySelectorAll( '.source' );
+const target = document.querySelector( '.target' );
+
+target.scrollIntoView();
+
+const positions = {
+	nw: ( targetRect ) => ( {
+		top: targetRect.top,
+		left: targetRect.left,
+		name: 'nw'
+	} ),
+	ne: ( targetRect, sourceRect ) => ( {
+		top: targetRect.top,
+		left: targetRect.right - sourceRect.width,
+		name: 'ne'
+	} ),
+	sw: ( targetRect, sourceRect ) => ( {
+		top: targetRect.bottom - sourceRect.height,
+		left: targetRect.left,
+		name: 'sw'
+	} ),
+	se: ( targetRect, sourceRect ) => ( {
+		top: targetRect.bottom - sourceRect.height,
+		left: targetRect.right - sourceRect.width,
+		name: 'se'
+	} )
+};
+
+for ( let box of boxes ) {
+	box.scrollTop = box.scrollHeight;
+	box.scrollLeft = box.scrollWidth;
+}
+
+// Wait for the scroll to stabilize.
+setTimeout( () => {
+	for ( let source of sources ) {
+		const position = getOptimalPosition( {
+			element: source,
+			target: target,
+			positions: [
+				positions[ source.className.split( '-' )[ 1 ] ]
+			],
+			limiter: document.body
+		} );
+
+		source.style.top = position.top + 'px';
+		source.style.left = position.left + 'px';
+	}
+}, 100 );

+ 10 - 0
packages/ckeditor5-utils/tests/manual/position/position.md

@@ -0,0 +1,10 @@
+## getOptimalPosition() utility
+
+**Expected**: At the bottom of the page, in a nested, scrolled container hierarchy, a **perfect** square divided equally into 4 sub–squares should be visible.
+
+The colors of the sub–squares should be as follows:
+
+* Upper-left corner: <span style="color: cyan">cyan</span>,
+* Upper-right corner: <span style="color: magenta">magenta</span>,
+* Bottom-left corner: <span style="color: yellow; background: black;">yellow</span>,
+* Bottom-right corner: <span style="color: black">black</span>.