Преглед изворни кода

Simplified the fix, added test for the issue.

Aleksander Nowodzinski пре 7 година
родитељ
комит
e3eaa71d61
2 измењених фајлова са 39 додато и 30 уклоњено
  1. 30 30
      packages/ckeditor5-utils/src/dom/scroll.js
  2. 9 0
      packages/ckeditor5-utils/tests/dom/scroll.js

+ 30 - 30
packages/ckeditor5-utils/src/dom/scroll.js

@@ -45,32 +45,36 @@ export function scrollViewportToShowTarget( { target, viewportOffset = 0 } ) {
 			firstAncestorToScroll = getParentElement( currentFrame );
 		}
 
-		if ( firstAncestorToScroll ) {
-			// Scroll the target's ancestors first. Once done, scrolling the viewport is easy.
-			scrollAncestorsToShowRect( firstAncestorToScroll, () => {
-				// Note: If the target does not belong to the current window **directly**,
-				// i.e. it resides in an iframe belonging to the window, obtain the target's rect
-				// in the coordinates of the current window. By default, a Rect returns geometry
-				// relative to the current window's viewport. To make it work in a parent window,
-				// it must be shifted.
-				return getRectRelativeToWindow( target, currentWindow );
-			} );
-
-			// Obtain the rect of the target after it has been scrolled within its ancestors.
-			// It's time to scroll the viewport.
-			const targetRect = getRectRelativeToWindow( target, currentWindow );
-
-			scrollWindowToShowRect( currentWindow, targetRect, viewportOffset );
-
-			if ( currentWindow.parent != currentWindow ) {
-				// Keep the reference to the <iframe> element the "previous current window" was
-				// rendered within. It will be useful to re–calculate the rect of the target
-				// in the parent window's relative geometry. The target's rect must be shifted
-				// by it's iframe's position.
-				currentFrame = currentWindow.frameElement;
-				currentWindow = currentWindow.parent;
-			} else {
-				currentWindow = null;
+		// Scroll the target's ancestors first. Once done, scrolling the viewport is easy.
+		scrollAncestorsToShowRect( firstAncestorToScroll, () => {
+			// Note: If the target does not belong to the current window **directly**,
+			// i.e. it resides in an iframe belonging to the window, obtain the target's rect
+			// in the coordinates of the current window. By default, a Rect returns geometry
+			// relative to the current window's viewport. To make it work in a parent window,
+			// it must be shifted.
+			return getRectRelativeToWindow( target, currentWindow );
+		} );
+
+		// Obtain the rect of the target after it has been scrolled within its ancestors.
+		// It's time to scroll the viewport.
+		const targetRect = getRectRelativeToWindow( target, currentWindow );
+
+		scrollWindowToShowRect( currentWindow, targetRect, viewportOffset );
+
+		if ( currentWindow.parent != currentWindow ) {
+			// Keep the reference to the <iframe> element the "previous current window" was
+			// rendered within. It will be useful to re–calculate the rect of the target
+			// in the parent window's relative geometry. The target's rect must be shifted
+			// by it's iframe's position.
+			currentFrame = currentWindow.frameElement;
+			currentWindow = currentWindow.parent;
+
+			// If the current window has some parent but frameElement is inaccessible, then they have
+			// different domains/ports and, due to security reasons, accessing and scrolling
+			// the parent window won't be possible.
+			// See https://github.com/ckeditor/ckeditor5/issues/930.
+			if ( !currentFrame ) {
+				return;
 			}
 		} else {
 			currentWindow = null;
@@ -254,10 +258,6 @@ function getWindow( elementOrRange ) {
 // @param {HTMLElement|Range} firstRect
 // @returns {HTMLelement}
 function getParentElement( elementOrRange ) {
-	if ( !elementOrRange ) {
-		return null;
-	}
-
 	if ( isRange( elementOrRange ) ) {
 		let parent = elementOrRange.commonAncestorContainer;
 

+ 9 - 0
packages/ckeditor5-utils/tests/dom/scroll.js

@@ -291,6 +291,15 @@ describe( 'scrollViewportToShowTarget()', () => {
 			sinon.assert.calledWithExactly( iframeWindow.scrollTo, 100, -100 );
 			sinon.assert.calledWithExactly( window.scrollTo, 1820, 1520 );
 		} );
+
+		// https://github.com/ckeditor/ckeditor5/issues/930
+		it( 'should not throw if the child frame has no access to the #frameElement of the parent', () => {
+			sinon.stub( iframeWindow, 'frameElement' ).get( () => null );
+
+			expect( () => {
+				scrollViewportToShowTarget( { target } );
+			} ).to.not.throw();
+		} );
 	} );
 
 	// Note: Because everything is a mock, scrolling the firstAncestor doesn't really change