From 3aa035cc79a38de7201ff74c8e19a0c483ba1f29 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Fri, 29 Nov 2024 17:20:52 +0100 Subject: [PATCH 1/2] The insertText event and related beforeinput events should not be canceled as this causes issues mostly in Safari. Postpone model modification to moment when the browser modify DOM to avoid mutations fixing back and again. --- .../src/view/observer/compositionobserver.ts | 4 +- packages/ckeditor5-typing/src/input.ts | 137 ++++++++---------- .../src/inserttextobserver.ts | 2 +- .../src/widgettypearound/widgettypearound.ts | 2 +- 4 files changed, 63 insertions(+), 82 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/observer/compositionobserver.ts b/packages/ckeditor5-engine/src/view/observer/compositionobserver.ts index 3c90d85b9e2..055935c305e 100644 --- a/packages/ckeditor5-engine/src/view/observer/compositionobserver.ts +++ b/packages/ckeditor5-engine/src/view/observer/compositionobserver.ts @@ -42,7 +42,7 @@ export default class CompositionObserver extends DomEventObserver<'compositionst // @if CK_DEBUG_TYPING // ); // @if CK_DEBUG_TYPING // } document.isComposing = true; - }, { priority: 'low' } ); + } ); document.on( 'compositionend', () => { // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { @@ -52,7 +52,7 @@ export default class CompositionObserver extends DomEventObserver<'compositionst // @if CK_DEBUG_TYPING // ); // @if CK_DEBUG_TYPING // } document.isComposing = false; - }, { priority: 'low' } ); + } ); } /** diff --git a/packages/ckeditor5-typing/src/input.ts b/packages/ckeditor5-typing/src/input.ts index e2262faf0f7..2c7e9a376bf 100644 --- a/packages/ckeditor5-typing/src/input.ts +++ b/packages/ckeditor5-typing/src/input.ts @@ -75,17 +75,9 @@ export default class Input extends Plugin { editor.commands.add( 'input', insertTextCommand ); this.listenTo( view.document, 'insertText', ( evt, data ) => { - // Rendering is disabled while composing so prevent events that will be rendered by the engine - // and should not be applied by the browser. - if ( !view.document.isComposing ) { - data.preventDefault(); - } - // Flush queue on the next beforeinput event because it could happen // that the mutation observer does not notice the DOM change in time. - if ( env.isAndroid && view.document.isComposing ) { - this._compositionQueue.flush( 'next beforeinput' ); - } + this._compositionQueue.flush( 'next beforeinput' ); const { text, selection: viewSelection } = data; @@ -139,33 +131,25 @@ export default class Input extends Plugin { selection: model.createSelection( modelRanges ) }; - // This is a composition event and those are not cancellable, so we need to wait until browser updates the DOM + // This is a beforeinput event, so we need to wait until browser updates the DOM // and we could apply changes to the model and verify if the DOM is valid. // The browser applies changes to the DOM not immediately on beforeinput event. // We just wait for mutation observer to notice changes or as a fallback a timeout. - if ( env.isAndroid && view.document.isComposing ) { - // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { - // @if CK_DEBUG_TYPING // console.log( `%c[Input]%c Queue insertText:%c "${ commandData.text }"%c ` + - // @if CK_DEBUG_TYPING // `[${ commandData.selection.getFirstPosition().path }]-` + - // @if CK_DEBUG_TYPING // `[${ commandData.selection.getLastPosition().path }]` + - // @if CK_DEBUG_TYPING // ` queue size: ${ this._compositionQueue.length + 1 }`, - // @if CK_DEBUG_TYPING // 'font-weight: bold; color: green;', 'font-weight: bold', 'color: blue', '' - // @if CK_DEBUG_TYPING // ); - // @if CK_DEBUG_TYPING // } + // + // Previously we were cancelling the non-composition events, but it caused issues especially in Safari. - this._compositionQueue.push( commandData ); - } else { - // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { - // @if CK_DEBUG_TYPING // console.log( `%c[Input]%c Execute insertText:%c "${ commandData.text }"%c ` + - // @if CK_DEBUG_TYPING // `[${ commandData.selection.getFirstPosition().path }]-` + - // @if CK_DEBUG_TYPING // `[${ commandData.selection.getLastPosition().path }]`, - // @if CK_DEBUG_TYPING // 'font-weight: bold; color: green;', 'font-weight: bold', 'color: blue', '' - // @if CK_DEBUG_TYPING // ); - // @if CK_DEBUG_TYPING // } + // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { + // @if CK_DEBUG_TYPING // console.log( `%c[Input]%c Queue insertText:%c "${ commandData.text }"%c ` + + // @if CK_DEBUG_TYPING // `[${ commandData.selection.getFirstPosition().path }]-` + + // @if CK_DEBUG_TYPING // `[${ commandData.selection.getLastPosition().path }]` + + // @if CK_DEBUG_TYPING // ` queue size: ${ this._compositionQueue.length + 1 }`, + // @if CK_DEBUG_TYPING // 'font-weight: bold; color: green;', 'font-weight: bold', 'color: blue', '' + // @if CK_DEBUG_TYPING // ); + // @if CK_DEBUG_TYPING // } - editor.execute( 'insertText', commandData ); - view.scrollToTheSelection(); - } + // TODO verify if this work correctly in WTA and table selection. + + this._compositionQueue.push( commandData ); } ); // Delete selected content on composition start. @@ -209,19 +193,15 @@ export default class Input extends Plugin { // @if CK_DEBUG_TYPING // } deleteSelectionContent( model, insertTextCommand ); - } ); + }, { priority: 'high' } ); } // Apply composed changes to the model. - if ( env.isAndroid ) { - // Apply changes to the model as they are applied to the DOM by the browser. - // On beforeinput event, the DOM is not yet modified. We wait for detected mutations to apply model changes. - this.listenTo( view.document, 'mutations', ( evt, { mutations } ) => { - if ( !view.document.isComposing ) { - return; - } - - // Check if mutations are relevant for queued changes. + // Apply changes to the model as they are applied to the DOM by the browser. + // On beforeinput event, the DOM is not yet modified. We wait for detected mutations to apply model changes. + this.listenTo( view.document, 'mutations', ( evt, { mutations } ) => { + // Check if mutations are relevant for queued changes. + if ( this._compositionQueue.hasComposedElements() ) { for ( const { node } of mutations ) { const viewElement = findMappedViewAncestor( node, mapper ); const modelElement = mapper.toModelElement( viewElement )!; @@ -232,27 +212,32 @@ export default class Input extends Plugin { return; } } + } - // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { - // @if CK_DEBUG_TYPING // console.log( '%c[Input]%c Mutations not related to the composition.', - // @if CK_DEBUG_TYPING // 'font-weight: bold; color: green;', 'font-style: italic' - // @if CK_DEBUG_TYPING // ); - // @if CK_DEBUG_TYPING // } - } ); + // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { + // @if CK_DEBUG_TYPING // console.log( '%c[Input]%c Mutations not related to the composition.', + // @if CK_DEBUG_TYPING // 'font-weight: bold; color: green;', 'font-style: italic' + // @if CK_DEBUG_TYPING // ); + // @if CK_DEBUG_TYPING // } + } ); - // Make sure that all changes are applied to the model before the end of composition. - this.listenTo( view.document, 'compositionend', () => { - this._compositionQueue.flush( 'composition end' ); - } ); + // Make sure that all changes are applied to the model before the end of composition. + this.listenTo( view.document, 'compositionend', () => { + this._compositionQueue.flush( 'before composition end' ); + }, { priority: 'high' } ); - // Trigger mutations check after the composition completes to fix all DOM changes that got ignored during composition. - // On Android the Renderer is not disabled while composing. While updating DOM nodes we ignore some changes - // that are not that important (like NBSP vs plain space character) and could break the composition flow. - // After composition is completed we trigger additional `mutations` event for elements affected by the composition - // so the Renderer can adjust the DOM to the expected structure without breaking the composition. - this.listenTo( view.document, 'compositionend', () => { - const mutations: Array = []; + // Trigger mutations check after the composition completes to fix all DOM changes that got ignored during composition. + // On Android the Renderer is not disabled while composing. While updating DOM nodes we ignore some changes + // that are not that important (like NBSP vs plain space character) and could break the composition flow. + // After composition is completed we trigger additional `mutations` event for elements affected by the composition + // so the Renderer can adjust the DOM to the expected structure without breaking the composition. + this.listenTo( view.document, 'compositionend', () => { + // There could be new item queued on composition end so flush it. + this._compositionQueue.flush( 'after composition end' ); + const mutations: Array = []; + + if ( this._compositionQueue.hasComposedElements() ) { for ( const element of this._compositionQueue.flushComposedElements() ) { const viewElement = mapper.toViewElement( element ); @@ -262,22 +247,11 @@ export default class Input extends Plugin { mutations.push( { type: 'children', node: viewElement } ); } + } - if ( mutations.length ) { - // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { - // @if CK_DEBUG_TYPING // console.group( '%c[Input]%c Fire post-composition mutation fixes.', - // @if CK_DEBUG_TYPING // 'font-weight: bold; color: green', 'font-weight: bold', '' - // @if CK_DEBUG_TYPING // ); - // @if CK_DEBUG_TYPING // } - - view.document.fire( 'mutations', { mutations } ); - - // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { - // @if CK_DEBUG_TYPING // console.groupEnd(); - // @if CK_DEBUG_TYPING // } - } - }, { priority: 'lowest' } ); - } else { + // Fire composition mutations if any. + // + // For non-Android: // After composition end we need to verify if there are no left-overs. // Listening at the lowest priority so after the `InsertTextObserver` added above (all composed text // should already be applied to the model, view, and DOM). @@ -289,20 +263,20 @@ export default class Input extends Plugin { // It in the most cases just clears the internal record of mutated text nodes // since all changes should already be applied to the DOM. // This is especially needed when user cancels composition, so we can clear nodes marked to sync. - this.listenTo( view.document, 'compositionend', () => { + if ( mutations.length || !env.isAndroid ) { // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { - // @if CK_DEBUG_TYPING // console.group( '%c[Input]%c Force render after composition end.', + // @if CK_DEBUG_TYPING // console.group( '%c[Input]%c Fire post-composition mutation fixes.', // @if CK_DEBUG_TYPING // 'font-weight: bold; color: green', 'font-weight: bold', '' // @if CK_DEBUG_TYPING // ); // @if CK_DEBUG_TYPING // } - view.document.fire( 'mutations', { mutations: [] } ); + view.document.fire( 'mutations', { mutations } ); // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { // @if CK_DEBUG_TYPING // console.groupEnd(); // @if CK_DEBUG_TYPING // } - }, { priority: 'lowest' } ); - } + } + }, { priority: 'lowest' } ); } /** @@ -470,6 +444,13 @@ class CompositionQueue { return this._compositionElements.has( element ); } + /** + * TODO + */ + public hasComposedElements(): boolean { + return this._compositionElements.size > 0; + } + /** * Returns an array of composition-related elements and clears the internal list. */ diff --git a/packages/ckeditor5-typing/src/inserttextobserver.ts b/packages/ckeditor5-typing/src/inserttextobserver.ts index 8a94e30f71f..6f7e890aaf5 100644 --- a/packages/ckeditor5-typing/src/inserttextobserver.ts +++ b/packages/ckeditor5-typing/src/inserttextobserver.ts @@ -124,7 +124,7 @@ export default class InsertTextObserver extends Observer { viewDocument.fire( 'insertText', new DomEventData( view, domEvent, { text: data } ) ); - }, { priority: 'lowest' } ); + }, { priority: 'low' } ); } } diff --git a/packages/ckeditor5-widget/src/widgettypearound/widgettypearound.ts b/packages/ckeditor5-widget/src/widgettypearound/widgettypearound.ts index 57078ca5770..509da5df925 100644 --- a/packages/ckeditor5-widget/src/widgettypearound/widgettypearound.ts +++ b/packages/ckeditor5-widget/src/widgettypearound/widgettypearound.ts @@ -695,7 +695,7 @@ export default class WidgetTypeAround extends Plugin { // Note: The priority must precede the default Input plugin compositionstart handler (to call it before delete content). this._listenToIfEnabled( viewDocument, 'compositionstart', () => { this._insertParagraphAccordingToFakeCaretPosition(); - }, { priority: 'high' } ); + }, { priority: 'highest' } ); } } From 6e6a200103d50a6e009862e87ddb6b14cf2863b5 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Fri, 29 Nov 2024 18:17:14 +0100 Subject: [PATCH 2/2] Improved handling of multi-line text replacements. --- .../src/view/observer/inputobserver.ts | 17 ++++++++--------- packages/ckeditor5-typing/src/input.ts | 7 +++++-- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/observer/inputobserver.ts b/packages/ckeditor5-engine/src/view/observer/inputobserver.ts index 4bea6774a02..b6b31fa170d 100644 --- a/packages/ckeditor5-engine/src/view/observer/inputobserver.ts +++ b/packages/ckeditor5-engine/src/view/observer/inputobserver.ts @@ -136,7 +136,7 @@ export default class InputObserver extends DomEventObserver<'beforeinput'> { // Normalize the insertText data that includes new-line characters. // https://github.com/ckeditor/ckeditor5/issues/2045. - if ( domEvent.inputType == 'insertText' && data && data.includes( '\n' ) ) { + if ( [ 'insertText', 'insertReplacementText' ].includes( domEvent.inputType ) && data && data.includes( '\n' ) ) { // There might be a single new-line or double for new paragraph, but we translate // it to paragraphs as it is our default action for enter handling. const parts = data.split( /\n{1,2}/g ); @@ -159,15 +159,14 @@ export default class InputObserver extends DomEventObserver<'beforeinput'> { partTargetRanges = [ viewDocument.selection.getFirstRange()! ]; } - if ( i + 1 < parts.length ) { - this.fire( domEvent.type, domEvent, { - inputType: 'insertParagraph', - targetRanges: partTargetRanges - } ); + // After a batch of paragraphs interleaved with text fire a dummy beforeinput just to flush the input queue. + this.fire( domEvent.type, domEvent, { + inputType: i + 1 < parts.length ? 'insertParagraph' : 'dummy', + targetRanges: partTargetRanges + } ); - // Use the result view selection so following events will be added one after another. - partTargetRanges = [ viewDocument.selection.getFirstRange()! ]; - } + // Use the result view selection so following events will be added one after another. + partTargetRanges = [ viewDocument.selection.getFirstRange()! ]; } // @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) { diff --git a/packages/ckeditor5-typing/src/input.ts b/packages/ckeditor5-typing/src/input.ts index 2c7e9a376bf..a30a6da6b87 100644 --- a/packages/ckeditor5-typing/src/input.ts +++ b/packages/ckeditor5-typing/src/input.ts @@ -25,7 +25,8 @@ import { type ViewDocumentCompositionStartEvent, type ViewDocumentCompositionEndEvent, type ViewDocumentKeyDownEvent, - type ViewDocumentMutationsEvent + type ViewDocumentMutationsEvent, + type ViewDocumentInputEvent } from '@ckeditor/ckeditor5-engine'; import { debounce } from 'lodash-es'; @@ -74,11 +75,13 @@ export default class Input extends Plugin { editor.commands.add( 'insertText', insertTextCommand ); editor.commands.add( 'input', insertTextCommand ); - this.listenTo( view.document, 'insertText', ( evt, data ) => { + this.listenTo( view.document, 'beforeinput', () => { // Flush queue on the next beforeinput event because it could happen // that the mutation observer does not notice the DOM change in time. this._compositionQueue.flush( 'next beforeinput' ); + }, { priority: 'high' } ); + this.listenTo( view.document, 'insertText', ( evt, data ) => { const { text, selection: viewSelection } = data; let modelRanges;