Merge pull request #13237 from ckeditor/cf/4914

Fix (engine): Fixed focus handling issue which happened on Chrome after nested editable was clicked.

packages/ckeditor5-engine/_src/view/document.js

@@ -80,22 +80,6 @@ export default class Document {
 		 */
 		this.set( 'isFocused', false );
 
-		/**
-		 * Set to `true` if the document is in the process of setting the focus.
-		 *
-		 * To be precise, there are two browser events that we care about: `focus` and `selectionchange`.
-		 *
-		 * Different browsers handle them differently. Chromium sends the `focus` event before the
-		 * `selectionchange` event, which leads to rendering with the old selection state.
-		 *
-		 * The flag is used to prevent rendering when setting the focus is in progress
-		 * and we are waiting for the new value.
-		 *
-		 * @protected
-		 * @observable
-		 * @member {Boolean} module:engine/view/document~Document#_isFocusChanging
-		 */
-
 		/**
 		 * `true` while the user is making a selection in the document (e.g. holding the mouse button and moving the cursor).
 		 * When they stop selecting, the property goes back to `false`.

packages/ckeditor5-engine/_src/view/observer/focusobserver.js

@@ -61,8 +61,23 @@ export default class FocusObserver extends DomEventObserver {
 		 * @private
 		 * @member {Number} #_renderTimeoutId
 		 */
+
+		/**
+		 * Set to `true` if the document is in the process of setting the focus.
+		 *
+		 * The flag is used to indicate that setting the focus is in progress.
+		 *
+		 * @protected
+		 * @type {Boolean} module:engine/view/observer/focusobserver#_isFocusChanging
+		 */
 	}
 
+	/**
+	 * Finishes setting the document focus state.
+	 *
+	 * @function flush
+	 */
+
 	onDomEvent( domEvent ) {
 		this.fire( domEvent.type, domEvent );
 	}

packages/ckeditor5-engine/_src/view/observer/selectionobserver.js

@@ -42,6 +42,15 @@ export default class SelectionObserver extends Observer {
 		 */
 		this.mutationObserver = view.getObserver( MutationObserver );
 
+		/**
+		 * Instance of the focus observer. Focus observer calls
+		 * {@link module:engine/view/observer/focusobserver~FocusObserver#flush} to mark the latest focus change as complete.
+		 *
+		 * @readonly
+		 * @member {module:engine/view/observer/focusobserver~FocusObserver}
+		 * module:engine/view/observer/focusobserver~FocusObserver#focusObserver
+		 */
+
 		/**
 		 * Reference to the view {@link module:engine/view/documentselection~DocumentSelection} object used to compare
 		 * new selection with it.

packages/ckeditor5-engine/_src/view/renderer.js

@@ -105,14 +105,6 @@ export default class Renderer {
 		 */
 		this.set( 'isFocused', false );
 
-		/**
-		 * Indicates if the view document is changing the focus (`true`) and selection rendering should be prevented.
-		 *
-		 * @protected
-		 * @observable
-		 * @member {Boolean} #_isFocusChanging
-		 */
-
 		/**
 		 * Indicates whether the user is making a selection in the document (e.g. holding the mouse button and moving the cursor).
 		 * When they stop selecting, the property goes back to `false`.

packages/ckeditor5-engine/src/view/document.ts

@@ -32,7 +32,6 @@ export default class Document extends BubblingEmitterMixin( ObservableMixin() )
 
 	declare public isReadOnly: boolean;
 	declare public isFocused: boolean;
-	declare public _isFocusChanging: boolean;
 	declare public isSelecting: boolean;
 	declare public isComposing: boolean;
 
@@ -98,23 +97,6 @@ export default class Document extends BubblingEmitterMixin( ObservableMixin() )
 		this.set( 'isFocused', false );
 
 		/**
-		 * Set to `true` if the document is in the process of setting the focus.
-		 *
-		 * To be precise, there are two browser events that we care about: `focus` and `selectionchange`.
-		 *
-		 * Different browsers handle them differently. Chromium sends the `focus` event before the
-		 * `selectionchange` event, which leads to rendering with the old selection state.
-		 *
-		 * The flag is used to prevent rendering when setting the focus is in progress
-		 * and we are waiting for the new value.
-		 *
-		 * @internal
-		 * @observable
-		 * @member {Boolean} module:engine/view/document~Document#_isFocusChanging
-		 */
-		this.set( '_isFocusChanging', false );
-
-		/** @
 		 * `true` while the user is making a selection in the document (e.g. holding the mouse button and moving the cursor).
 		 * When they stop selecting, the property goes back to `false`.
 		 *

packages/ckeditor5-engine/src/view/observer/focusobserver.ts

@@ -25,6 +25,7 @@ import type View from '../view';
  */
 export default class FocusObserver extends DomEventObserver<'focus' | 'blur'> {
 	private _renderTimeoutId!: ReturnType<typeof setTimeout>;
+	private _isFocusChanging: boolean = false;
 
 	constructor( view: View ) {
 		super( view );
@@ -34,8 +35,15 @@ export default class FocusObserver extends DomEventObserver<'focus' | 'blur'> {
 		const document = this.document;
 
 		document.on<ViewDocumentFocusEvent>( 'focus', () => {
-			document.isFocused = true;
-			document._isFocusChanging = true;
+			/**
+			 * Set to `true` if the document is in the process of setting the focus.
+			 *
+			 * The flag is used to indicate that setting the focus is in progress.
+			 *
+			 * @internal
+			 * @type {Boolean} module:engine/view/observer/focusobserver#_isFocusChanging
+			 */
+			this._isFocusChanging = true;
 
 			// Unfortunately native `selectionchange` event is fired asynchronously.
 			// We need to wait until `SelectionObserver` handle the event and then render. Otherwise rendering will
@@ -46,7 +54,7 @@ export default class FocusObserver extends DomEventObserver<'focus' | 'blur'> {
 			// Using `view.change()` instead of `view.forceRender()` to prevent double rendering
 			// in a situation where `selectionchange` already caused selection change.
 			this._renderTimeoutId = setTimeout( () => {
-				document._isFocusChanging = false;
+				this.flush();
 				view.change( () => {} );
 			}, 50 );
 		} );
@@ -56,6 +64,7 @@ export default class FocusObserver extends DomEventObserver<'focus' | 'blur'> {
 
 			if ( selectedEditable === null || selectedEditable === data.target ) {
 				document.isFocused = false;
+				this._isFocusChanging = false;
 
 				// Re-render the document to update view elements
 				// (changing document.isFocused already marked view as changed since last rendering).
@@ -71,6 +80,16 @@ export default class FocusObserver extends DomEventObserver<'focus' | 'blur'> {
 		 */
 	}
 
+	/**
+	 * Finishes setting the document focus state.
+	 */
+	public flush(): void {
+		if ( this._isFocusChanging ) {
+			this._isFocusChanging = false;
+			this.document.isFocused = true;
+		}
+	}
+
 	public onDomEvent( domEvent: FocusEvent ): void {
 		this.fire( domEvent.type, domEvent );
 	}

packages/ckeditor5-engine/src/view/observer/selectionobserver.ts

@@ -18,6 +18,7 @@ import type View from '../view';
 import type DocumentSelection from '../documentselection';
 import type DomConverter from '../domconverter';
 import type Selection from '../selection';
+import FocusObserver from './focusobserver';
 
 type DomSelection = globalThis.Selection;
 
@@ -35,6 +36,7 @@ type DomSelection = globalThis.Selection;
  */
 export default class SelectionObserver extends Observer {
 	public readonly mutationObserver: MutationObserver;
+	public readonly focusObserver: FocusObserver;
 	public readonly selection: DocumentSelection;
 	public readonly domConverter: DomConverter;
 
@@ -58,6 +60,16 @@ export default class SelectionObserver extends Observer {
 		 */
 		this.mutationObserver = view.getObserver( MutationObserver )!;
 
+		/**
+		 * Instance of the focus observer. Focus observer calls
+		 * {@link module:engine/view/observer/focusobserver~FocusObserver#flush} to mark the latest focus change as complete.
+		 *
+		 * @readonly
+		 * @member {module:engine/view/observer/focusobserver~FocusObserver}
+		 * module:engine/view/observer/focusobserver~FocusObserver#focusObserver
+		 */
+		this.focusObserver = view.getObserver( FocusObserver )!;
+
 		/**
 		 * Reference to the view {@link module:engine/view/documentselection~DocumentSelection} object used to compare
 		 * new selection with it.
@@ -283,6 +295,9 @@ export default class SelectionObserver extends Observer {
 			return;
 		}
 
+		// Mark the latest focus change as complete (we got new selection after the focus so the selection is in the focused element).
+		this.focusObserver.flush();
+
 		if ( this.selection.isSimilar( newViewSelection ) ) {
 			// If selection was equal and we are at this point of algorithm, it means that it was incorrect.
 			// Just re-render it, no need to fire any events, etc.
@@ -301,8 +316,6 @@ export default class SelectionObserver extends Observer {
 			// @if CK_DEBUG_TYPING // 	);
 			// @if CK_DEBUG_TYPING // }
 
-			this.document._isFocusChanging = false;
-
 			// Prepare data for new selection and fire appropriate events.
 			this.document.fire<ViewDocumentSelectionEvent>( 'selectionChange', data );
 

packages/ckeditor5-engine/src/view/renderer.ts

@@ -62,7 +62,6 @@ export default class Renderer extends ObservableMixin() {
 	public readonly selection: DocumentSelection;
 
 	declare public readonly isFocused: boolean;
-	declare public readonly _isFocusChanging: boolean;
 	declare public readonly isSelecting: boolean;
 	declare public readonly isComposing: boolean;
 
@@ -135,15 +134,6 @@ export default class Renderer extends ObservableMixin() {
 		 */
 		this.set( 'isFocused', false );
 
-		/**
-         * Indicates if the view document is changing the focus (`true`) and selection rendering should be prevented.
-		 *
-		 * @internal
-		 * @observable
-		 * @member {Boolean}
-		 */
-		this.set( '_isFocusChanging', false );
-
 		/**
 		 * Indicates whether the user is making a selection in the document (e.g. holding the mouse button and moving the cursor).
 		 * When they stop selecting, the property goes back to `false`.
@@ -911,12 +901,6 @@ export default class Renderer extends ObservableMixin() {
 			return;
 		}
 
-		// The focus is still in progress and we are waiting for new values from `selectionchange` event.
-		// In that case, we need to prevent update selection since it would be updated using old values.
-		if ( this._isFocusChanging ) {
-			return;
-		}
-
 		// If there is no selection - remove DOM and fake selections.
 		if ( this.selection.rangeCount === 0 ) {
 			this._removeDomSelection();

packages/ckeditor5-engine/src/view/view.ts

@@ -144,8 +144,8 @@ export default class View extends ObservableMixin() {
 		 * @type {module:engine/view/renderer~Renderer}
 		 */
 		this._renderer = new Renderer( this.domConverter, this.document.selection );
-		this._renderer.bind( 'isFocused', 'isSelecting', 'isComposing', '_isFocusChanging' )
-			.to( this.document, 'isFocused', 'isSelecting', 'isComposing', '_isFocusChanging' );
+		this._renderer.bind( 'isFocused', 'isSelecting', 'isComposing' )
+			.to( this.document, 'isFocused', 'isSelecting', 'isComposing' );
 
 		/**
 		 * A DOM root attributes cache. It saves the initial values of DOM root attributes before the DOM element
@@ -209,8 +209,8 @@ export default class View extends ObservableMixin() {
 
 		// Add default observers.
 		this.addObserver( MutationObserver );
-		this.addObserver( SelectionObserver );
 		this.addObserver( FocusObserver );
+		this.addObserver( SelectionObserver );
 		this.addObserver( KeyObserver );
 		this.addObserver( FakeSelectionObserver );
 		this.addObserver( CompositionObserver );
@@ -542,7 +542,7 @@ export default class View extends ObservableMixin() {
 	 */
 	public forceRender(): void {
 		this._hasChangedSinceTheLastRendering = true;
-		this.document._isFocusChanging = false;
+		this.getObserver( FocusObserver )!.flush();
 		this.change( () => {} );
 	}
 

packages/ckeditor5-engine/tests/view/document.js

@@ -46,16 +46,6 @@ describe( 'Document', () => {
 			sinon.assert.calledOnce( spy );
 		} );
 
-		it( 'should set the observable #_isFocusChanging property', () => {
-			const spy = sinon.spy();
-
-			expect( viewDocument._isFocusChanging ).to.be.false;
-
-			viewDocument.on( 'change:_isFocusChanging', spy );
-			viewDocument._isFocusChanging = true;
-			sinon.assert.calledOnce( spy );
-		} );
-
 		it( 'should set the observable #isFocused property', () => {
 			const spy = sinon.spy();