Merge pull request #16256 from ckeditor/ck/16106-predictive-text

Fix (typing, engine): Predictive text should not get doubled while typing. Closes #16106.

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

@@ -11,14 +11,15 @@
 
 import Observer from './observer.js';
 import MutationObserver from './mutationobserver.js';
+import FocusObserver from './focusobserver.js';
 import { env } from '@ckeditor/ckeditor5-utils';
 import { debounce, type DebouncedFunc } from 'lodash-es';
 
 import type View from '../view.js';
 import type DocumentSelection from '../documentselection.js';
 import type DomConverter from '../domconverter.js';
 import type Selection from '../selection.js';
-import FocusObserver from './focusobserver.js';
+import type { ViewDocumentCompositionStartEvent } from './compositionobserver.js';
 
 type DomSelection = globalThis.Selection;
 
@@ -128,7 +129,7 @@ export default class SelectionObserver extends Observer {
 
 			// Make sure that model selection is up-to-date at the end of selecting process.
 			// Sometimes `selectionchange` events could arrive after the `mouseup` event and that selection could be already outdated.
-			this._handleSelectionChange( null, domDocument );
+			this._handleSelectionChange( domDocument );
 
 			this.document.isSelecting = false;
 
@@ -177,7 +178,7 @@ export default class SelectionObserver extends Observer {
 				return;
 			}
 
-			this._handleSelectionChange( domEvent, domDocument );
+			this._handleSelectionChange( domDocument );
 
 			// @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) {
 			// @if CK_DEBUG_TYPING // 	console.groupEnd();
@@ -188,6 +189,13 @@ export default class SelectionObserver extends Observer {
 			this._documentIsSelectingInactivityTimeoutDebounced();
 		} );
 
+		// Update the model DocumentSelection just after the Renderer and the SelectionObserver are locked.
+		// We do this synchronously (without waiting for the `selectionchange` DOM event) as browser updates
+		// the DOM selection (but not visually) to span the text that is under composition and could be replaced.
+		this.listenTo<ViewDocumentCompositionStartEvent>( this.view.document, 'compositionstart', () => {
+			this._handleSelectionChange( domDocument );
+		}, { priority: 'lowest' } );
+
 		this._documents.add( domDocument );
 	}
 
@@ -222,10 +230,9 @@ export default class SelectionObserver extends Observer {
 	 * a selection changes and fires {@link module:engine/view/document~Document#event:selectionChange} event on every change
 	 * and {@link module:engine/view/document~Document#event:selectionChangeDone} when a selection stop changing.
 	 *
-	 * @param domEvent DOM event.
 	 * @param domDocument DOM document.
 	 */
-	private _handleSelectionChange( domEvent: unknown, domDocument: Document ) {
+	private _handleSelectionChange( domDocument: Document ) {
 		if ( !this.isEnabled ) {
 			return;
 		}

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

@@ -18,6 +18,7 @@ import createViewRoot from '../_utils/createroot.js';
 import { parse } from '../../../src/dev-utils/view.js';
 import { StylesProcessor } from '../../../src/view/stylesmap.js';
 import env from '@ckeditor/ckeditor5-utils/src/env.js';
+import { priorities } from '@ckeditor/ckeditor5-utils';
 
 describe( 'SelectionObserver', () => {
 	let view, viewDocument, viewRoot, selectionObserver, domRoot, domMain, domDocument;
@@ -192,6 +193,48 @@ describe( 'SelectionObserver', () => {
 		changeDomSelection();
 	} );
 
+	it( 'should fire selectionChange synchronously on composition start event (at lowest priority)', () => {
+		let eventCount = 0;
+		let priorityCheck = 0;
+
+		viewDocument.on( 'selectionChange', ( evt, data ) => {
+			expect( data ).to.have.property( 'domSelection' ).that.equals( domDocument.getSelection() );
+
+			expect( data ).to.have.property( 'oldSelection' ).that.is.instanceof( DocumentSelection );
+			expect( data.oldSelection.rangeCount ).to.equal( 0 );
+
+			expect( data ).to.have.property( 'newSelection' ).that.is.instanceof( ViewSelection );
+			expect( data.newSelection.rangeCount ).to.equal( 1 );
+
+			const newViewRange = data.newSelection.getFirstRange();
+			const viewFoo = viewDocument.getRoot().getChild( 1 ).getChild( 0 );
+
+			expect( newViewRange.start.parent ).to.equal( viewFoo );
+			expect( newViewRange.start.offset ).to.equal( 2 );
+			expect( newViewRange.end.parent ).to.equal( viewFoo );
+			expect( newViewRange.end.offset ).to.equal( 2 );
+
+			expect( priorityCheck ).to.equal( 1 );
+
+			eventCount++;
+		} );
+
+		viewDocument.on( 'compositionstart', () => {
+			priorityCheck++;
+		}, { priority: priorities.lowest + 1 } );
+
+		viewDocument.on( 'compositionstart', () => {
+			priorityCheck++;
+		}, { priority: priorities.lowest - 1 } );
+
+		changeDomSelection();
+
+		viewDocument.fire( 'compositionstart' );
+
+		expect( eventCount ).to.equal( 1 );
+		expect( priorityCheck ).to.equal( 2 );
+	} );
+
 	it( 'should not fire selectionChange for ignored target', done => {
 		viewDocument.on( 'selectionChange', () => {
 			throw 'selectionChange fired in ignored elements';

packages/ckeditor5-typing/src/input.ts

@@ -13,7 +13,11 @@ import { env } from '@ckeditor/ckeditor5-utils';
 import InsertTextCommand from './inserttextcommand.js';
 import InsertTextObserver, { type ViewDocumentInsertTextEvent } from './inserttextobserver.js';
 
-import type { Model } from '@ckeditor/ckeditor5-engine';
+import {
+	type Model,
+	type ViewDocumentCompositionStartEvent,
+	type ViewDocumentKeyDownEvent
+} from '@ckeditor/ckeditor5-engine';
 
 /**
  * Handles text input coming from the keyboard or other input methods.
@@ -53,10 +57,17 @@ export default class Input extends Plugin {
 
 			const { text, selection: viewSelection, resultRange: viewResultRange } = data;
 
+			let modelRanges;
+
 			// If view selection was specified, translate it to model selection.
-			const modelRanges = Array.from( viewSelection.getRanges() ).map( viewRange => {
-				return editor.editing.mapper.toModelRange( viewRange );
-			} );
+			if ( viewSelection ) {
+				modelRanges = Array.from( viewSelection.getRanges() ).map( viewRange => {
+					return editor.editing.mapper.toModelRange( viewRange );
+				} );
+			}
+			else {
+				modelRanges = Array.from( modelSelection.getRanges() );
+			}
 
 			let insertText = text;
 
@@ -109,7 +120,7 @@ export default class Input extends Plugin {
 			// On Android with English keyboard, the composition starts just by putting caret
 			// at the word end or by selecting a table column. This is not a real composition started.
 			// Trigger delete content on first composition key pressed.
-			this.listenTo( view.document, 'keydown', ( evt, data ) => {
+			this.listenTo<ViewDocumentKeyDownEvent>( view.document, 'keydown', ( evt, data ) => {
 				if ( modelSelection.isCollapsed || data.keyCode != 229 || !view.document.isComposing ) {
 					return;
 				}
@@ -129,7 +140,7 @@ export default class Input extends Plugin {
 		} else {
 			// Note: The priority must precede the CompositionObserver handler to call it before
 			// the renderer is blocked, because we want to render this change.
-			this.listenTo( view.document, 'compositionstart', () => {
+			this.listenTo<ViewDocumentCompositionStartEvent>( view.document, 'compositionstart', () => {
 				if ( modelSelection.isCollapsed ) {
 					return;
 				}

packages/ckeditor5-typing/src/inserttextobserver.ts

@@ -112,24 +112,12 @@ export default class InsertTextObserver extends Observer {
 			// @if CK_DEBUG_TYPING // }
 
 			// How do we know where to insert the composed text?
-			// The selection observer is blocked and the view is not updated with the composition changes.
-			// There were three options:
-			//   - Store the selection on `compositionstart` and use it now. This wouldn't work in RTC
-			//     where the view would change and the stored selection might get incorrect.
-			//     We'd need to fallback to the current view selection anyway.
-			//   - Use the current view selection. This is a bit weird and non-intuitive because
-			//     this isn't necessarily the selection on which the user started composing.
-			//     We cannot even know whether it's still collapsed (there might be some weird
-			//     editor feature that changed it in unpredictable ways for us). But it's by far
-			//     the simplest solution and should be stable (the selection is definitely correct)
-			//     and probably mostly predictable (features usually don't modify the selection
-			//     unless called explicitly by the user).
-			//   - Try to follow it from the `beforeinput` events. This would be really complex as each
-			//     `beforeinput` would come with just the range it's changing and we'd need to calculate that.
-			// We decided to go with the 2nd option for its simplicity and stability.
+			// 1. The SelectionObserver is blocked and the view is not updated with the composition changes.
+			// 2. The last moment before it's locked is the `compositionstart` event.
+			// 3. The `SelectionObserver` is listening for `compositionstart` event and immediately converts
+			//    the selection. Handles this at the lowest priority so after the rendering is blocked.
 			viewDocument.fire( 'insertText', new DomEventData( view, domEvent, {
-				text: data,
-				selection: viewDocument.selection
+				text: data
 			} ) );
 		}, { priority: 'lowest' } );
 	}
@@ -174,7 +162,7 @@ export interface InsertTextEventData extends DomEventData {
 	 * The selection into which the text should be inserted.
 	 * If not specified, the insertion should occur at the current view selection.
 	 */
-	selection: ViewSelection | ViewDocumentSelection;
+	selection?: ViewSelection | ViewDocumentSelection;
 
 	/**
 	 * The range that view selection should be set to after insertion.

packages/ckeditor5-typing/tests/input.js

@@ -3,11 +3,12 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
-/* global document */
+/* global document, window */
 
 import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor.js';
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils.js';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph.js';
+import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata.js';
 
 import Input from '../src/input.js';
 import InsertTextCommand from '../src/inserttextcommand.js';
@@ -127,6 +128,28 @@ describe( 'Input', () => {
 				expect( firstCallArgs.resultRange ).to.be.undefined;
 			} );
 
+			it( 'should use model document selection if the selection property is not passed', async () => {
+				const expectedSelection = editor.model.createSelection(
+					editor.model.createPositionAt( editor.model.document.getRoot().getChild( 0 ), 1 )
+				);
+
+				editor.model.change( writer => {
+					writer.setSelection( expectedSelection );
+				} );
+
+				viewDocument.fire( 'insertText', {
+					text: 'bar',
+					preventDefault: sinon.spy()
+				} );
+
+				const firstCallArgs = insertTextCommandSpy.firstCall.args[ 0 ];
+
+				sinon.assert.calledOnce( insertTextCommandSpy );
+				expect( firstCallArgs.text ).to.equal( 'bar' );
+				expect( firstCallArgs.selection.isEqual( expectedSelection ) ).to.be.true;
+				expect( firstCallArgs.resultRange ).to.be.undefined;
+			} );
+
 			it( 'should have result range passed correctly to the insert text command', async () => {
 				const expectedSelection = editor.model.createSelection(
 					editor.model.createPositionAt( editor.model.document.getRoot().getChild( 0 ), 1 )
@@ -169,6 +192,40 @@ describe( 'Input', () => {
 				sinon.assert.calledWithExactly( spy, editor.model.document.selection );
 			} );
 
+			it( 'should update model selection to the DOM selection on composition start and use it on compositionend', () => {
+				const root = editor.model.document.getRoot();
+				const modelSelection = editor.model.document.selection;
+
+				const modelParagraph = root.getChild( 0 );
+				const viewParagraph = viewDocument.getRoot().getChild( 0 );
+				const domParagraph = view.domConverter.mapViewToDom( viewParagraph );
+
+				const expectedRange = editor.model.createRange(
+					editor.model.createPositionAt( modelParagraph, 1 ),
+					editor.model.createPositionAt( modelParagraph, 3 )
+				);
+
+				editor.model.change( writer => writer.setSelection( root.getChild( 0 ), 0 ) );
+
+				window.getSelection().setBaseAndExtent( domParagraph.childNodes[ 0 ], 1, domParagraph.childNodes[ 0 ], 3 );
+
+				viewDocument.fire( 'compositionstart' );
+
+				expect( modelSelection.getFirstRange().isEqual( expectedRange ) ).to.be.true;
+
+				viewDocument.fire( 'compositionend', new DomEventData( view, {
+					preventDefault() {}
+				}, {
+					data: 'bar'
+				} ) );
+
+				const firstCallArgs = insertTextCommandSpy.firstCall.args[ 0 ];
+
+				sinon.assert.calledOnce( insertTextCommandSpy );
+				expect( firstCallArgs.text ).to.equal( 'bar' );
+				expect( firstCallArgs.selection.getFirstRange().isEqual( expectedRange ) ).to.be.true;
+			} );
+
 			it( 'should not call model.deleteContent() on composition start for collapsed model selection', () => {
 				const spy = sinon.spy( editor.model, 'deleteContent' );
 				const root = editor.model.document.getRoot();

packages/ckeditor5-typing/tests/inserttextobserver.js

@@ -186,7 +186,7 @@ describe( 'InsertTextObserver', () => {
 		const firstCallArgs = insertTextEventSpy.firstCall.args[ 1 ];
 
 		expect( firstCallArgs.text ).to.equal( 'bar' );
-		expect( firstCallArgs.selection.isEqual( view.document.selection ) ).to.be.true;
+		expect( firstCallArgs.selection ).to.be.undefined;
 	} );
 
 	it( 'should ignore the empty compositionend event (without any data)', () => {