Merge pull request #864 from ckeditor/t/795

Fix: View document is now re-rendered after focusing. Closes #795.

Author: Piotr Jasiun

src/view/observer/focusobserver.js

@@ -7,6 +7,8 @@
  * @module engine/view/observer/focusobserver
  */
 
+/* globals setTimeout, clearTimeout */
+
 import DomEventObserver from './domeventobserver';
 
 /**
@@ -28,6 +30,12 @@ export default class FocusObserver extends DomEventObserver {
 
 		document.on( 'focus', () => {
 			document.isFocused = true;
+
+			// Unfortunately native `selectionchange` event is fired asynchronously.
+			// We need to wait until `SelectionObserver` handle the event and then render. Otherwise rendering will
+			// overwrite new DOM selection with selection from the view.
+			// See https://github.com/ckeditor/ckeditor5-engine/issues/795 for more details.
+			this._renderTimeoutId = setTimeout( () => document.render(), 0 );
 		} );
 
 		document.on( 'blur', ( evt, data ) => {
@@ -40,11 +48,29 @@ export default class FocusObserver extends DomEventObserver {
 				document.render();
 			}
 		} );
+
+		/**
+		 * Identifier of the timeout currently used by focus listener to delay rendering execution.
+		 *
+		 * @private
+		 * @member {Number} #_renderTimeoutId
+		 */
 	}
 
 	onDomEvent( domEvent ) {
 		this.fire( domEvent.type, domEvent );
 	}
+
+	/**
+	 * @inheritDoc
+	 */
+	destroy() {
+		if ( this._renderTimeoutId ) {
+			clearTimeout( this._renderTimeoutId );
+		}
+
+		super.destroy();
+	}
 }
 
 /**

tests/manual/nestededitable.md

@@ -3,3 +3,8 @@
 * Put selection inside `foo bar baz` nested editable. Main editable and nested one should be focused (blue outline should be visible).
 * Change selection inside nested editable and see if `Model contents` change accordingly.
 * Click outside the editor. Outline from main editable and nested editable should be removed.
+* Check following scenario:
+  * put selection inside nested editable: `foo bar baz{}`,
+  * click outside the editor (outlines should be removed),
+  * put selection at exact same place as before: `foo bar baz{}`,
+  * both editables should be focused (blue outline should be visible).

tests/view/observer/focusobserver.js

@@ -3,11 +3,11 @@
  * For licensing, see LICENSE.md.
  */
 
-/* globals document */
-
+/* globals document, window */
 import FocusObserver from '../../../src/view/observer/focusobserver';
 import ViewDocument from '../../../src/view/document';
 import ViewRange from '../../../src/view/range';
+import { setData } from '../../../src/dev-utils/view';
 
 describe( 'FocusObserver', () => {
 	let viewDocument, observer;
@@ -115,5 +115,84 @@ describe( 'FocusObserver', () => {
 
 			expect( viewDocument.isFocused ).to.be.true;
 		} );
+
+		it( 'should delay rendering to the next iteration of event loop', () => {
+			const renderSpy = sinon.spy( viewDocument, 'render' );
+			const clock = sinon.useFakeTimers();
+
+			observer.onDomEvent( { type: 'focus', target: domMain } );
+			sinon.assert.notCalled( renderSpy );
+			clock.tick( 0 );
+			sinon.assert.called( renderSpy );
+
+			clock.restore();
+		} );
+
+		it( 'should not call render if destroyed', () => {
+			const renderSpy = sinon.spy( viewDocument, 'render' );
+			const clock = sinon.useFakeTimers();
+
+			observer.onDomEvent( { type: 'focus', target: domMain } );
+			sinon.assert.notCalled( renderSpy );
+			observer.destroy();
+			clock.tick( 0 );
+			sinon.assert.notCalled( renderSpy );
+
+			clock.restore();
+		} );
+	} );
+
+	describe( 'integration test', () => {
+		let viewDocument, viewRoot, domRoot, observer, domSelection;
+
+		beforeEach( () => {
+			domRoot = document.createElement( 'div' );
+			document.body.appendChild( domRoot );
+			viewDocument = new ViewDocument();
+			viewRoot = viewDocument.createRoot( domRoot );
+			observer = viewDocument.getObserver( FocusObserver );
+			domSelection = window.getSelection();
+		} );
+
+		it( 'should render document after selectionChange event', ( done ) => {
+			const selectionChangeSpy = sinon.spy();
+			const renderSpy = sinon.spy();
+
+			setData( viewDocument, '<div contenteditable="true">foo bar</div>' );
+			viewDocument.render();
+			const domEditable = domRoot.childNodes[ 0 ];
+
+			viewDocument.on( 'selectionChange', selectionChangeSpy );
+			viewDocument.on( 'render', renderSpy, { priority: 'low' } );
+
+			viewDocument.on( 'render', () => {
+				sinon.assert.callOrder( selectionChangeSpy, renderSpy );
+				done();
+			}, { priority: 'low' } );
+
+			observer.onDomEvent( { type: 'focus', target: domEditable } );
+			domSelection.collapse( domEditable );
+		} );
+
+		it( 'should render without selectionChange event', ( done ) => {
+			const selectionChangeSpy = sinon.spy();
+			const renderSpy = sinon.spy();
+
+			setData( viewDocument, '<div contenteditable="true">foo bar</div>' );
+			viewDocument.render();
+			const domEditable = domRoot.childNodes[ 0 ];
+
+			viewDocument.on( 'selectionChange', selectionChangeSpy );
+			viewDocument.on( 'render', renderSpy, { priority: 'low' } );
+
+			viewDocument.on( 'render', () => {
+				sinon.assert.notCalled( selectionChangeSpy );
+				sinon.assert.called( renderSpy );
+
+				done();
+			}, { priority: 'low' } );
+
+			observer.onDomEvent( { type: 'focus', target: domEditable } );
+		} );
 	} );
 } );

tests/view/observer/selectionobserver.js

@@ -3,31 +3,32 @@
  * For licensing, see LICENSE.md.
  */
 
-/* globals setTimeout, document */
+/* globals setTimeout, setInterval, clearInterval, document */
 
 import ViewRange from '../../../src/view/range';
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 import ViewSelection from '../../../src/view/selection';
 import ViewDocument from '../../../src/view/document';
 import SelectionObserver from '../../../src/view/observer/selectionobserver';
 import MutationObserver from '../../../src/view/observer/mutationobserver';
-
+import FocusObserver from '../../../src/view/observer/focusobserver';
 import log from '@ckeditor/ckeditor5-utils/src/log';
-
 import { parse } from '../../../src/dev-utils/view';
 
 testUtils.createSinonSandbox();
 
 describe( 'SelectionObserver', () => {
-	let viewDocument, viewRoot, mutationObserver, selectionObserver, domRoot;
+	let viewDocument, viewRoot, mutationObserver, selectionObserver, domRoot, domMain, domDocument;
 
 	beforeEach( ( done ) => {
-		domRoot = document.createElement( 'div' );
-		domRoot.innerHTML = `<div contenteditable="true" id="main"></div><div contenteditable="true" id="additional"></div>`;
-		document.body.appendChild( domRoot );
+		domDocument = document;
+		domRoot = domDocument.createElement( 'div' );
+		domRoot.innerHTML = `<div contenteditable="true"></div><div contenteditable="true" id="additional"></div>`;
+		domMain = domRoot.childNodes[ 0 ];
+		domDocument.body.appendChild( domRoot );
 
 		viewDocument = new ViewDocument();
-		viewDocument.createRoot( document.getElementById( 'main' ) );
+		viewDocument.createRoot( domMain );
 
 		mutationObserver = viewDocument.getObserver( MutationObserver );
 		selectionObserver = viewDocument.getObserver( SelectionObserver );
@@ -41,7 +42,7 @@ describe( 'SelectionObserver', () => {
 		viewDocument.render();
 
 		viewDocument.selection.removeAllRanges();
-		document.getSelection().removeAllRanges();
+		domDocument.getSelection().removeAllRanges();
 
 		viewDocument.isFocused = true;
 
@@ -59,7 +60,7 @@ describe( 'SelectionObserver', () => {
 
 	it( 'should fire selectionChange when it is the only change', ( done ) => {
 		viewDocument.on( 'selectionChange', ( evt, data ) => {
-			expect( data ).to.have.property( 'domSelection' ).that.equals( document.getSelection() );
+			expect( data ).to.have.property( 'domSelection' ).that.equals( domDocument.getSelection() );
 
 			expect( data ).to.have.property( 'oldSelection' ).that.is.instanceof( ViewSelection );
 			expect( data.oldSelection.rangeCount ).to.equal( 0 );
@@ -83,7 +84,7 @@ describe( 'SelectionObserver', () => {
 
 	it( 'should add only one listener to one document', ( done ) => {
 		// Add second roots to ensure that listener is added once.
-		viewDocument.createRoot( document.getElementById( 'additional' ), 'additional' );
+		viewDocument.createRoot( domDocument.getElementById( 'additional' ), 'additional' );
 
 		viewDocument.on( 'selectionChange', () => {
 			done();
@@ -138,6 +139,8 @@ describe( 'SelectionObserver', () => {
 	it( 'should warn and not enter infinite loop', ( done ) => {
 		// Reset infinite loop counters so other tests won't mess up with this test.
 		selectionObserver._clearInfiniteLoop();
+		clearInterval( selectionObserver._clearInfiniteLoopInterval );
+		selectionObserver._clearInfiniteLoopInterval = setInterval( () => selectionObserver._clearInfiniteLoop(), 2000 );
 
 		let counter = 100;
 
@@ -232,6 +235,9 @@ describe( 'SelectionObserver', () => {
 
 		viewDocument.on( 'selectionChangeDone', spy );
 
+		// Disable focus observer to not re-render view on each focus.
+		viewDocument.getObserver( FocusObserver ).disable();
+
 		// Change selection.
 		changeDomSelection();
 
@@ -250,7 +256,7 @@ describe( 'SelectionObserver', () => {
 				const data = spy.firstCall.args[ 1 ];
 
 				expect( spy.calledOnce ).to.true;
-				expect( data ).to.have.property( 'domSelection' ).to.equal( document.getSelection() );
+				expect( data ).to.have.property( 'domSelection' ).to.equal( domDocument.getSelection() );
 
 				expect( data ).to.have.property( 'oldSelection' ).to.instanceof( ViewSelection );
 				expect( data.oldSelection.rangeCount ).to.equal( 0 );
@@ -295,13 +301,13 @@ describe( 'SelectionObserver', () => {
 			}, 110 );
 		}, 100 );
 	} );
-} );
 
-function changeDomSelection() {
-	const domSelection = document.getSelection();
-	const domFoo = document.getElementById( 'main' ).childNodes[ 0 ].childNodes[ 0 ];
-	const offset = domSelection.anchorOffset;
+	function changeDomSelection() {
+		const domSelection = domDocument.getSelection();
+		const domFoo = domMain.childNodes[ 0 ].childNodes[ 0 ];
+		const offset = domSelection.anchorOffset;
 
-	domSelection.removeAllRanges();
-	domSelection.collapse( domFoo, offset == 2 ? 3 : 2 );
-}
+		domSelection.removeAllRanges();
+		domSelection.collapse( domFoo, offset == 2 ? 3 : 2 );
+	}
+} );