Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Simplified the fix, added test for the issue.
Browse files Browse the repository at this point in the history
  • Loading branch information
oleq committed May 29, 2018
1 parent 6ed1ef8 commit 974ea70
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 30 deletions.
60 changes: 30 additions & 30 deletions src/dom/scroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down
9 changes: 9 additions & 0 deletions tests/dom/scroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 974ea70

Please sign in to comment.