Fix: The getOptimalPosition utility should consider limiter ancesto…

…rs with CSS overflow. Closes #148. T/148: getOptimalPosition utility should consider limiter ancestors with CSS overflow. Closes #148.
ckeditor · Apr 19, 2017 · 6bf1741 · 6bf1741
2 parents 16995e2 + 1123afd
commit 6bf1741
Show file tree

Hide file tree

Showing 10 changed files with 385 additions and 7 deletions.
diff --git a/src/dom/position.js b/src/dom/position.js
@@ -87,7 +87,7 @@ export function getOptimalPosition( { element, target, positions, limiter, fitIn
 	if ( !limiter && !fitInViewport ) {
 		[ name, bestPosition ] = getPosition( positions[ 0 ], targetRect, elementRect );
 	} else {
-		const limiterRect = limiter && new Rect( limiter );
+		const limiterRect = limiter && new Rect( limiter ).getVisible();
 		const viewportRect = fitInViewport && Rect.getViewportRect();
 
 		[ name, bestPosition ] =

diff --git a/src/dom/rect.js b/src/dom/rect.js
@@ -37,14 +37,28 @@ export default class Rect {
 	 *		// Rect out of a ClientRect.
 	 *		const rectE = new Rect( document.body.getClientRects().item( 0 ) );
 	 *
-	 * @param {HTMLElement|Range|ClientRect|module:utils/dom/rect~Rect|Object} obj A source object to create the rect.
+	 * @param {HTMLElement|Range|ClientRect|module:utils/dom/rect~Rect|Object} source A source object to create the rect.
 	 */
-	constructor( obj ) {
-		if ( isElement( obj ) || isRange( obj ) ) {
-			obj = obj.getBoundingClientRect();
+	constructor( source ) {
+		/**
+		 * The object this rect is for.
+		 *
+		 * @protected
+		 * @readonly
+		 * @member {HTMLElement|Range|ClientRect|module:utils/dom/rect~Rect|Object} #_source
+		 */
+		Object.defineProperty( this, '_source', {
+			// source._source if already the Rect instance
+			value: source._source || source,
+			writable: false,
+			enumerable: false
+		} );
+
+		if ( isElement( source ) || isRange( source ) ) {
+			source = source.getBoundingClientRect();
 		}
 
-		rectProperties.forEach( p => this[ p ] = obj[ p ] );
+		rectProperties.forEach( p => this[ p ] = source[ p ] );
 
 		/**
 		 * The "top" value of the rect.
@@ -179,6 +193,46 @@ export default class Rect {
 		return this.width * this.height;
 	}
 
+	/**
+	 * Returns a new rect, a part of the original rect, which is actually visible to the user,
+	 * e.g. an original rect cropped by parent element rects which have `overflow` set in CSS
+	 * other than `"visible"`.
+	 *
+	 * If there's no such visible rect, which is when the rect is limited by one or many of
+	 * the ancestors, `null` is returned.
+	 *
+	 * @returns {module:utils/dom/rect~Rect|null} A visible rect instance or `null`, if there's none.
+	 */
+	getVisible() {
+		const source = this._source;
+		let visibleRect = this.clone();
+
+		// There's no ancestor to crop <body> with the overflow.
+		if ( source != global.document.body ) {
+			let parent = source.parentNode || source.commonAncestorContainer;
+
+			// Check the ancestors all the way up to the <body>.
+			while ( parent && parent != global.document.body ) {
+				const parentRect = new Rect( parent );
+				const intersectionRect = visibleRect.getIntersection( parentRect );
+
+				if ( intersectionRect ) {
+					if ( intersectionRect.getArea() < visibleRect.getArea() ) {
+						// Reduce the visible rect to the intersection.
+						visibleRect = intersectionRect;
+					}
+				} else {
+					// There's no intersection, the rect is completely invisible.
+					return null;
+				}
+
+				parent = parent.parentNode;
+			}
+		}
+
+		return visibleRect;
+	}
+
 	/**
 	 * Returns a rect of the web browser viewport.
 	 *

diff --git a/tests/dom/position.js b/tests/dom/position.js
@@ -159,6 +159,27 @@ describe( 'getOptimalPosition()', () => {
 				name: 'left'
 			} );
 		} );
+
+		// https://github.com/ckeditor/ckeditor5-utils/issues/148
+		it( 'should return coordinates (#3)', () => {
+			limiter.parentNode = getElement( {
+				top: 100,
+				left: 0,
+				bottom: 110,
+				right: 10,
+				width: 10,
+				height: 10
+			} );
+
+			assertPosition( {
+				element, target, limiter,
+				positions: [ attachRight, attachLeft ]
+			}, {
+				top: 100,
+				left: 10,
+				name: 'right'
+			} );
+		} );
 	} );
 
 	describe( 'with fitInViewport on', () => {

diff --git a/tests/dom/rect.js b/tests/dom/rect.js
@@ -26,6 +26,13 @@ describe( 'Rect', () => {
 	} );
 
 	describe( 'constructor()', () => {
+		it( 'should store passed object in #_source property', () => {
+			const obj = {};
+			const rect = new Rect( obj );
+
+			expect( rect._source ).to.equal( obj );
+		} );
+
 		it( 'should accept HTMLElement', () => {
 			const element = document.createElement( 'div' );
 
@@ -97,6 +104,14 @@ describe( 'Rect', () => {
 			expect( clone ).not.equal( rect );
 			assertRect( clone, rect );
 		} );
+
+		it( 'should preserve #_source', () => {
+			const rect = new Rect( geometry );
+			const clone = rect.clone();
+
+			expect( clone._source ).to.equal( rect._source );
+			assertRect( clone, rect );
+		} );
 	} );
 
 	describe( 'moveTo()', () => {
@@ -320,6 +335,204 @@ describe( 'Rect', () => {
 		} );
 	} );
 
+	describe( 'getVisible()', () => {
+		let element, range, ancestorA, ancestorB;
+
+		beforeEach( () => {
+			element = document.createElement( 'div' );
+			range = document.createRange();
+			ancestorA = document.createElement( 'div' );
+			ancestorB = document.createElement( 'div' );
+
+			ancestorA.append( element );
+			document.body.appendChild( ancestorA );
+		} );
+
+		afterEach( () => {
+			ancestorA.remove();
+			ancestorB.remove();
+		} );
+
+		it( 'should return a new rect', () => {
+			const rect = new Rect( {} );
+			const visible = rect.getVisible();
+
+			expect( visible ).to.not.equal( rect );
+		} );
+
+		it( 'should not fail when the rect is for document#body', () => {
+			testUtils.sinon.stub( document.body, 'getBoundingClientRect' ).returns( {
+				top: 0,
+				right: 100,
+				bottom: 100,
+				left: 0,
+				width: 100,
+				height: 100
+			} );
+
+			assertRect( new Rect( document.body ).getVisible(), {
+				top: 0,
+				right: 100,
+				bottom: 100,
+				left: 0,
+				width: 100,
+				height: 100
+			} );
+		} );
+
+		it( 'should return the visible rect (HTMLElement), partially cropped', () => {
+			testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( {
+				top: 0,
+				right: 100,
+				bottom: 100,
+				left: 0,
+				width: 100,
+				height: 100
+			} );
+
+			testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
+				top: 50,
+				right: 150,
+				bottom: 150,
+				left: 50,
+				width: 100,
+				height: 100
+			} );
+
+			assertRect( new Rect( element ).getVisible(), {
+				top: 50,
+				right: 100,
+				bottom: 100,
+				left: 50,
+				width: 50,
+				height: 50
+			} );
+		} );
+
+		it( 'should return the visible rect (HTMLElement), fully visible', () => {
+			testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( {
+				top: 0,
+				right: 100,
+				bottom: 100,
+				left: 0,
+				width: 100,
+				height: 100
+			} );
+
+			testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
+				top: 0,
+				right: 150,
+				bottom: 150,
+				left: 0,
+				width: 150,
+				height: 150
+			} );
+
+			assertRect( new Rect( element ).getVisible(), {
+				top: 0,
+				right: 100,
+				bottom: 100,
+				left: 0,
+				width: 100,
+				height: 100
+			} );
+		} );
+
+		it( 'should return the visible rect (HTMLElement), partially cropped, deep ancestor overflow', () => {
+			ancestorB.append( ancestorA );
+			document.body.appendChild( ancestorB );
+
+			testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( {
+				top: 0,
+				right: 100,
+				bottom: 100,
+				left: 0,
+				width: 100,
+				height: 100
+			} );
+
+			testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
+				top: 50,
+				right: 100,
+				bottom: 100,
+				left: 0,
+				width: 50,
+				height: 50
+			} );
+
+			testUtils.sinon.stub( ancestorB, 'getBoundingClientRect' ).returns( {
+				top: 0,
+				right: 150,
+				bottom: 100,
+				left: 50,
+				width: 100,
+				height: 100
+			} );
+
+			assertRect( new Rect( element ).getVisible(), {
+				top: 50,
+				right: 100,
+				bottom: 100,
+				left: 50,
+				width: 50,
+				height: 50
+			} );
+		} );
+
+		it( 'should return the visible rect (Range), partially cropped', () => {
+			range.setStart( ancestorA, 0 );
+
+			testUtils.sinon.stub( range, 'getBoundingClientRect' ).returns( {
+				top: 0,
+				right: 100,
+				bottom: 100,
+				left: 0,
+				width: 100,
+				height: 100
+			} );
+
+			testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
+				top: 50,
+				right: 150,
+				bottom: 150,
+				left: 50,
+				width: 100,
+				height: 100
+			} );
+
+			assertRect( new Rect( range ).getVisible(), {
+				top: 50,
+				right: 100,
+				bottom: 100,
+				left: 50,
+				width: 50,
+				height: 50
+			} );
+		} );
+
+		it( 'should return null if there\'s no visible rect', () => {
+			testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( {
+				top: 0,
+				right: 100,
+				bottom: 100,
+				left: 0,
+				width: 100,
+				height: 100
+			} );
+
+			testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
+				top: 150,
+				right: 200,
+				bottom: 200,
+				left: 150,
+				width: 50,
+				height: 50
+			} );
+
+			expect( new Rect( element ).getVisible() ).to.equal( null );
+		} );
+	} );
+
 	describe( 'getViewportRect()', () => {
 		it( 'should reaturn a rect', () => {
 			expect( Rect.getViewportRect() ).to.be.instanceOf( Rect );

diff --git a/tests/manual/focustracker.html → tests/manual/focustracker/focustracker.html b/tests/manual/focustracker.html → tests/manual/focustracker/focustracker.html
diff --git a/tests/manual/focustracker.js → tests/manual/focustracker/focustracker.js b/tests/manual/focustracker.js → tests/manual/focustracker/focustracker.js
@@ -5,7 +5,7 @@
 
 /* global document */
 
-import FocusTracker from '../../src/focustracker';
+import FocusTracker from '../../../src/focustracker';
 
 const focusTracker = new FocusTracker();
 const counters = document.querySelectorAll( '.status b' );

diff --git a/tests/manual/focustracker.md → tests/manual/focustracker/focustracker.md b/tests/manual/focustracker.md → tests/manual/focustracker/focustracker.md
diff --git a/tests/manual/tickets/148/1.html b/tests/manual/tickets/148/1.html
@@ -0,0 +1,38 @@
+<div class="wrapper">
+	<div class="limiter">
+		<div class="target"></div>
+	</div>
+
+	<div class="source"></div>
+</div>
+
+
+<style>
+	.wrapper {
+		overflow: scroll;
+		outline: 1px solid #000;
+		height: 400px;
+		position: relative;
+		padding: 20px;
+		margin-top: 100px;
+	}
+
+	.limiter {
+		height: 500px;
+		overflow: hidden;
+	}
+
+	.target {
+		height: 150px;
+		background: #ccc;
+		width: 50px;
+		margin: 170px auto 0;
+	}
+
+	.source {
+		height: 50px;
+		width: 50px;
+		background: red;
+		position: absolute;
+	}
+</style>