Browse Source

Function getOptimalPosition should prefer positions inside viewport and not lock on first not optimal position
(because of intersection flipping between viewport and limiter for different position).

Kuba Niegowski 5 years ago
parent
commit
af056645af

+ 93 - 31
packages/ckeditor5-utils/src/dom/position.js

@@ -169,18 +169,57 @@ function getPosition( position, targetRect, elementRect ) {
 // @param {utils/dom/rect~Rect} viewportRect A rect of the viewport.
 // @returns {Array} An array containing the name of the position and it's rect.
 function getBestPosition( positions, targetRect, elementRect, limiterRect, viewportRect ) {
-	let maxLimiterIntersectArea = 0;
-	let maxViewportIntersectArea = 0;
-	let bestPositionRect;
-	let bestPositionName;
+	// This is when element is fully visible.
+	const elementRectArea = elementRect.getArea();
+
+	// Let's calculate intersection areas for positions. It will end early if best match is found.
+	const processedPositions = processPositionsToAreas( positions, targetRect, elementRect, limiterRect, viewportRect );
+
+	// First let's check all positions that fully fit in the viewport.
+	if ( viewportRect ) {
+		const processedPositionsInViewport = processedPositions
+			.filter( ( { viewportIntersectArea } ) => viewportIntersectArea === elementRectArea );
+
+		// Try to find best position from those which fit completely in viewport.
+		const bestPositionData = getBestOfProcessedPositions( processedPositionsInViewport, elementRectArea );
+
+		if ( bestPositionData ) {
+			return bestPositionData;
+		}
+	}
+
+	// Either there is no viewportRect or there is no position that fits completely in the viewport.
+	return getBestOfProcessedPositions( processedPositions, elementRectArea );
+}
+
+// For a given array of positioning functions, calculates intersection areas for them.
+//
+// It will return early with only one item on the list if found position that fully fits in `limiterRect`.
+//
+// @private
+// @param {module:utils/dom/position~Options#positions} positions Functions returning
+// {@link module:utils/dom/position~Position} to be checked, in the order of preference.
+// @param {utils/dom/rect~Rect} targetRect A rect of the {@link module:utils/dom/position~Options#target}.
+// @param {utils/dom/rect~Rect} elementRect A rect of positioned {@link module:utils/dom/position~Options#element}.
+// @param {utils/dom/rect~Rect} limiterRect A rect of the {@link module:utils/dom/position~Options#limiter}.
+// @param {utils/dom/rect~Rect} viewportRect A rect of the viewport.
+// @returns {Array.<Object>} Array of positions with calculated intersection areas. Each item is an object containing:
+//
+//		* {module:engine/src/view/element~Element} element List-like element.
+//		* {String} positionName Name of position.
+//		* {utils/dom/rect~Rect} positionRect Rect of position.
+//		* {Number} limiterIntersectArea Area of intersection of the position with limiter part that is in the viewport.
+//		* {Number} viewportIntersectArea Area of intersection of the position with viewport.
+function processPositionsToAreas( positions, targetRect, elementRect, limiterRect, viewportRect ) {
+	const processedPositions = [];
 
 	// This is when element is fully visible.
 	const elementRectArea = elementRect.getArea();
 
-	positions.some( position => {
+	for ( const position of positions ) {
 		const [ positionName, positionRect ] = getPosition( position, targetRect, elementRect );
-		let limiterIntersectArea;
-		let viewportIntersectArea;
+		let limiterIntersectArea = 0;
+		let viewportIntersectArea = 0;
 
 		if ( limiterRect ) {
 			if ( viewportRect ) {
@@ -191,8 +230,6 @@ function getBestPosition( positions, targetRect, elementRect, limiterRect, viewp
 					// If the limiter is within the viewport, then check the intersection between that part of the
 					// limiter and actual position.
 					limiterIntersectArea = limiterViewportIntersectRect.getIntersectionArea( positionRect );
-				} else {
-					limiterIntersectArea = 0;
 				}
 			} else {
 				limiterIntersectArea = limiterRect.getIntersectionArea( positionRect );
@@ -203,30 +240,55 @@ function getBestPosition( positions, targetRect, elementRect, limiterRect, viewp
 			viewportIntersectArea = viewportRect.getIntersectionArea( positionRect );
 		}
 
-		// The only criterion: intersection with the viewport.
-		if ( viewportRect && !limiterRect ) {
-			if ( viewportIntersectArea > maxViewportIntersectArea ) {
-				setBestPosition();
-			}
-		}
-		// The only criterion: intersection with the limiter.
-		else if ( !viewportRect && limiterRect ) {
-			if ( limiterIntersectArea > maxLimiterIntersectArea ) {
-				setBestPosition();
-			}
-		}
-		// Two criteria: intersection with the viewport and the limiter visible in the viewport.
-		else {
-			if ( viewportIntersectArea > maxViewportIntersectArea && limiterIntersectArea >= maxLimiterIntersectArea ) {
-				setBestPosition();
-			} else if ( viewportIntersectArea >= maxViewportIntersectArea && limiterIntersectArea > maxLimiterIntersectArea ) {
-				setBestPosition();
-			}
+		const processedPosition = {
+			positionName,
+			positionRect,
+			limiterIntersectArea,
+			viewportIntersectArea
+		};
+
+		// If a such position is found that element is fully contained by the limiter then, obviously,
+		// there will be no better one, so finishing.
+		if ( limiterIntersectArea === elementRectArea ) {
+			return [ processedPosition ];
 		}
 
-		function setBestPosition() {
-			maxViewportIntersectArea = viewportIntersectArea;
-			maxLimiterIntersectArea = limiterIntersectArea;
+		processedPositions.push( processedPosition );
+	}
+
+	return processedPositions;
+}
+
+// For a given array of processed position data (with calculated Rects for positions and intersection areas)
+// returns such that provides the best fit of the `elementRect` into the `limiterRect` and `viewportRect`.
+//
+// It will return early if found position that fully fits in `limiterRect`.
+//
+// @private
+// @param {Array.<Object>} Array of positions with calculated intersection areas (in order of preference).
+// Each item is an object containing:
+//
+//		* {module:engine/src/view/element~Element} element List-like element.
+//		* {String} positionName Name of position.
+//		* {utils/dom/rect~Rect} positionRect Rect of position.
+//		* {Number} limiterIntersectArea Area of intersection of the position with limiter part that is in the viewport.
+//		* {Number} viewportIntersectArea Area of intersection of the position with viewport.
+//
+//
+// @param {Number} elementRectArea Area of positioned {@link module:utils/dom/position~Options#element}.
+// @returns {Array|null} An array containing the name of the position and it's rect, or null if not found.
+function getBestOfProcessedPositions( processedPositions, elementRectArea ) {
+	let maxFitFactor = 0;
+	let bestPositionRect;
+	let bestPositionName;
+
+	processedPositions.some( ( { positionName, positionRect, limiterIntersectArea, viewportIntersectArea } ) => {
+		// To maximize both viewport and limiter intersection areas we use distance on viewportIntersectArea
+		// and limiterIntersectArea plane (without sqrt because we are looking for max value).
+		const fitFactor = viewportIntersectArea ** 2 + limiterIntersectArea ** 2;
+
+		if ( fitFactor > maxFitFactor ) {
+			maxFitFactor = fitFactor;
 			bestPositionRect = positionRect;
 			bestPositionName = positionName;
 		}

+ 305 - 54
packages/ckeditor5-utils/tests/dom/position.js

@@ -13,20 +13,42 @@ let element, target, limiter;
 
 //	+--------+-----+
 //	|    E   |  T  |
-//	+--------+-----+
-const attachLeft = ( targetRect, elementRect ) => ( {
+//	|        |-----+
+//  +--------+
+const attachLeftBottom = ( targetRect, elementRect ) => ( {
 	top: targetRect.top,
 	left: targetRect.left - elementRect.width,
-	name: 'left'
+	name: 'left-bottom'
+} );
+
+//	+--------+
+//	|    E   |-----+
+//	|        |  T  |
+//  +--------+-----+
+const attachLeftTop = ( targetRect, elementRect ) => ( {
+	top: targetRect.bottom - elementRect.height,
+	left: targetRect.left - elementRect.width,
+	name: 'left-top'
 } );
 
 //	+-----+--------+
 //	|  T  |    E   |
-//	+-----+--------+
-const attachRight = targetRect => ( {
+//	+-----|        |
+//        +--------+
+const attachRightBottom = targetRect => ( {
 	top: targetRect.top,
-	left: targetRect.left + targetRect.width,
-	name: 'right'
+	left: targetRect.right,
+	name: 'right-bottom'
+} );
+
+//	      +--------+
+//	+-----|    E   |
+//	|  T  |        |
+//  +-----+--------+
+const attachRightTop = ( targetRect, elementRect ) => ( {
+	top: targetRect.bottom - elementRect.height,
+	left: targetRect.right,
+	name: 'right-top'
 } );
 
 //	+-----+
@@ -34,10 +56,21 @@ const attachRight = targetRect => ( {
 //	+-----+--+
 //	|    E   |
 //	+--------+
-const attachBottom = targetRect => ( {
+const attachBottomRight = targetRect => ( {
 	top: targetRect.bottom,
 	left: targetRect.left,
-	name: 'bottom'
+	name: 'bottom-right'
+} );
+
+//	   +-----+
+//	   |  T  |
+//	+--+-----+
+//	|    E   |
+//	+--------+
+const attachBottomLeft = ( targetRect, elementRect ) => ( {
+	top: targetRect.bottom,
+	left: targetRect.right - elementRect.width,
+	name: 'bottom-left'
 } );
 
 //	+--------+
@@ -45,12 +78,34 @@ const attachBottom = targetRect => ( {
 //	+--+-----+
 //	   |  T  |
 //	   +-----+
-const attachTop = ( targetRect, elementRect ) => ( {
+const attachTopLeft = ( targetRect, elementRect ) => ( {
 	top: targetRect.top - elementRect.height,
-	left: targetRect.left - ( elementRect.width - targetRect.width ),
-	name: 'bottom'
+	left: targetRect.right - elementRect.width,
+	name: 'top-left'
 } );
 
+//	+--------+
+//	|   E    |
+//	+-----+--+
+//	|  T  |
+//	+-----+
+const attachTopRight = ( targetRect, elementRect ) => ( {
+	top: targetRect.top - elementRect.height,
+	left: targetRect.left,
+	name: 'top-right'
+} );
+
+const allPositions = [
+	attachLeftBottom,
+	attachLeftTop,
+	attachRightBottom,
+	attachRightTop,
+	attachBottomRight,
+	attachBottomLeft,
+	attachTopLeft,
+	attachTopRight
+];
+
 describe( 'getOptimalPosition()', () => {
 	testUtils.createSinonSandbox();
 
@@ -71,11 +126,11 @@ describe( 'getOptimalPosition()', () => {
 		assertPosition( {
 			element,
 			target: () => target,
-			positions: [ attachLeft ]
+			positions: [ attachLeftBottom ]
 		}, {
 			top: 100,
 			left: 80,
-			name: 'left'
+			name: 'left-bottom'
 		} );
 	} );
 
@@ -85,11 +140,11 @@ describe( 'getOptimalPosition()', () => {
 		assertPosition( {
 			element,
 			target: new Rect( target ),
-			positions: [ attachLeft ]
+			positions: [ attachLeftBottom ]
 		}, {
 			top: 100,
 			left: 80,
-			name: 'left'
+			name: 'left-bottom'
 		} );
 	} );
 
@@ -97,10 +152,10 @@ describe( 'getOptimalPosition()', () => {
 		beforeEach( setElementTargetPlayground );
 
 		it( 'should return coordinates', () => {
-			assertPosition( { element, target, positions: [ attachLeft ] }, {
+			assertPosition( { element, target, positions: [ attachLeftBottom ] }, {
 				top: 100,
 				left: 80,
-				name: 'left'
+				name: 'left-bottom'
 			} );
 		} );
 
@@ -112,10 +167,10 @@ describe( 'getOptimalPosition()', () => {
 				scrollY: 100
 			} );
 
-			assertPosition( { element, target, positions: [ attachLeft ] }, {
+			assertPosition( { element, target, positions: [ attachLeftBottom ] }, {
 				top: 200,
 				left: 180,
-				name: 'left'
+				name: 'left-bottom'
 			} );
 		} );
 
@@ -143,10 +198,10 @@ describe( 'getOptimalPosition()', () => {
 
 				element.parentElement = parent;
 
-				assertPosition( { element, target, positions: [ attachLeft ] }, {
+				assertPosition( { element, target, positions: [ attachLeftBottom ] }, {
 					top: -900,
 					left: -920,
-					name: 'left'
+					name: 'left-bottom'
 				} );
 			} );
 
@@ -175,10 +230,10 @@ describe( 'getOptimalPosition()', () => {
 
 				element.parentElement = parent;
 
-				assertPosition( { element, target, positions: [ attachLeft ] }, {
+				assertPosition( { element, target, positions: [ attachLeftBottom ] }, {
 					top: 160,
 					left: 260,
-					name: 'left'
+					name: 'left-bottom'
 				} );
 			} );
 		} );
@@ -190,22 +245,22 @@ describe( 'getOptimalPosition()', () => {
 		it( 'should return coordinates', () => {
 			assertPosition( {
 				element, target,
-				positions: [ attachLeft, attachRight ]
+				positions: [ attachLeftBottom, attachRightBottom ]
 			}, {
 				top: 100,
 				left: 80,
-				name: 'left'
+				name: 'left-bottom'
 			} );
 		} );
 
 		it( 'should return coordinates (position preference order)', () => {
 			assertPosition( {
 				element, target,
-				positions: [ attachRight, attachLeft ]
+				positions: [ attachRightBottom, attachLeftBottom ]
 			}, {
 				top: 100,
 				left: 110,
-				name: 'right'
+				name: 'right-bottom'
 			} );
 		} );
 	} );
@@ -217,11 +272,11 @@ describe( 'getOptimalPosition()', () => {
 			assertPosition( {
 				element, target,
 				limiter: () => limiter,
-				positions: [ attachLeft, attachRight ]
+				positions: [ attachLeftBottom, attachRightBottom ]
 			}, {
 				top: 100,
 				left: -20,
-				name: 'left'
+				name: 'left-bottom'
 			} );
 		} );
 
@@ -229,33 +284,33 @@ describe( 'getOptimalPosition()', () => {
 			assertPosition( {
 				element, target,
 				limiter: new Rect( limiter ),
-				positions: [ attachLeft, attachRight ]
+				positions: [ attachLeftBottom, attachRightBottom ]
 			}, {
 				top: 100,
 				left: -20,
-				name: 'left'
+				name: 'left-bottom'
 			} );
 		} );
 
 		it( 'should return coordinates (#1)', () => {
 			assertPosition( {
 				element, target, limiter,
-				positions: [ attachLeft, attachRight ]
+				positions: [ attachLeftBottom, attachRightBottom ]
 			}, {
 				top: 100,
 				left: -20,
-				name: 'left'
+				name: 'left-bottom'
 			} );
 		} );
 
 		it( 'should return coordinates (#2)', () => {
 			assertPosition( {
 				element, target, limiter,
-				positions: [ attachRight, attachLeft ]
+				positions: [ attachRightBottom, attachLeftBottom ]
 			}, {
 				top: 100,
 				left: -20,
-				name: 'left'
+				name: 'left-bottom'
 			} );
 		} );
 
@@ -272,11 +327,30 @@ describe( 'getOptimalPosition()', () => {
 
 			assertPosition( {
 				element, target, limiter,
-				positions: [ attachRight, attachLeft ]
+				positions: [ attachRightBottom, attachLeftBottom ]
 			}, {
 				top: 100,
 				left: 10,
-				name: 'right'
+				name: 'right-bottom'
+			} );
+		} );
+
+		it( 'should return first position that completely fits limiter', () => {
+			element = getElement( {
+				top: 0,
+				right: 5,
+				bottom: 5,
+				left: 0,
+				width: 5,
+				height: 5
+			} );
+			assertPosition( {
+				element, target, limiter,
+				positions: [ attachRightBottom, attachLeftBottom ]
+			}, {
+				top: 100,
+				left: -5,
+				name: 'left-bottom'
 			} );
 		} );
 	} );
@@ -287,36 +361,36 @@ describe( 'getOptimalPosition()', () => {
 		it( 'should return coordinates (#1)', () => {
 			assertPosition( {
 				element, target,
-				positions: [ attachLeft, attachRight ],
+				positions: [ attachLeftBottom, attachRightBottom ],
 				fitInViewport: true
 			}, {
 				top: 100,
 				left: 10,
-				name: 'right'
+				name: 'right-bottom'
 			} );
 		} );
 
 		it( 'should return coordinates (#2)', () => {
 			assertPosition( {
 				element, target,
-				positions: [ attachRight, attachLeft ],
+				positions: [ attachRightBottom, attachLeftBottom ],
 				fitInViewport: true
 			}, {
 				top: 100,
 				left: 10,
-				name: 'right'
+				name: 'right-bottom'
 			} );
 		} );
 
 		it( 'should return coordinates (#3)', () => {
 			assertPosition( {
 				element, target,
-				positions: [ attachLeft, attachBottom, attachRight ],
+				positions: [ attachLeftBottom, attachBottomRight, attachRightBottom ],
 				fitInViewport: true
 			}, {
 				top: 110,
 				left: 0,
-				name: 'bottom'
+				name: 'bottom-right'
 			} );
 		} );
 	} );
@@ -327,48 +401,48 @@ describe( 'getOptimalPosition()', () => {
 		it( 'should return coordinates (#1)', () => {
 			assertPosition( {
 				element, target, limiter,
-				positions: [ attachLeft, attachRight ],
+				positions: [ attachLeftBottom, attachRightBottom ],
 				fitInViewport: true
 			}, {
 				top: 100,
 				left: 10,
-				name: 'right'
+				name: 'right-bottom'
 			} );
 		} );
 
 		it( 'should return coordinates (#2)', () => {
 			assertPosition( {
 				element, target, limiter,
-				positions: [ attachRight, attachLeft ],
+				positions: [ attachRightBottom, attachLeftBottom ],
 				fitInViewport: true
 			}, {
 				top: 100,
 				left: 10,
-				name: 'right'
+				name: 'right-bottom'
 			} );
 		} );
 
 		it( 'should return coordinates (#3)', () => {
 			assertPosition( {
 				element, target, limiter,
-				positions: [ attachRight, attachLeft, attachBottom ],
+				positions: [ attachRightBottom, attachLeftBottom, attachBottomRight ],
 				fitInViewport: true
 			}, {
 				top: 110,
 				left: 0,
-				name: 'bottom'
+				name: 'bottom-right'
 			} );
 		} );
 
 		it( 'should return coordinates (#4)', () => {
 			assertPosition( {
 				element, target, limiter,
-				positions: [ attachTop, attachRight ],
+				positions: [ attachTopLeft, attachRightBottom ],
 				fitInViewport: true
 			}, {
 				top: 100,
 				left: 10,
-				name: 'right'
+				name: 'right-bottom'
 			} );
 		} );
 
@@ -402,13 +476,140 @@ describe( 'getOptimalPosition()', () => {
 
 			assertPosition( {
 				element, target, limiter,
-				positions: [ attachRight, attachTop ],
+				positions: [ attachRightBottom, attachTopLeft ],
 				fitInViewport: true
 			}, {
 				top: 100,
 				left: 10,
-				name: 'right'
+				name: 'right-bottom'
+			} );
+		} );
+
+		it( 'should prefer position fully fitting in viewport', () => {
+			target = getElement( {
+				top: 100,
+				right: 35,
+				bottom: 120,
+				left: 15,
+				width: 20,
+				height: 20
+			} );
+			assertPosition( {
+				element, target, limiter,
+				positions: [ attachLeftBottom, attachRightBottom ],
+				fitInViewport: true
+			}, {
+				top: 100,
+				left: 35,
+				name: 'right-bottom'
+			} );
+		} );
+	} );
+
+	describe( 'maximizes intersection area with both limiter and viewport', () => {
+		beforeEach( setElementTargetBigLimiterPlayground );
+
+		it( 'should prefer position with bigger intersection (#1)', () => {
+			target = getElement( {
+				top: 90,
+				right: -10,
+				bottom: 110,
+				left: -30,
+				width: 20,
+				height: 20
+			} );
+			assertPositionName( {
+				element, target, limiter,
+				positions: allPositions,
+				fitInViewport: true
+			}, 'right-bottom' );
+		} );
+
+		it( 'should prefer position with bigger intersection (#2)', () => {
+			target = getElement( {
+				top: 290,
+				right: -10,
+				bottom: 310,
+				left: -30,
+				width: 20,
+				height: 20
+			} );
+			assertPositionName( {
+				element, target, limiter,
+				positions: allPositions,
+				fitInViewport: true
+			}, 'right-top' );
+		} );
+
+		it( 'should prefer position with bigger intersection (#3)', () => {
+			target = getElement( {
+				top: 90,
+				right: 130,
+				bottom: 110,
+				left: 110,
+				width: 20,
+				height: 20
+			} );
+			assertPositionName( {
+				element, target, limiter,
+				positions: allPositions,
+				fitInViewport: true
+			}, 'left-bottom' );
+		} );
+
+		it( 'should prefer position with bigger intersection (#4)', () => {
+			target = getElement( {
+				top: 290,
+				right: 130,
+				bottom: 310,
+				left: 110,
+				width: 20,
+				height: 20
+			} );
+			assertPositionName( {
+				element, target, limiter,
+				positions: allPositions,
+				fitInViewport: true
+			}, 'left-top' );
+		} );
+
+		it( 'should not stick to first biggest intersection in one area', () => {
+			// First position intersects more with limiter but little with viewport,
+			// second position intersects less with limiter but more with viewport and it should not be ignored.
+			//
+			// Target is outside viewport to force checking all positions, not only those completely fitting in viewport.
+			limiter = getElement( {
+				top: -100,
+				right: 100,
+				bottom: 100,
+				left: -100,
+				width: 200,
+				height: 200
+			} );
+			target = getElement( {
+				top: -30,
+				right: 80,
+				bottom: -10,
+				left: 60,
+				width: 20,
+				height: 20
+			} );
+			element = getElement( {
+				top: 0,
+				right: 200,
+				bottom: 200,
+				left: 0,
+				width: 200,
+				height: 200
 			} );
+			assertPositionName( {
+				element, target, limiter,
+				positions: [
+					attachLeftBottom,
+					attachRightBottom
+				],
+				fitInViewport: true
+			}, 'right-bottom' );
 		} );
 	} );
 } );
@@ -419,6 +620,12 @@ function assertPosition( options, expected ) {
 	expect( position ).to.deep.equal( expected );
 }
 
+function assertPositionName( options, expected ) {
+	const position = getOptimalPosition( options );
+
+	expect( position.name ).to.equal( expected );
+}
+
 // Returns a synthetic element.
 //
 // @private
@@ -432,6 +639,9 @@ function getElement( properties = {}, styles = {} ) {
 		ownerDocument: document
 	};
 
+	expect( properties.right - properties.left ).to.equal( properties.width, 'getElement incorrect horizontal values' );
+	expect( properties.bottom - properties.top ).to.equal( properties.height, 'getElement incorrect vertical values' );
+
 	Object.assign( element, properties );
 
 	if ( !styles.borderLeftWidth ) {
@@ -540,3 +750,44 @@ function setElementTargetLimiterPlayground() {
 		height: 10
 	} );
 }
+
+//
+//
+//     ^                +-----------[ Viewport ]----------------------
+//     |                |
+//   100px              |
+//     |   <--------- 200px ------->
+//	   |               <-- 100px -->
+//	   V                |
+//	       +------------+---------+     ^
+//	       |            |         |     |
+//	       |            |         |     |
+//	       |            |         |     |
+//	       |            |         |    200px
+//	       |            |         |     |
+//	       |            |         |     |
+//	       |            |         |     |
+//         +------[ Limiter ]-----+     V
+//                      |
+//                      |
+//
+//
+function setElementTargetBigLimiterPlayground() {
+	element = getElement( {
+		top: 0,
+		right: 50,
+		bottom: 50,
+		left: 0,
+		width: 50,
+		height: 50
+	} );
+
+	limiter = getElement( {
+		top: 100,
+		right: 100,
+		bottom: 300,
+		left: -100,
+		width: 200,
+		height: 200
+	} );
+}