Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Testing] Tracking the composed range so content can be replaced while composing. #16247

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions packages/ckeditor5-engine/src/view/observer/selectionobserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Expand All @@ -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<ViewDocumentCompositionStartEvent>( this.view.document, 'compositionstart', () => {
this._handleSelectionChange( domDocument );
}, { priority: 'lowest' } );

this._documents.add( domDocument );
}

Expand Down Expand Up @@ -222,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;
}
Expand Down
43 changes: 43 additions & 0 deletions packages/ckeditor5-engine/tests/view/observer/selectionobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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';
Expand Down
23 changes: 17 additions & 6 deletions packages/ckeditor5-typing/src/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down
3 changes: 1 addition & 2 deletions packages/ckeditor5-typing/src/inserttextobserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' } );
}
Expand Down
59 changes: 58 additions & 1 deletion packages/ckeditor5-typing/tests/input.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 )
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-typing/tests/inserttextobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)', () => {
Expand Down