From 1e6187d1da1616132f9ec89fb46b06ba2d8803f9 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 16 Apr 2024 19:23:49 +0200 Subject: [PATCH 1/4] Tracking the composed range so content can be replaced while composing. --- .../src/view/observer/selectionobserver.ts | 8 ++++++- packages/ckeditor5-typing/src/input.ts | 23 ++++++++++++++----- .../src/inserttextobserver.ts | 3 +-- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts b/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts index 71bd54a89e7..ba86bf823fe 100644 --- a/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts +++ b/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts @@ -11,6 +11,7 @@ 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'; @@ -18,7 +19,7 @@ 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; @@ -188,6 +189,11 @@ export default class SelectionObserver extends Observer { this._documentIsSelectingInactivityTimeoutDebounced(); } ); + // On composition start upcast the selection, so it includes composed text to be replaced on composition end. + this.listenTo( this.view.document, 'compositionstart', () => { + this._handleSelectionChange( null, domDocument ); + }, { priority: 'lowest' } ); + this._documents.add( domDocument ); } diff --git a/packages/ckeditor5-typing/src/input.ts b/packages/ckeditor5-typing/src/input.ts index 7716904fa02..941b683dde6 100644 --- a/packages/ckeditor5-typing/src/input.ts +++ b/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( 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( view.document, 'compositionstart', () => { if ( modelSelection.isCollapsed ) { return; } diff --git a/packages/ckeditor5-typing/src/inserttextobserver.ts b/packages/ckeditor5-typing/src/inserttextobserver.ts index 3a94c18da04..dfb7e070cb2 100644 --- a/packages/ckeditor5-typing/src/inserttextobserver.ts +++ b/packages/ckeditor5-typing/src/inserttextobserver.ts @@ -128,8 +128,7 @@ export default class InsertTextObserver extends Observer { // `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. viewDocument.fire( 'insertText', new DomEventData( view, domEvent, { - text: data, - selection: viewDocument.selection + text: data } ) ); }, { priority: 'lowest' } ); } From 84e74039c002430bfcb497ada5f8d5b596c96652 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 18 Apr 2024 19:36:53 +0200 Subject: [PATCH 2/4] Added tests. --- .../src/view/observer/selectionobserver.ts | 9 ++- .../tests/view/observer/selectionobserver.js | 43 ++++++++++++++ packages/ckeditor5-typing/tests/input.js | 59 ++++++++++++++++++- .../tests/inserttextobserver.js | 2 +- 4 files changed, 106 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts b/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts index ba86bf823fe..5c68eb17acb 100644 --- a/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts +++ b/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts @@ -129,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; @@ -178,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(); @@ -191,7 +191,7 @@ export default class SelectionObserver extends Observer { // On composition start upcast the selection, so it includes composed text to be replaced on composition end. this.listenTo( this.view.document, 'compositionstart', () => { - this._handleSelectionChange( null, domDocument ); + this._handleSelectionChange( domDocument ); }, { priority: 'lowest' } ); this._documents.add( domDocument ); @@ -228,10 +228,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; } diff --git a/packages/ckeditor5-engine/tests/view/observer/selectionobserver.js b/packages/ckeditor5-engine/tests/view/observer/selectionobserver.js index 4c4c75359e2..31ff2e5c0c4 100644 --- a/packages/ckeditor5-engine/tests/view/observer/selectionobserver.js +++ b/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'; diff --git a/packages/ckeditor5-typing/tests/input.js b/packages/ckeditor5-typing/tests/input.js index ffd325fa73e..73c6465ce64 100644 --- a/packages/ckeditor5-typing/tests/input.js +++ b/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(); diff --git a/packages/ckeditor5-typing/tests/inserttextobserver.js b/packages/ckeditor5-typing/tests/inserttextobserver.js index 7ff945192d6..0f8cfdf9881 100644 --- a/packages/ckeditor5-typing/tests/inserttextobserver.js +++ b/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)', () => { From da0b502ae12dc538abf20c4980c966bdefd46536 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 25 Apr 2024 16:53:14 +0200 Subject: [PATCH 3/4] Updated code comment. --- .../src/inserttextobserver.ts | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/packages/ckeditor5-typing/src/inserttextobserver.ts b/packages/ckeditor5-typing/src/inserttextobserver.ts index dfb7e070cb2..46a32963f52 100644 --- a/packages/ckeditor5-typing/src/inserttextobserver.ts +++ b/packages/ckeditor5-typing/src/inserttextobserver.ts @@ -112,21 +112,10 @@ 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 } ) ); @@ -173,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. From d812da85bb4dabcbf236b8cdaf2e446b7f6a8e3e Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 25 Apr 2024 17:06:34 +0200 Subject: [PATCH 4/4] Updated code comment. --- .../ckeditor5-engine/src/view/observer/selectionobserver.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts b/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts index 5c68eb17acb..552f6861865 100644 --- a/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts +++ b/packages/ckeditor5-engine/src/view/observer/selectionobserver.ts @@ -189,7 +189,9 @@ export default class SelectionObserver extends Observer { this._documentIsSelectingInactivityTimeoutDebounced(); } ); - // On composition start upcast the selection, so it includes composed text to be replaced on composition end. + // 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( this.view.document, 'compositionstart', () => { this._handleSelectionChange( domDocument ); }, { priority: 'lowest' } );