Merge pull request #958 from ckeditor/t/957

Fix: None of the editable's ancestors should scroll when the DomConverter focuses an editable. Closes #957.

Author: oskarwrobel

src/view/domconverter.js

@@ -736,14 +736,33 @@ export default class DomConverter {
 		const domEditable = this.getCorrespondingDomElement( viewEditable );
 
 		if ( domEditable && domEditable.ownerDocument.activeElement !== domEditable ) {
+			// Save the scrollX and scrollY positions before the focus.
 			const { scrollX, scrollY } = global.window;
-			const { scrollLeft, scrollTop } = domEditable;
+			const scrollPositions = [];
+
+			// Save all scrollLeft and scrollTop values starting from domEditable up to
+			// document#documentElement.
+			forEachDomNodeAncestor( domEditable, node => {
+				const { scrollLeft, scrollTop } = node;
+
+				scrollPositions.push( [ scrollLeft, scrollTop ] );
+			} );
 
 			domEditable.focus();
 
+			// Restore scrollLeft and scrollTop values starting from domEditable up to
+			// document#documentElement.
+			// https://github.com/ckeditor/ckeditor5-engine/issues/951
+			// https://github.com/ckeditor/ckeditor5-engine/issues/957
+			forEachDomNodeAncestor( domEditable, node => {
+				const [ scrollLeft, scrollTop ] = scrollPositions.shift();
+
+				node.scrollLeft = scrollLeft;
+				node.scrollTop = scrollTop;
+			} );
+
+			// Restore the scrollX and scrollY positions after the focus.
 			// https://github.com/ckeditor/ckeditor5-engine/issues/951
-			domEditable.scrollLeft = scrollLeft;
-			domEditable.scrollTop = scrollTop;
 			global.window.scrollTo( scrollX, scrollY );
 		}
 	}
@@ -1058,3 +1077,15 @@ function _hasDomParentOfType( node, types, boundaryParent ) {
 
 	return parents.some( parent => parent.tagName && types.includes( parent.tagName.toLowerCase() ) );
 }
+
+// A helper that executes given callback for each DOM node's ancestor, starting from the given node
+// and ending in document#documentElement.
+//
+// @param {Node} node
+// @param {Function} callback A callback to be executed for each ancestor.
+function forEachDomNodeAncestor( node, callback ) {
+	while ( node && node != global.document ) {
+		callback( node );
+		node = node.parentNode;
+	}
+}

tests/view/domconverter/domconverter.js

@@ -33,21 +33,23 @@ describe( 'DomConverter', () => {
 	} );
 
 	describe( 'focus()', () => {
-		let viewEditable, domEditable, viewDocument;
+		let viewEditable, domEditable, domEditableParent, viewDocument;
 
 		beforeEach( () => {
 			viewDocument = new ViewDocument();
 			viewEditable = new ViewEditable( 'div' );
 			viewEditable.document = viewDocument;
 
 			domEditable = document.createElement( 'div' );
+			domEditableParent = document.createElement( 'div' );
 			converter.bindElements( domEditable, viewEditable );
 			domEditable.setAttribute( 'contenteditable', 'true' );
-			document.body.appendChild( domEditable );
+			domEditableParent.appendChild( domEditable );
+			document.body.appendChild( domEditableParent );
 		} );
 
 		afterEach( () => {
-			document.body.removeChild( domEditable );
+			document.body.removeChild( domEditableParent );
 			viewDocument.destroy();
 		} );
 
@@ -69,19 +71,46 @@ describe( 'DomConverter', () => {
 		} );
 
 		// https://github.com/ckeditor/ckeditor5-engine/issues/951
-		it( 'should actively prevent window scroll in WebKit', () => {
+		// https://github.com/ckeditor/ckeditor5-engine/issues/957
+		it( 'should actively prevent scrolling', () => {
 			const scrollToSpy = testUtils.sinon.stub( global.window, 'scrollTo' );
-			const scrollLeftSpy = sinon.spy();
-			const scrollTopSpy = sinon.spy();
+			const editableScrollLeftSpy = sinon.spy();
+			const editableScrollTopSpy = sinon.spy();
+			const parentScrollLeftSpy = sinon.spy();
+			const parentScrollTopSpy = sinon.spy();
+			const documentElementScrollLeftSpy = sinon.spy();
+			const documentElementScrollTopSpy = sinon.spy();
 
 			Object.defineProperties( domEditable, {
 				scrollLeft: {
 					get: () => 20,
-					set: scrollLeftSpy
+					set: editableScrollLeftSpy
 				},
 				scrollTop: {
 					get: () => 200,
-					set: scrollTopSpy
+					set: editableScrollTopSpy
+				}
+			} );
+
+			Object.defineProperties( domEditableParent, {
+				scrollLeft: {
+					get: () => 40,
+					set: parentScrollLeftSpy
+				},
+				scrollTop: {
+					get: () => 400,
+					set: parentScrollTopSpy
+				}
+			} );
+
+			Object.defineProperties( global.document.documentElement, {
+				scrollLeft: {
+					get: () => 60,
+					set: documentElementScrollLeftSpy
+				},
+				scrollTop: {
+					get: () => 600,
+					set: documentElementScrollTopSpy
 				}
 			} );
 
@@ -90,8 +119,12 @@ describe( 'DomConverter', () => {
 
 			converter.focus( viewEditable );
 			sinon.assert.calledWithExactly( scrollToSpy, 10, 100 );
-			sinon.assert.calledWithExactly( scrollLeftSpy, 20 );
-			sinon.assert.calledWithExactly( scrollTopSpy, 200 );
+			sinon.assert.calledWithExactly( editableScrollLeftSpy, 20 );
+			sinon.assert.calledWithExactly( editableScrollTopSpy, 200 );
+			sinon.assert.calledWithExactly( parentScrollLeftSpy, 40 );
+			sinon.assert.calledWithExactly( parentScrollTopSpy, 400 );
+			sinon.assert.calledWithExactly( documentElementScrollLeftSpy, 60 );
+			sinon.assert.calledWithExactly( documentElementScrollTopSpy, 600 );
 		} );
 	} );