Jelajahi Sumber

Merge pull request #332 from ckeditor/i/6573

Other: The `getPositionedAncestor()` helper should use `offsetParent` instead of `getComputedStyle()` for performance reasons. Closes ckeditor/ckeditor5#6573.

MINOR BREAKING CHANGE: The `getPositionedAncestor()` helper will no longer return the passed element when it is positioned.
Piotrek Koszuliński 5 tahun lalu
induk
melakukan
f8a9363e8c

+ 6 - 6
packages/ckeditor5-utils/src/dom/getpositionedancestor.js

@@ -16,13 +16,13 @@ import global from './global';
  * @returns {HTMLElement|null}
  */
 export default function getPositionedAncestor( element ) {
-	while ( element && element.tagName.toLowerCase() != 'html' ) {
-		if ( global.window.getComputedStyle( element ).position != 'static' ) {
-			return element;
-		}
+	if ( !element || !element.parentNode ) {
+		return null;
+	}
 
-		element = element.parentElement;
+	if ( element.offsetParent === global.document.body ) {
+		return null;
 	}
 
-	return null;
+	return element.offsetParent;
 }

+ 1 - 1
packages/ckeditor5-utils/src/dom/position.js

@@ -90,7 +90,7 @@ export function getOptimalPosition( { element, target, positions, limiter, fitIn
 		limiter = limiter();
 	}
 
-	const positionedElementAncestor = getPositionedAncestor( element.parentElement );
+	const positionedElementAncestor = getPositionedAncestor( element );
 	const elementRect = new Rect( element );
 	const targetRect = new Rect( target );
 

+ 3 - 3
packages/ckeditor5-utils/tests/dom/getpositionedancestor.js

@@ -24,14 +24,14 @@ describe( 'getPositionedAncestor', () => {
 		expect( getPositionedAncestor() ).to.be.null;
 	} );
 
-	it( 'should return null when there is no parent', () => {
+	it( 'should return null when there is no positioned ancestor', () => {
 		expect( getPositionedAncestor( element ) ).to.be.null;
 	} );
 
-	it( 'should consider passed element', () => {
+	it( 'should not consider the passed element', () => {
 		element.style.position = 'relative';
 
-		expect( getPositionedAncestor( element ) ).to.equal( element );
+		expect( getPositionedAncestor( element ) ).to.be.null;
 	} );
 
 	it( 'should find the positioned ancestor (direct parent)', () => {

+ 69 - 26
packages/ckeditor5-utils/tests/dom/position.js

@@ -111,6 +111,7 @@ describe( 'getOptimalPosition()', () => {
 
 	beforeEach( () => {
 		testUtils.sinon.stub( window, 'getComputedStyle' );
+		window.getComputedStyle.callThrough();
 
 		stubWindow( {
 			innerWidth: 10000,
@@ -120,6 +121,15 @@ describe( 'getOptimalPosition()', () => {
 		} );
 	} );
 
+	afterEach( () => {
+		element.remove();
+		target.remove();
+
+		if ( limiter ) {
+			limiter.remove();
+		}
+	} );
+
 	it( 'should work when the target is a Function', () => {
 		setElementTargetPlayground();
 
@@ -177,6 +187,10 @@ describe( 'getOptimalPosition()', () => {
 		describe( 'positioned element parent', () => {
 			let parent;
 
+			afterEach( () => {
+				parent.remove();
+			} );
+
 			it( 'should return coordinates', () => {
 				stubWindow( {
 					innerWidth: 10000,
@@ -196,7 +210,7 @@ describe( 'getOptimalPosition()', () => {
 					position: 'absolute'
 				} );
 
-				element.parentElement = parent;
+				parent.appendChild( element );
 
 				assertPosition( { element, target, positions: [ attachLeftBottom ] }, {
 					top: -900,
@@ -219,20 +233,34 @@ describe( 'getOptimalPosition()', () => {
 					bottom: 10,
 					left: 0,
 					width: 10,
-					height: 10,
-					scrollTop: 100,
-					scrollLeft: 200
+					height: 10
 				}, {
 					position: 'absolute',
 					borderLeftWidth: '20px',
-					borderTopWidth: '40px'
+					borderTopWidth: '40px',
+					overflow: 'scroll',
+					width: '10px',
+					height: '10px',
+					background: 'red'
+				} );
+
+				Object.assign( element.style, {
+					width: '20px',
+					height: '20px',
+					marginTop: '100px',
+					marginLeft: '200px',
+					background: 'green'
 				} );
 
-				element.parentElement = parent;
+				parent.appendChild( element );
+				document.body.appendChild( parent );
+
+				parent.scrollLeft = 200;
+				parent.scrollTop = 100;
 
 				assertPosition( { element, target, positions: [ attachLeftBottom ] }, {
-					top: 160,
-					left: 260,
+					top: 200,
+					left: 280,
 					name: 'left-bottom'
 				} );
 			} );
@@ -268,6 +296,10 @@ describe( 'getOptimalPosition()', () => {
 	describe( 'with a limiter', () => {
 		beforeEach( setElementTargetLimiterPlayground );
 
+		afterEach( () => {
+			limiter.remove();
+		} );
+
 		it( 'should work when the limiter is a Function', () => {
 			assertPosition( {
 				element, target,
@@ -316,7 +348,7 @@ describe( 'getOptimalPosition()', () => {
 
 		// https://github.com/ckeditor/ckeditor5-utils/issues/148
 		it( 'should return coordinates (#3)', () => {
-			limiter.parentNode = getElement( {
+			const parentNode = getElement( {
 				top: 100,
 				left: 0,
 				bottom: 110,
@@ -325,6 +357,9 @@ describe( 'getOptimalPosition()', () => {
 				height: 10
 			} );
 
+			parentNode.appendChild( limiter );
+			document.body.appendChild( parentNode );
+
 			assertPosition( {
 				element, target, limiter,
 				positions: [ attachRightBottom, attachLeftBottom ]
@@ -333,10 +368,12 @@ describe( 'getOptimalPosition()', () => {
 				left: 10,
 				name: 'right-bottom'
 			} );
+
+			parentNode.remove();
 		} );
 
 		it( 'should return the first position that completely fits in the limiter', () => {
-			element = getElement( {
+			const element = getElement( {
 				top: 0,
 				right: 5,
 				bottom: 5,
@@ -352,6 +389,8 @@ describe( 'getOptimalPosition()', () => {
 				left: -5,
 				name: 'left-bottom'
 			} );
+
+			element.remove();
 		} );
 	} );
 
@@ -465,7 +504,7 @@ describe( 'getOptimalPosition()', () => {
 		} );
 
 		it( 'should return the very first coordinates if limiter does not fit into the viewport', () => {
-			limiter = getElement( {
+			const limiter = getElement( {
 				top: -100,
 				right: -80,
 				bottom: -80,
@@ -483,10 +522,12 @@ describe( 'getOptimalPosition()', () => {
 				left: 10,
 				name: 'right-bottom'
 			} );
+
+			limiter.remove();
 		} );
 
 		it( 'should prefer positions fitting entirely into the viewport', () => {
-			target = getElement( {
+			const target = getElement( {
 				top: 100,
 				right: 35,
 				bottom: 120,
@@ -503,6 +544,8 @@ describe( 'getOptimalPosition()', () => {
 				left: 35,
 				name: 'right-bottom'
 			} );
+
+			target.remove();
 		} );
 	} );
 
@@ -578,7 +621,7 @@ describe( 'getOptimalPosition()', () => {
 			// 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( {
+			const limiter = getElement( {
 				top: -100,
 				right: 100,
 				bottom: 100,
@@ -586,7 +629,7 @@ describe( 'getOptimalPosition()', () => {
 				width: 200,
 				height: 200
 			} );
-			target = getElement( {
+			const target = getElement( {
 				top: -30,
 				right: 80,
 				bottom: -10,
@@ -594,7 +637,7 @@ describe( 'getOptimalPosition()', () => {
 				width: 20,
 				height: 20
 			} );
-			element = getElement( {
+			const element = getElement( {
 				top: 0,
 				right: 200,
 				bottom: 200,
@@ -610,6 +653,10 @@ describe( 'getOptimalPosition()', () => {
 				],
 				fitInViewport: true
 			}, 'right-bottom' );
+
+			limiter.remove();
+			target.remove();
+			element.remove();
 		} );
 	} );
 } );
@@ -631,18 +678,14 @@ function assertPositionName( options, expected ) {
 // @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,
-		ownerDocument: document
-	};
+function getElement( rect = {}, styles = {} ) {
+	expect( rect.right - rect.left ).to.equal( rect.width, 'getElement incorrect horizontal values' );
+	expect( rect.bottom - rect.top ).to.equal( rect.height, 'getElement incorrect vertical values' );
 
-	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' );
+	const element = document.createElement( 'div' );
+	document.body.appendChild( element );
 
-	Object.assign( element, properties );
+	sinon.stub( element, 'getBoundingClientRect' ).returns( rect );
 
 	if ( !styles.borderLeftWidth ) {
 		styles.borderLeftWidth = '0px';
@@ -652,7 +695,7 @@ function getElement( properties = {}, styles = {} ) {
 		styles.borderTopWidth = '0px';
 	}
 
-	window.getComputedStyle.withArgs( element ).returns( styles );
+	Object.assign( element.style, styles );
 
 	return element;
 }