Review fixes.

ckeditor · Jul 18, 2023 · aa4667b · aa4667b
1 parent 45558a4
commit aa4667b
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 60 deletions.
diff --git a/packages/ckeditor5-utils/src/dom/scroll.ts b/packages/ckeditor5-utils/src/dom/scroll.ts
@@ -151,14 +151,17 @@ export function scrollViewportToShowTarget<T extends boolean, U extends IfTrue<T
  * @param target A target, which supposed to become visible to the user.
  * @param ancestorOffset An offset between the target and the boundary of scrollable ancestors
  * to be maintained while scrolling.
+ * @param limiterElement The outermost ancestor that should be scrolled. If specified, it can prevent
+ * scrolling the whole page.
  */
-export function scrollAncestorsToShowTarget( target: HTMLElement | Range, ancestorOffset?: number ): void {
+export function scrollAncestorsToShowTarget( target: HTMLElement | Range, ancestorOffset?: number, limiterElement?: HTMLElement ): void {
 	const targetParent = getParentElement( target );
 
 	scrollAncestorsToShowRect( {
 		parent: targetParent,
 		getRect: () => new Rect( target ),
-		ancestorOffset
+		ancestorOffset,
+		limiterElement
 	} );
 }
 
@@ -291,8 +294,9 @@ function scrollWindowToShowRect<T extends boolean, U extends IfTrue<T>>(
  * anyway.
  * @param options.forceScroll When set `true`, the `rect` will be aligned to the top of scrollable ancestors
  * whether it is already visible or not. This option will only work when `alignToTop` is `true`
+ * @param options.limiterElement The outermost ancestor that should be scrolled. Defaults to the `<body>` element.
  */
-export function scrollAncestorsToShowRect<T extends boolean, U extends IfTrue<T>>(
+function scrollAncestorsToShowRect<T extends boolean, U extends IfTrue<T>>(
 	{
 		parent,
 		getRect,

diff --git a/packages/ckeditor5-utils/tests/dom/scroll.js b/packages/ckeditor5-utils/tests/dom/scroll.js
@@ -7,8 +7,7 @@
 
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 import { stubGeometry, assertScrollPosition } from '../_utils/scroll';
-import { scrollViewportToShowTarget, scrollAncestorsToShowTarget, scrollAncestorsToShowRect } from '../../src/dom/scroll';
-import { Rect } from '../../src';
+import { scrollViewportToShowTarget, scrollAncestorsToShowTarget } from '../../src/dom/scroll';
 
 describe( 'scrollAncestorsToShowTarget()', () => {
 	let target, element, firstAncestor, secondAncestor;
@@ -116,6 +115,15 @@ describe( 'scrollAncestorsToShowTarget()', () => {
 			assertScrollPosition( document.body, { scrollLeft: 1000, scrollTop: 1000 } );
 		} );
 
+		it( 'should not change the scroll of the ancestors of the given limiter', () => {
+			stubGeometry( testUtils, target, { top: 25, right: 75, bottom: 75, left: 25, width: 50, height: 50 } );
+
+			scrollAncestorsToShowTarget( target, 0, firstAncestor );
+
+			assertScrollPosition( firstAncestor, { scrollTop: 100, scrollLeft: 100 } );
+			assertScrollPosition( secondAncestor, { scrollTop: 100, scrollLeft: 100 } );
+		} );
+
 		it( 'should set #scrollTop and #scrollLeft of the ancestor to show the target (above)', () => {
 			stubGeometry( testUtils, target, { top: -100, right: 75, bottom: 0, left: 25, width: 50, height: 100 } );
 
@@ -172,6 +180,15 @@ describe( 'scrollAncestorsToShowTarget()', () => {
 			assertScrollPosition( document.body, { scrollLeft: 1000, scrollTop: 1000 } );
 		} );
 
+		it( 'should not change the scroll of the ancestors of the given limiter', () => {
+			stubGeometry( testUtils, target, { top: 25, right: 75, bottom: 75, left: 25, width: 50, height: 50 } );
+
+			scrollAncestorsToShowTarget( target, 20, firstAncestor );
+
+			assertScrollPosition( firstAncestor, { scrollTop: 100, scrollLeft: 100 } );
+			assertScrollPosition( secondAncestor, { scrollTop: 100, scrollLeft: 100 } );
+		} );
+
 		it( 'should set #scrollTop and #scrollLeft of the ancestor to show the target (above)', () => {
 			stubGeometry( testUtils, target, { top: -100, right: 75, bottom: 0, left: 25, width: 50, height: 100 } );
 
@@ -216,61 +233,6 @@ describe( 'scrollAncestorsToShowTarget()', () => {
 	/* eslint-enable mocha/no-identical-title */
 } );
 
-describe( 'scrollAncestorsToShowRect', () => {
-	it( 'should not change the scroll of the ancestors of the given limiter', () => {
-		testUtils.createSinonSandbox();
-
-		const element = document.createElement( 'p' );
-		const firstAncestor = document.createElement( 'blockquote' );
-		const secondAncestor = document.createElement( 'div' );
-		const target = element;
-
-		document.body.appendChild( secondAncestor );
-		secondAncestor.appendChild( firstAncestor );
-		firstAncestor.appendChild( element );
-
-		// Make the element immune to the border-width-* styles in the test environment.
-		testUtils.sinon.stub( window, 'getComputedStyle' ).returns( {
-			borderTopWidth: '0px',
-			borderRightWidth: '0px',
-			borderBottomWidth: '0px',
-			borderLeftWidth: '0px',
-			direction: 'ltr'
-		} );
-
-		stubGeometry( testUtils, firstAncestor, {
-			top: 0, right: 100, bottom: 100, left: 0, width: 100, height: 100
-		}, {
-			scrollLeft: 100, scrollTop: 100
-		} );
-
-		stubGeometry( testUtils, secondAncestor, {
-			top: -100, right: 0, bottom: 0, left: -100, width: 100, height: 100
-		}, {
-			scrollLeft: 100, scrollTop: 100
-		} );
-
-		stubGeometry( testUtils, document.body, {
-			top: 1000, right: 2000, bottom: 1000, left: 1000, width: 1000, height: 1000
-		}, {
-			scrollLeft: 1000, scrollTop: 1000
-		} );
-
-		stubGeometry( testUtils, target, { top: 0, right: 200, bottom: 100, left: 100, width: 100, height: 100 } );
-
-		scrollAncestorsToShowRect( {
-			parent: firstAncestor,
-			getRect: () => new Rect( target ),
-			limiterElement: firstAncestor
-		} );
-
-		assertScrollPosition( firstAncestor, { scrollTop: 100, scrollLeft: 100 } );
-		assertScrollPosition( secondAncestor, { scrollTop: 100, scrollLeft: 100 } );
-
-		secondAncestor.remove();
-	} );
-} );
-
 describe( 'scrollViewportToShowTarget()', () => {
 	let target, firstAncestor, element;
 	const viewportOffset = 30;