From c1edaefa26735f26df573502b173e856168ba429 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sat, 18 Nov 2023 20:25:22 +0100 Subject: [PATCH 01/10] GHS coupled attributes should be removed from selection change. --- packages/ckeditor5-engine/src/index.ts | 3 +- .../ckeditor5-html-support/src/datafilter.ts | 39 +++++++++++++++- .../tests/manual/ghs-link.html | 18 ++++++++ .../tests/manual/ghs-link.js | 44 +++++++++++++++++++ .../tests/manual/ghs-link.md | 1 + 5 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 packages/ckeditor5-html-support/tests/manual/ghs-link.html create mode 100644 packages/ckeditor5-html-support/tests/manual/ghs-link.js create mode 100644 packages/ckeditor5-html-support/tests/manual/ghs-link.md diff --git a/packages/ckeditor5-engine/src/index.ts b/packages/ckeditor5-engine/src/index.ts index 9407b6843ae..ef8dbcfe93e 100644 --- a/packages/ckeditor5-engine/src/index.ts +++ b/packages/ckeditor5-engine/src/index.ts @@ -78,7 +78,8 @@ export { transformSets } from './model/operation/transform'; export { default as DocumentSelection, type DocumentSelectionChangeRangeEvent, - type DocumentSelectionChangeMarkerEvent + type DocumentSelectionChangeMarkerEvent, + type DocumentSelectionChangeAttributeEvent } from './model/documentselection'; export { default as Range } from './model/range'; export { default as LiveRange, type LiveRangeChangeRangeEvent } from './model/liverange'; diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index 97f79620ad7..7e1c254d6f0 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -16,7 +16,8 @@ import { type ViewElement, type MatchResult, type ViewConsumable, - type MatcherObjectPattern + type MatcherObjectPattern, + type DocumentSelectionChangeAttributeEvent } from 'ckeditor5/src/engine'; import { @@ -438,6 +439,7 @@ export default class DataFilter extends Plugin { */ private _registerCoupledAttributesPostFixer() { const model = this.editor.model; + const selection = model.document.selection; model.document.registerPostFixer( writer => { const changes = model.document.differ.getChanges(); @@ -471,6 +473,41 @@ export default class DataFilter extends Plugin { return changed; } ); + + this.listenTo( selection, 'change:attribute', ( evt, { attributeKeys } ) => { + const removeAttributes = new Set(); + const coupledAttributes = this._getCoupledAttributesMap(); + + for ( const attributeKey of attributeKeys ) { + // Handle only attribute removals. + if ( selection.hasAttribute( attributeKey ) ) { + continue; + } + + // Find a list of coupled GHS attributes. + const coupledAttributeKeys = coupledAttributes.get( attributeKey ); + + if ( !coupledAttributeKeys ) { + continue; + } + + for ( const coupledAttributeKey of coupledAttributeKeys ) { + if ( selection.hasAttribute( coupledAttributeKey ) ) { + removeAttributes.add( coupledAttributeKey ); + } + } + } + + if ( removeAttributes.size == 0 ) { + return; + } + + model.change( writer => { + for ( const attributeKey of removeAttributes ) { + writer.removeSelectionAttribute( attributeKey ); + } + } ); + } ); } /** diff --git a/packages/ckeditor5-html-support/tests/manual/ghs-link.html b/packages/ckeditor5-html-support/tests/manual/ghs-link.html new file mode 100644 index 00000000000..2f67972a55f --- /dev/null +++ b/packages/ckeditor5-html-support/tests/manual/ghs-link.html @@ -0,0 +1,18 @@ + + + + + +
+

foo bar

+

foo bar baz

+

bar baz

+

bar

+

foobar

+
diff --git a/packages/ckeditor5-html-support/tests/manual/ghs-link.js b/packages/ckeditor5-html-support/tests/manual/ghs-link.js new file mode 100644 index 00000000000..adf1b189ed6 --- /dev/null +++ b/packages/ckeditor5-html-support/tests/manual/ghs-link.js @@ -0,0 +1,44 @@ +/** + * @license Copyright (c) 2003-2023, CKSource Holding sp. z o.o. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +/* globals console:false, window, document */ + +import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'; +import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset'; +import SourceEditing from '@ckeditor/ckeditor5-source-editing/src/sourceediting'; + +import GeneralHtmlSupport from '../../src/generalhtmlsupport'; + +ClassicEditor + .create( document.querySelector( '#editor' ), { + plugins: [ + ArticlePluginSet, SourceEditing, GeneralHtmlSupport + ], + toolbar: [ + 'sourceEditing', + '|', + 'heading', + '|', + 'bold', 'italic', 'link', + '|', + 'undo', 'redo' + ], + htmlSupport: { + allow: [ + { + name: /^.*$/, + styles: true, + attributes: true, + classes: true + } + ] + } + } ) + .then( editor => { + window.editor = editor; + } ) + .catch( err => { + console.error( err.stack ); + } ); diff --git a/packages/ckeditor5-html-support/tests/manual/ghs-link.md b/packages/ckeditor5-html-support/tests/manual/ghs-link.md new file mode 100644 index 00000000000..19972829017 --- /dev/null +++ b/packages/ckeditor5-html-support/tests/manual/ghs-link.md @@ -0,0 +1 @@ +## GHS Link handling From 047e948b0e276b66471663aa9f7c677e6c902d8e Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sat, 18 Nov 2023 20:27:31 +0100 Subject: [PATCH 02/10] 2-SCM fixes and edge click handling moved from link editing. --- packages/ckeditor5-link/src/linkediting.ts | 80 ++-------- packages/ckeditor5-link/tests/linkediting.js | 19 --- .../src/twostepcaretmovement.ts | 141 ++++++++++++++++-- 3 files changed, 141 insertions(+), 99 deletions(-) diff --git a/packages/ckeditor5-link/src/linkediting.ts b/packages/ckeditor5-link/src/linkediting.ts index 8b4bb669a8c..59d2abc39ca 100644 --- a/packages/ckeditor5-link/src/linkediting.ts +++ b/packages/ckeditor5-link/src/linkediting.ts @@ -11,18 +11,15 @@ import { Plugin, type Editor } from 'ckeditor5/src/core'; -import { - MouseObserver, - type Model, - type Schema, - type Writer, - type ViewElement, - type ModelDeleteContentEvent, - type ModelInsertContentEvent, - type ViewDocumentKeyDownEvent, - type ViewDocumentMouseDownEvent, - type ViewDocumentClickEvent, - type ViewDocumentSelectionChangeEvent +import type { + Model, + Schema, + Writer, + ViewElement, + ModelDeleteContentEvent, + ModelInsertContentEvent, + ViewDocumentKeyDownEvent, + ViewDocumentClickEvent } from 'ckeditor5/src/engine'; import { Input, @@ -146,9 +143,6 @@ export default class LinkEditing extends Plugin { // Change the attributes of the selection in certain situations after the link was inserted into the document. this._enableInsertContentSelectionAttributesFixer(); - // Handle a click at the beginning/end of a link element. - this._enableClickingAfterLink(); - // Handle typing over the link. this._enableTypingOverLink(); @@ -400,62 +394,6 @@ export default class LinkEditing extends Plugin { }, { priority: 'low' } ); } - /** - * Starts listening to {@link module:engine/view/document~Document#event:mousedown} and - * {@link module:engine/view/document~Document#event:selectionChange} and puts the selection before/after a link node - * if clicked at the beginning/ending of the link. - * - * The purpose of this action is to allow typing around the link node directly after a click. - * - * See https://github.com/ckeditor/ckeditor5/issues/1016. - */ - private _enableClickingAfterLink(): void { - const editor = this.editor; - const model = editor.model; - - editor.editing.view.addObserver( MouseObserver ); - - let clicked = false; - - // Detect the click. - this.listenTo( editor.editing.view.document, 'mousedown', () => { - clicked = true; - } ); - - // When the selection has changed... - this.listenTo( editor.editing.view.document, 'selectionChange', () => { - if ( !clicked ) { - return; - } - - // ...and it was caused by the click... - clicked = false; - - const selection = model.document.selection; - - // ...and no text is selected... - if ( !selection.isCollapsed ) { - return; - } - - // ...and clicked text is the link... - if ( !selection.hasAttribute( 'linkHref' ) ) { - return; - } - - const position = selection.getFirstPosition()!; - const linkRange = findAttributeRange( position, 'linkHref', selection.getAttribute( 'linkHref' ), model ); - - // ...check whether clicked start/end boundary of the link. - // If so, remove the `linkHref` attribute. - if ( position.isTouching( linkRange.start ) || position.isTouching( linkRange.end ) ) { - model.change( writer => { - removeLinkAttributesFromSelection( writer, getLinkAttributesAllowedOnText( model.schema ) ); - } ); - } - } ); - } - /** * Starts listening to {@link module:engine/model/model~Model#deleteContent} and {@link module:engine/model/model~Model#insertContent} * and checks whether typing over the link. If so, attributes of removed text are preserved and applied to the inserted text. diff --git a/packages/ckeditor5-link/tests/linkediting.js b/packages/ckeditor5-link/tests/linkediting.js index b6c4fab6554..0f29582a1cb 100644 --- a/packages/ckeditor5-link/tests/linkediting.js +++ b/packages/ckeditor5-link/tests/linkediting.js @@ -1281,25 +1281,6 @@ describe( 'LinkEditing', () => { ); } ); - it( 'should not touch other attributes than `linkHref`', () => { - setModelData( model, '<$text bold="true" linkHref="url">Bar[]' ); - - editor.editing.view.document.fire( 'mousedown' ); - editor.editing.view.document.fire( 'selectionChange', { - newSelection: view.document.selection - } ); - - expect( getModelData( model ) ).to.equal( - '<$text bold="true" linkHref="url">Bar<$text bold="true">[]' - ); - - editor.execute( 'insertText', { text: 'Foo' } ); - - expect( getModelData( model ) ).to.equal( - '<$text bold="true" linkHref="url">Bar<$text bold="true">Foo[]' - ); - } ); - it( 'should do nothing if the text was not clicked', () => { setModelData( model, '<$text linkHref="url">Bar[]' ); diff --git a/packages/ckeditor5-typing/src/twostepcaretmovement.ts b/packages/ckeditor5-typing/src/twostepcaretmovement.ts index 1c4becd5990..d77edebb560 100644 --- a/packages/ckeditor5-typing/src/twostepcaretmovement.ts +++ b/packages/ckeditor5-typing/src/twostepcaretmovement.ts @@ -11,13 +11,16 @@ import { Plugin, type Editor } from '@ckeditor/ckeditor5-core'; import { keyCodes } from '@ckeditor/ckeditor5-utils'; -import type { - DocumentSelection, - DocumentSelectionChangeEvent, - DomEventData, - Model, - Position, - ViewDocumentArrowKeyEvent +import { + MouseObserver, + type DocumentSelection, + type DocumentSelectionChangeEvent, + type DomEventData, + type Model, + type Position, + type ViewDocumentArrowKeyEvent, + type ViewDocumentMouseDownEvent, + type ViewDocumentSelectionChangeEvent } from '@ckeditor/ckeditor5-engine'; /** @@ -256,6 +259,8 @@ export default class TwoStepCaretMovement extends Plugin { this._restoreGravity(); } ); + + this._enableClickingAfterNode(); } /** @@ -313,7 +318,16 @@ export default class TwoStepCaretMovement extends Plugin { // if ( isBetweenDifferentAttributes( position, attributes ) ) { preventCaretMovement( data ); - this._overrideGravity(); + + // TODO + if ( + hasAnyAttribute( selection, attributes ) && + isBetweenDifferentAttributeValues( position, attributes ) + ) { + clearDifferentValueSelectionAttributes( model, attributes ); + } else { + this._overrideGravity(); + } return true; } @@ -345,7 +359,13 @@ export default class TwoStepCaretMovement extends Plugin { if ( this._isGravityOverridden ) { preventCaretMovement( data ); this._restoreGravity(); - setSelectionAttributesFromTheNodeBefore( model, attributes, position ); + + // TODO + if ( isBetweenDifferentAttributeValues( position, attributes ) ) { + clearDifferentValueSelectionAttributes( model, attributes ); + } else { + setSelectionAttributesFromTheNodeBefore( model, attributes, position ); + } return true; } else { @@ -365,6 +385,17 @@ export default class TwoStepCaretMovement extends Plugin { return false; } + // TODO + if ( + !hasAnyAttribute( selection, attributes ) && + isBetweenDifferentAttributeValues( position, attributes ) + ) { + preventCaretMovement( data ); + setSelectionAttributesFromTheNodeBefore( model, attributes, position ); + + return true; + } + // When we are moving from natural gravity, to the position of the 2SCM, we need to override the gravity, // and make sure it won't be restored. Unless it's at the end of the block and an observed attribute. // We need to check if the caret is a one position before the attribute boundary: @@ -406,6 +437,68 @@ export default class TwoStepCaretMovement extends Plugin { return false; } + /** + * Starts listening to {@link module:engine/view/document~Document#event:mousedown} and + * {@link module:engine/view/document~Document#event:selectionChange} and puts the selection before/after a 2-step node + * if clicked at the beginning/ending of the 2-step node. + * + * The purpose of this action is to allow typing around the 2-step node directly after a click. + */ + private _enableClickingAfterNode(): void { + const editor = this.editor; + const model = editor.model; + const selection = model.document.selection; + const document = editor.editing.view.document; + + editor.editing.view.addObserver( MouseObserver ); + + let clicked = false; + + // Detect the click. + this.listenTo( document, 'mousedown', () => { + clicked = true; + } ); + + // When the selection has changed... + this.listenTo( document, 'selectionChange', () => { + const attributes = this.attributes; + + if ( !clicked ) { + return; + } + + // ...and it was caused by the click... + clicked = false; + + // ...and no text is selected... + if ( !selection.isCollapsed ) { + return; + } + + // ...and clicked text is the 2-step node... + if ( !hasAnyAttribute( selection, attributes ) ) { + return; + } + + const position = selection.getFirstPosition()!; + + if ( !isBetweenDifferentAttributes( position, attributes ) ) { + return; + } + + // TODO + if ( position.isAtStart ) { + setSelectionAttributesFromTheNodeBefore( model, attributes, position ); + } + else if ( isBetweenDifferentAttributeValues( position, attributes ) ) { + clearDifferentValueSelectionAttributes( model, attributes ); + } + else if ( !this._isGravityOverridden ) { + this._overrideGravity(); + } + } ); + } + /** * `true` when the gravity is overridden for the plugin. */ @@ -458,6 +551,7 @@ function hasAnyAttribute( selection: DocumentSelection, attributes: Set */ function setSelectionAttributesFromTheNodeBefore( model: Model, attributes: Set, position: Position ) { const nodeBefore = position.nodeBefore; + model.change( writer => { if ( nodeBefore ) { const attributes: Array<[string, unknown]> = []; @@ -501,6 +595,7 @@ function isStepAfterAnyAttributeBoundary( position: Position, attributes: Set ): boolean { const { nodeBefore, nodeAfter } = position; + for ( const observedAttribute of attributes ) { const attrBefore = nodeBefore ? nodeBefore.getAttribute( observedAttribute ) : undefined; const attrAfter = nodeAfter ? nodeAfter.getAttribute( observedAttribute ) : undefined; @@ -509,5 +604,33 @@ function isBetweenDifferentAttributes( position: Position, attributes: Set ): boolean { + const { nodeBefore, nodeAfter } = position; + + for ( const observedAttribute of attributes ) { + const attrBefore = nodeBefore ? nodeBefore.getAttribute( observedAttribute ) : undefined; + const attrAfter = nodeAfter ? nodeAfter.getAttribute( observedAttribute ) : undefined; + + if ( attrBefore !== undefined && attrAfter !== undefined && attrAfter !== attrBefore ) { + return true; + } + } + return false; } + +/** + * TODO + */ +function clearDifferentValueSelectionAttributes( model: Model, attributes: Set ) { + model.change( writer => { + writer.removeSelectionAttribute( attributes ); + } ); +} From cab60a445c257459025ad87374704271519e55df Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sat, 18 Nov 2023 21:05:51 +0100 Subject: [PATCH 03/10] Added comments and code cleaning. --- .../src/twostepcaretmovement.ts | 82 +++++++++---------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/packages/ckeditor5-typing/src/twostepcaretmovement.ts b/packages/ckeditor5-typing/src/twostepcaretmovement.ts index d77edebb560..7615558494a 100644 --- a/packages/ckeditor5-typing/src/twostepcaretmovement.ts +++ b/packages/ckeditor5-typing/src/twostepcaretmovement.ts @@ -319,12 +319,16 @@ export default class TwoStepCaretMovement extends Plugin { if ( isBetweenDifferentAttributes( position, attributes ) ) { preventCaretMovement( data ); - // TODO + // CLEAR 2-SCM attributes if we are at the end of one 2-SCM and before + // the next one with a different value of the same attribute. + // + // foo<$text attribute=1>bar{}<$text attribute=2>barbaz + // if ( hasAnyAttribute( selection, attributes ) && - isBetweenDifferentAttributeValues( position, attributes ) + isBetweenDifferentAttributes( position, attributes, true ) ) { - clearDifferentValueSelectionAttributes( model, attributes ); + clearSelectionAttributes( model, attributes ); } else { this._overrideGravity(); } @@ -360,9 +364,13 @@ export default class TwoStepCaretMovement extends Plugin { preventCaretMovement( data ); this._restoreGravity(); - // TODO - if ( isBetweenDifferentAttributeValues( position, attributes ) ) { - clearDifferentValueSelectionAttributes( model, attributes ); + // CLEAR 2-SCM attributes if we are at the end of one 2-SCM and before + // the next one with a different value of the same attribute. + // + // foo<$text attribute=1>bar<$text attribute=2>{}barbaz + // + if ( isBetweenDifferentAttributes( position, attributes, true ) ) { + clearSelectionAttributes( model, attributes ); } else { setSelectionAttributesFromTheNodeBefore( model, attributes, position ); } @@ -385,10 +393,13 @@ export default class TwoStepCaretMovement extends Plugin { return false; } - // TODO + // SET 2-SCM attributes if we are between nodes with the same attribute but with different values. + // + // foo<$text attribute=1>bar[]<$text attribute=2>barbaz + // if ( !hasAnyAttribute( selection, attributes ) && - isBetweenDifferentAttributeValues( position, attributes ) + isBetweenDifferentAttributes( position, attributes, true ) ) { preventCaretMovement( data ); setSelectionAttributesFromTheNodeBefore( model, attributes, position ); @@ -486,12 +497,15 @@ export default class TwoStepCaretMovement extends Plugin { return; } - // TODO - if ( position.isAtStart ) { - setSelectionAttributesFromTheNodeBefore( model, attributes, position ); - } - else if ( isBetweenDifferentAttributeValues( position, attributes ) ) { - clearDifferentValueSelectionAttributes( model, attributes ); + // The selection at the start of a block would use surrounding attributes + // from text after the selection so just clear 2-SCM attributes. + // + // Also, clear attributes for selection between same attribute with different values. + if ( + position.isAtStart || + isBetweenDifferentAttributes( position, attributes, true ) + ) { + clearSelectionAttributes( model, attributes ); } else if ( !this._isGravityOverridden ) { this._overrideGravity(); @@ -573,6 +587,15 @@ function setSelectionAttributesFromTheNodeBefore( model: Model, attributes: Set< } ); } +/** + * Removes 2-SCM attributes from the selection. + */ +function clearSelectionAttributes( model: Model, attributes: Set ) { + model.change( writer => { + writer.removeSelectionAttribute( attributes ); + } ); +} + /** * Prevents the caret movement in the view by calling `preventDefault` on the event data. * @@ -593,44 +616,21 @@ function isStepAfterAnyAttributeBoundary( position: Position, attributes: Set ): boolean { +function isBetweenDifferentAttributes( position: Position, attributes: Set, isStrict: boolean = false ): boolean { const { nodeBefore, nodeAfter } = position; for ( const observedAttribute of attributes ) { const attrBefore = nodeBefore ? nodeBefore.getAttribute( observedAttribute ) : undefined; const attrAfter = nodeAfter ? nodeAfter.getAttribute( observedAttribute ) : undefined; - if ( attrAfter !== attrBefore ) { - return true; + if ( isStrict && ( attrBefore === undefined || attrAfter === undefined ) ) { + continue; } - } - - return false; -} - -/** - * TODO - */ -function isBetweenDifferentAttributeValues( position: Position, attributes: Set ): boolean { - const { nodeBefore, nodeAfter } = position; - - for ( const observedAttribute of attributes ) { - const attrBefore = nodeBefore ? nodeBefore.getAttribute( observedAttribute ) : undefined; - const attrAfter = nodeAfter ? nodeAfter.getAttribute( observedAttribute ) : undefined; - if ( attrBefore !== undefined && attrAfter !== undefined && attrAfter !== attrBefore ) { + if ( attrAfter !== attrBefore ) { return true; } } return false; } - -/** - * TODO - */ -function clearDifferentValueSelectionAttributes( model: Model, attributes: Set ) { - model.change( writer => { - writer.removeSelectionAttribute( attributes ); - } ); -} From e51e10df0e65ee15fc0fb674c98b3fd3e34037e9 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sun, 19 Nov 2023 02:50:43 +0100 Subject: [PATCH 04/10] Fixed 2-SCM integration with insertContent and deleteContent. --- packages/ckeditor5-link/src/linkediting.ts | 286 +------- packages/ckeditor5-link/tests/linkediting.js | 656 +++++++++++++----- .../ckeditor5-typing/src/inserttextcommand.ts | 6 +- .../src/twostepcaretmovement.ts | 113 ++- 4 files changed, 591 insertions(+), 470 deletions(-) diff --git a/packages/ckeditor5-link/src/linkediting.ts b/packages/ckeditor5-link/src/linkediting.ts index 59d2abc39ca..1584fe3ccb3 100644 --- a/packages/ckeditor5-link/src/linkediting.ts +++ b/packages/ckeditor5-link/src/linkediting.ts @@ -12,21 +12,17 @@ import { type Editor } from 'ckeditor5/src/core'; import type { - Model, Schema, Writer, ViewElement, - ModelDeleteContentEvent, - ModelInsertContentEvent, ViewDocumentKeyDownEvent, - ViewDocumentClickEvent + ViewDocumentClickEvent, + DocumentSelectionChangeAttributeEvent } from 'ckeditor5/src/engine'; import { Input, TwoStepCaretMovement, - inlineHighlight, - findAttributeRange, - type ViewDocumentDeleteEvent + inlineHighlight } from 'ckeditor5/src/typing'; import { ClipboardPipeline, @@ -140,14 +136,8 @@ export default class LinkEditing extends Plugin { // Handle link following by CTRL+click or ALT+ENTER this._enableLinkOpen(); - // Change the attributes of the selection in certain situations after the link was inserted into the document. - this._enableInsertContentSelectionAttributesFixer(); - - // Handle typing over the link. - this._enableTypingOverLink(); - - // Handle removing the content after the link element. - this._handleDeleteContentAfterLink(); + // TODO update comment - Change the attributes of the selection in certain situations after the link was inserted into the document. + this._enableSelectionDecoratorAttributesFixer(); // Handle adding default protocol to pasted links. this._enableClipboardIntegration(); @@ -314,228 +304,22 @@ export default class LinkEditing extends Plugin { * The purpose of this action is to improve the overall UX because the user is no longer "trapped" by the * `linkHref` attribute of the selection and they can type a "clean" (`linkHref`–less) text right away. * - * See https://github.com/ckeditor/ckeditor5/issues/6053. + * TODO update description */ - private _enableInsertContentSelectionAttributesFixer(): void { + private _enableSelectionDecoratorAttributesFixer(): void { const editor = this.editor; const model = editor.model; const selection = model.document.selection; - this.listenTo( model, 'insertContent', () => { - const nodeBefore = selection.anchor!.nodeBefore; - const nodeAfter = selection.anchor!.nodeAfter; - - // NOTE: ↰ and ↱ represent the gravity of the selection. - - // The only truly valid case is: - // - // ↰ - // ...<$text linkHref="foo">INSERTED[] - // - // If the selection is not "trapped" by the `linkHref` attribute after inserting, there's nothing - // to fix there. - if ( !selection.hasAttribute( 'linkHref' ) ) { - return; - } - - // Filter out the following case where a link with the same href (e.g. INSERTED) is inserted - // in the middle of an existing link: - // - // Before insertion: - // ↰ - // <$text linkHref="foo">l[]ink - // - // Expected after insertion: - // ↰ - // <$text linkHref="foo">lINSERTED[]ink - // - if ( !nodeBefore ) { - return; - } - - // Filter out the following case where the selection has the "linkHref" attribute because the - // gravity is overridden and some text with another attribute (e.g. INSERTED) is inserted: - // - // Before insertion: - // - // ↱ - // <$text linkHref="foo">[]link - // - // Expected after insertion: - // - // ↱ - // <$text bold="true">INSERTED<$text linkHref="foo">[]link - // - if ( !nodeBefore.hasAttribute( 'linkHref' ) ) { - return; - } - - // Filter out the following case where a link is a inserted in the middle (or before) another link - // (different URLs, so they will not merge). In this (let's say weird) case, we can leave the selection - // attributes as they are because the user will end up writing in one link or another anyway. - // - // Before insertion: - // - // ↰ - // <$text linkHref="foo">l[]ink - // - // Expected after insertion: - // - // ↰ - // <$text linkHref="foo">l<$text linkHref="bar">INSERTED[]<$text linkHref="foo">ink - // - if ( nodeAfter && nodeAfter.hasAttribute( 'linkHref' ) ) { + this.listenTo( selection, 'change:attribute', ( evt, { attributeKeys } ) => { + if ( !attributeKeys.includes( 'linkHref' ) || selection.hasAttribute( 'linkHref' ) ) { return; } model.change( writer => { removeLinkAttributesFromSelection( writer, getLinkAttributesAllowedOnText( model.schema ) ); } ); - }, { priority: 'low' } ); - } - - /** - * Starts listening to {@link module:engine/model/model~Model#deleteContent} and {@link module:engine/model/model~Model#insertContent} - * and checks whether typing over the link. If so, attributes of removed text are preserved and applied to the inserted text. - * - * The purpose of this action is to allow modifying a text without loosing the `linkHref` attribute (and other). - * - * See https://github.com/ckeditor/ckeditor5/issues/4762. - */ - private _enableTypingOverLink(): void { - const editor = this.editor; - const view = editor.editing.view; - - // Selection attributes when started typing over the link. - let selectionAttributes: IterableIterator<[ string, unknown ]> | null = null; - - // Whether pressed `Backspace` or `Delete`. If so, attributes should not be preserved. - let deletedContent = false; - - // Detect pressing `Backspace` / `Delete`. - this.listenTo( view.document, 'delete', () => { - deletedContent = true; - }, { priority: 'high' } ); - - // Listening to `model#deleteContent` allows detecting whether selected content was a link. - // If so, before removing the element, we will copy its attributes. - this.listenTo( editor.model, 'deleteContent', () => { - const selection = editor.model.document.selection; - - // Copy attributes only if anything is selected. - if ( selection.isCollapsed ) { - return; - } - - // When the content was deleted, do not preserve attributes. - if ( deletedContent ) { - deletedContent = false; - - return; - } - - // Enabled only when typing. - if ( !isTyping( editor ) ) { - return; - } - - if ( shouldCopyAttributes( editor.model ) ) { - selectionAttributes = selection.getAttributes(); - } - }, { priority: 'high' } ); - - // Listening to `model#insertContent` allows detecting the content insertion. - // We want to apply attributes that were removed while typing over the link. - this.listenTo( editor.model, 'insertContent', ( evt, [ element ] ) => { - deletedContent = false; - - // Enabled only when typing. - if ( !isTyping( editor ) ) { - return; - } - - if ( !selectionAttributes ) { - return; - } - - editor.model.change( writer => { - for ( const [ attribute, value ] of selectionAttributes! ) { - writer.setAttribute( attribute, value, element ); - } - } ); - - selectionAttributes = null; - }, { priority: 'high' } ); - } - - /** - * Starts listening to {@link module:engine/model/model~Model#deleteContent} and checks whether - * removing a content right after the "linkHref" attribute. - * - * If so, the selection should not preserve the `linkHref` attribute. However, if - * the {@link module:typing/twostepcaretmovement~TwoStepCaretMovement} plugin is active and - * the selection has the "linkHref" attribute due to overriden gravity (at the end), the `linkHref` attribute should stay untouched. - * - * The purpose of this action is to allow removing the link text and keep the selection outside the link. - * - * See https://github.com/ckeditor/ckeditor5/issues/7521. - */ - private _handleDeleteContentAfterLink(): void { - const editor = this.editor; - const model = editor.model; - const selection = model.document.selection; - const view = editor.editing.view; - - // A flag whether attributes `linkHref` attribute should be preserved. - let shouldPreserveAttributes = false; - - // A flag whether the `Backspace` key was pressed. - let hasBackspacePressed = false; - - // Detect pressing `Backspace`. - this.listenTo( view.document, 'delete', ( evt, data ) => { - hasBackspacePressed = data.direction === 'backward'; - }, { priority: 'high' } ); - - // Before removing the content, check whether the selection is inside a link or at the end of link but with 2-SCM enabled. - // If so, we want to preserve link attributes. - this.listenTo( model, 'deleteContent', () => { - // Reset the state. - shouldPreserveAttributes = false; - - const position = selection.getFirstPosition()!; - const linkHref = selection.getAttribute( 'linkHref' ); - - if ( !linkHref ) { - return; - } - - const linkRange = findAttributeRange( position, 'linkHref', linkHref, model ); - - // Preserve `linkHref` attribute if the selection is in the middle of the link or - // the selection is at the end of the link and 2-SCM is activated. - shouldPreserveAttributes = linkRange.containsPosition( position ) || linkRange.end.isEqual( position ); - }, { priority: 'high' } ); - - // After removing the content, check whether the current selection should preserve the `linkHref` attribute. - this.listenTo( model, 'deleteContent', () => { - // If didn't press `Backspace`. - if ( !hasBackspacePressed ) { - return; - } - - hasBackspacePressed = false; - - // Disable the mechanism if inside a link (`<$text url="foo">F[]oo` or <$text url="foo">Foo[]`). - if ( shouldPreserveAttributes ) { - return; - } - - // Use `model.enqueueChange()` in order to execute the callback at the end of the changes process. - editor.model.enqueueChange( writer => { - removeLinkAttributesFromSelection( writer, getLinkAttributesAllowedOnText( model.schema ) ); - } ); - }, { priority: 'low' } ); + } ); } /** @@ -579,56 +363,6 @@ function removeLinkAttributesFromSelection( writer: Writer, linkAttributes: Arra } } -/** - * Checks whether selection's attributes should be copied to the new inserted text. - */ -function shouldCopyAttributes( model: Model ): boolean { - const selection = model.document.selection; - const firstPosition = selection.getFirstPosition()!; - const lastPosition = selection.getLastPosition()!; - const nodeAtFirstPosition = firstPosition.nodeAfter; - - // The text link node does not exist... - if ( !nodeAtFirstPosition ) { - return false; - } - - // ...or it isn't the text node... - if ( !nodeAtFirstPosition.is( '$text' ) ) { - return false; - } - - // ...or isn't the link. - if ( !nodeAtFirstPosition.hasAttribute( 'linkHref' ) ) { - return false; - } - - // `textNode` = the position is inside the link element. - // `nodeBefore` = the position is at the end of the link element. - const nodeAtLastPosition = lastPosition.textNode || lastPosition.nodeBefore; - - // If both references the same node selection contains a single text node. - if ( nodeAtFirstPosition === nodeAtLastPosition ) { - return true; - } - - // If nodes are not equal, maybe the link nodes has defined additional attributes inside. - // First, we need to find the entire link range. - const linkRange = findAttributeRange( firstPosition, 'linkHref', nodeAtFirstPosition.getAttribute( 'linkHref' ), model ); - - // Then we can check whether selected range is inside the found link range. If so, attributes should be preserved. - return linkRange.containsRange( model.createRange( firstPosition, lastPosition ), true ); -} - -/** - * Checks whether provided changes were caused by typing. - */ -function isTyping( editor: Editor ): boolean { - const currentBatch = editor.model.change( writer => writer.batch ); - - return currentBatch.isTyping; -} - /** * Returns an array containing names of the attributes allowed on `$text` that describes the link item. */ diff --git a/packages/ckeditor5-link/tests/linkediting.js b/packages/ckeditor5-link/tests/linkediting.js index 0f29582a1cb..fe031f12732 100644 --- a/packages/ckeditor5-link/tests/linkediting.js +++ b/packages/ckeditor5-link/tests/linkediting.js @@ -221,16 +221,16 @@ describe( 'LinkEditing', () => { expect( getModelData( model ) ).to.equal( '' + 'foo' + - '<$text bold="true">INSERTED' + - '<$text linkHref="ckeditor.com">[]bar' + + '<$text bold="true">INSERTED[]' + + '<$text linkHref="ckeditor.com">bar' + '' ); expect( model.document.selection ).to.have.property( 'isGravityOverridden', true ); - expect( model.document.selection ).to.have.attribute( 'linkHref' ); + expect( [ ...model.document.selection.getAttributeKeys() ] ).to.be.empty; } ); - it( 'should not remove link attributes when pasting a link into another link (different URLs, no merge)', () => { + it( 'should remove link attributes when pasting a link into another link (different URLs, no merge)', () => { setModelData( model, '<$text linkHref="ckeditor.com">f[]oo' ); model.change( writer => { @@ -240,12 +240,12 @@ describe( 'LinkEditing', () => { expect( getModelData( model ) ).to.equal( '' + '<$text linkHref="ckeditor.com">f' + - '<$text linkHref="http://INSERTED">INSERTED[]' + + '<$text linkHref="http://INSERTED">INSERTED[]' + '<$text linkHref="ckeditor.com">oo' + '' ); - expect( model.document.selection ).to.have.attribute( 'linkHref' ); + expect( [ ...model.document.selection.getAttributeKeys() ] ).to.be.empty; } ); it( 'should not remove link attributes when pasting before another link (different URLs, no merge)', () => { @@ -259,13 +259,12 @@ describe( 'LinkEditing', () => { expect( getModelData( model ) ).to.equal( '' + - '<$text linkHref="http://INSERTED">INSERTED[]' + + '<$text linkHref="http://INSERTED">INSERTED[]' + '<$text linkHref="ckeditor.com">foo' + '' ); - expect( model.document.selection ).to.have.attribute( 'linkHref' ); - expect( model.document.selection ).to.have.attribute( 'linkHref', 'http://INSERTED' ); + expect( [ ...model.document.selection.getAttributeKeys() ] ).to.be.empty; } ); // https://github.com/ckeditor/ckeditor5/issues/8158 @@ -1358,7 +1357,7 @@ describe( 'LinkEditing', () => { beforeEach( async () => { editor = await ClassicTestEditor.create( element, { - plugins: [ Paragraph, LinkEditing, Enter, BoldEditing, ItalicEditing, ImageBlockEditing ], + plugins: [ Paragraph, LinkEditing, Enter, Delete, BoldEditing, ItalicEditing, ImageBlockEditing ], link: { decorators: { isFoo: { @@ -1400,23 +1399,25 @@ describe( 'LinkEditing', () => { expect( LinkEditing.requires.includes( Input ) ).to.equal( true ); } ); - it( 'should preserve selection attributes when the entire link is selected', () => { - setModelData( model, - 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' - ); + describe( 'insertText with specific selection (not DocumentSelection, beforeinput)', () => { + it( 'should preserve selection attributes when the entire link is selected', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); - editor.execute( 'insertText', { - text: 'Abcde' - } ); + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); - expect( getModelData( model ) ).to.equal( - 'This is <$text linkHref="foo">Abcde[] from <$text linkHref="bar">Bar.' - ); - } ); + expect( getModelData( model ) ).to.equal( + 'This is <$text linkHref="foo">Abcde[] from <$text linkHref="bar">Bar.' + ); + } ); - it( 'should preserve selection attributes when the entire link is selected (mixed attributes in the link)', () => { - setModelData( model, - '' + + it( 'should preserve selection attributes when the entire link is selected (mixed attributes in the link)', () => { + setModelData( model, + '' + 'This is [' + '<$text linkHref="foo" italic="true">F' + '<$text linkHref="foo" bold="true">o' + @@ -1427,42 +1428,43 @@ describe( 'LinkEditing', () => { ' from ' + '<$text linkHref="bar">Bar' + '.' + - '' - ); + '' + ); - editor.execute( 'insertText', { - text: 'Abcde' - } ); + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); - expect( getModelData( model ) ).to.equal( - '' + + expect( getModelData( model ) ).to.equal( + '' + 'This is ' + - '<$text italic="true" linkHref="foo">Abcde' + - '<$text italic="true">[]' + + '<$text italic="true" linkHref="foo">Abcde[]' + ' from ' + '<$text linkHref="bar">Bar' + '.' + - '' - ); - } ); + '' + ); + } ); - it( 'should preserve selection attributes when the selection starts at the beginning of the link', () => { - setModelData( model, - 'This is [<$text linkHref="foo">Fo]o from <$text linkHref="bar">Bar.' - ); + it( 'should preserve selection attributes when the selection starts at the beginning of the link', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Fo]o from <$text linkHref="bar">Bar.' + ); - editor.execute( 'insertText', { - text: 'Abcde' - } ); + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); - expect( getModelData( model ) ).to.equal( - 'This is <$text linkHref="foo">Abcde[]o from <$text linkHref="bar">Bar.' - ); - } ); + expect( getModelData( model ) ).to.equal( + 'This is <$text linkHref="foo">Abcde[]o from <$text linkHref="bar">Bar.' + ); + } ); - it( 'should preserve selection attributes when it starts at the beginning of the link (mixed attributes in the link)', () => { - setModelData( model, - '' + + it( 'should preserve selection attributes when it starts at the beginning of the link (mixed attributes in the link)', () => { + setModelData( model, + '' + 'This is [' + '<$text linkHref="foo" italic="true">F' + '<$text linkHref="foo" bold="true">o' + @@ -1473,213 +1475,483 @@ describe( 'LinkEditing', () => { ' from ' + '<$text linkHref="bar">Bar' + '.' + - '' - ); + '' + ); - editor.execute( 'insertText', { - text: 'Abcde' - } ); + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); - expect( getModelData( model ) ).to.equal( - '' + + expect( getModelData( model ) ).to.equal( + '' + 'This is ' + '<$text italic="true" linkHref="foo">Abcde[]' + '<$text linkHref="foo">r' + ' from ' + '<$text linkHref="bar">Bar' + '.' + - '' - ); - } ); + '' + ); + } ); - it( 'should preserve all attributes of the link node (decorators check)', () => { - setModelData( model, - '' + + it( 'should preserve all attributes of the link node (decorators check)', () => { + setModelData( model, + '' + 'This is ' + '<$text linkIsFoo="true" linkIsBar="true" linkHref="foo">[Foo]' + ' from ' + '<$text linkHref="bar">Bar' + '.' + - '' - ); + '' + ); - editor.execute( 'insertText', { - text: 'Abcde' - } ); + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); - expect( getModelData( model ) ).to.equal( - '' + + expect( getModelData( model ) ).to.equal( + '' + 'This is ' + - '<$text linkHref="foo" linkIsBar="true" linkIsFoo="true">Abcde[]' + + '<$text linkHref="foo" linkIsBar="true" linkIsFoo="true">Abcde[]' + ' from ' + '<$text linkHref="bar">Bar' + '.' + - '' - ); - } ); + '' + ); + } ); - it( 'should not preserve selection attributes when the changes are not caused by typing', () => { - setModelData( model, - 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' - ); + it( 'should not preserve selection attributes when the changes are not caused by typing', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); - model.change( writer => { - model.deleteContent( model.document.selection ); - model.insertContent( writer.createText( 'Abcde' ) ); + model.change( writer => { + model.deleteContent( model.document.selection ); + model.insertContent( writer.createText( 'Abcde' ) ); + } ); + + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); } ); - expect( getModelData( model ) ).to.equal( - 'This is Abcde[] from <$text linkHref="bar">Bar.' - ); - } ); + it( 'should not preserve selection attributes when the changes are not caused by typing (pasting check)', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); - it( 'should not preserve selection attributes when the changes are not caused by typing (pasting check)', () => { - setModelData( model, - 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' - ); + view.document.fire( 'paste', { + dataTransfer: createDataTransfer( { + 'text/html': '

Abcde

', + 'text/plain': 'Abcde' + } ), + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + } ); - view.document.fire( 'paste', { - dataTransfer: createDataTransfer( { - 'text/html': '

Abcde

', - 'text/plain': 'Abcde' - } ), - preventDefault: sinon.spy(), - stopPropagation: sinon.spy() + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); } ); - expect( getModelData( model ) ).to.equal( - 'This is Abcde[] from <$text linkHref="bar">Bar.' - ); - } ); + it( 'should not preserve selection attributes when typed after cutting the content', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); - it( 'should not preserve selection attributes when typed after cutting the content', () => { - setModelData( model, - 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' - ); + view.document.fire( 'cut', { + dataTransfer: createDataTransfer(), + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + } ); - view.document.fire( 'cut', { - dataTransfer: createDataTransfer(), - preventDefault: sinon.spy(), - stopPropagation: sinon.spy() + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); + + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); } ); - editor.execute( 'insertText', { - text: 'Abcde' + it( 'should not preserve anything if selected an element instead of text', () => { + setModelData( model, + '[]' + ); + + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); + + expect( getModelData( model ) ).to.equal( + 'Abcde[]' + ); } ); - expect( getModelData( model ) ).to.equal( - 'This is Abcde[] from <$text linkHref="bar">Bar.' - ); - } ); + it( 'should not preserve selection attributes when the entire link is selected and pressed "Backspace"', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); - it( 'should not preserve anything if selected an element instead of text', () => { - setModelData( model, - '[]' - ); + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => { + } + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); - editor.execute( 'insertText', { - text: 'Abcde' + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); } ); - expect( getModelData( model ) ).to.equal( - 'Abcde[]' - ); - } ); + it( 'should not preserve selection attributes when the entire link is selected and pressed "Delete"', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); - it( 'should not preserve anything if selected text does not have the `linkHref` attribute`', () => { - setModelData( model, - 'This is [<$text bold="foo">Foo] from <$text linkHref="bar">Bar.' - ); + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => { + } + }, { + direction: 'forward', + selectionToRemove: view.document.selection + } ) ); + + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); - editor.execute( 'insertText', { - text: 'Abcde' + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); } ); - expect( getModelData( model ) ).to.equal( - 'This is Abcde[] from <$text linkHref="bar">Bar.' - ); - } ); + it( 'should preserve selection attributes (from first link) when selected different links', () => { + setModelData( model, + 'This is <$text linkHref="foo">[Foo from <$text linkHref="bar">Bar].' + ); - it( 'should not preserve selection attributes when the entire link is selected and pressed "Backspace"', () => { - setModelData( model, - 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' - ); + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); - view.document.fire( 'delete', new DomEventData( view.document, { - preventDefault: () => {} - }, { - direction: 'backward', - selectionToRemove: view.document.selection - } ) ); + expect( getModelData( model ) ).to.equal( 'This is <$text linkHref="foo">Abcde[].' ); + } ); + + it( 'should not preserve selection attributes when selected more than single link (start of the selection)', () => { + setModelData( model, + 'This is[ <$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); + + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); - editor.execute( 'insertText', { - text: 'Abcde' + expect( getModelData( model ) ).to.equal( + 'This isAbcde[] from <$text linkHref="bar">Bar.' + ); } ); - expect( getModelData( model ) ).to.equal( - 'This is Abcde[] from <$text linkHref="bar">Bar.' - ); + it( 'should preserve selection attributes when selected more than single link (end of the selection)', () => { + setModelData( model, + 'This is <$text linkHref="foo">[Foo ]from <$text linkHref="bar">Bar.' + ); + + editor.execute( 'insertText', { + text: 'Abcde', + selection: model.createSelection( model.document.selection ) + } ); + + expect( getModelData( model ) ).to.equal( + 'This is <$text linkHref="foo">Abcde[]from <$text linkHref="bar">Bar.' + ); + } ); } ); - it( 'should not preserve selection attributes when the entire link is selected and pressed "Delete"', () => { - setModelData( model, - 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' - ); + describe( 'insertText with default selection (DocumentSelection, not beforeinput)', () => { + it( 'should preserve selection attributes when the entire link is selected', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); - view.document.fire( 'delete', new DomEventData( view.document, { - preventDefault: () => {} - }, { - direction: 'forward', - selectionToRemove: view.document.selection - } ) ); + editor.execute( 'insertText', { + text: 'Abcde' + } ); - editor.execute( 'insertText', { - text: 'Abcde' + expect( getModelData( model ) ).to.equal( + 'This is <$text linkHref="foo">Abcde[] from <$text linkHref="bar">Bar.' + ); } ); - expect( getModelData( model ) ).to.equal( - 'This is Abcde[] from <$text linkHref="bar">Bar.' - ); - } ); + it( 'should preserve selection attributes when the entire link is selected (mixed attributes in the link)', () => { + setModelData( model, + '' + + 'This is [' + + '<$text linkHref="foo" italic="true">F' + + '<$text linkHref="foo" bold="true">o' + + '<$text linkHref="foo" bold="true" italic="true">o' + + '<$text linkHref="foo" bold="true">B' + + '<$text linkHref="foo" bold="true" italic="true">a' + + '<$text linkHref="foo">r]' + + ' from ' + + '<$text linkHref="bar">Bar' + + '.' + + '' + ); - it( 'should not preserve selection attributes when selected different links', () => { - setModelData( model, - 'This is <$text linkHref="foo">[Foo from <$text linkHref="bar">Bar].' - ); + editor.execute( 'insertText', { + text: 'Abcde' + } ); - editor.execute( 'insertText', { - text: 'Abcde' + expect( getModelData( model ) ).to.equal( + '' + + 'This is ' + + '<$text italic="true" linkHref="foo">Abcde' + + '<$text italic="true">[]' + + ' from ' + + '<$text linkHref="bar">Bar' + + '.' + + '' + ); } ); - expect( getModelData( model ) ).to.equal( 'This is Abcde[].' ); - } ); + it( 'should preserve selection attributes when the selection starts at the beginning of the link', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Fo]o from <$text linkHref="bar">Bar.' + ); - it( 'should not preserve selection attributes when selected more than single link (start of the selection)', () => { - setModelData( model, - 'This is[ <$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' - ); + editor.execute( 'insertText', { + text: 'Abcde' + } ); - editor.execute( 'insertText', { - text: 'Abcde' + expect( getModelData( model ) ).to.equal( + 'This is <$text linkHref="foo">Abcde[]o from <$text linkHref="bar">Bar.' + ); } ); - expect( getModelData( model ) ).to.equal( - 'This isAbcde[] from <$text linkHref="bar">Bar.' - ); - } ); + it( 'should preserve selection attributes when it starts at the beginning of the link (mixed attributes in the link)', () => { + setModelData( model, + '' + + 'This is [' + + '<$text linkHref="foo" italic="true">F' + + '<$text linkHref="foo" bold="true">o' + + '<$text linkHref="foo" bold="true" italic="true">o' + + '<$text linkHref="foo" bold="true">B' + + '<$text linkHref="foo" bold="true" italic="true">a]' + + '<$text linkHref="foo">r' + + ' from ' + + '<$text linkHref="bar">Bar' + + '.' + + '' + ); - it( 'should not preserve selection attributes when selected more than single link (end of the selection)', () => { - setModelData( model, - 'This is <$text linkHref="foo">[Foo ]from <$text linkHref="bar">Bar.' - ); + editor.execute( 'insertText', { + text: 'Abcde' + } ); - editor.execute( 'insertText', { - text: 'Abcde' + expect( getModelData( model ) ).to.equal( + '' + + 'This is ' + + '<$text italic="true" linkHref="foo">Abcde[]' + + '<$text linkHref="foo">r' + + ' from ' + + '<$text linkHref="bar">Bar' + + '.' + + '' + ); } ); - expect( getModelData( model ) ).to.equal( - 'This is Abcde[]from <$text linkHref="bar">Bar.' - ); + it( 'should preserve all attributes of the link node (decorators check)', () => { + setModelData( model, + '' + + 'This is ' + + '<$text linkIsFoo="true" linkIsBar="true" linkHref="foo">[Foo]' + + ' from ' + + '<$text linkHref="bar">Bar' + + '.' + + '' + ); + + editor.execute( 'insertText', { + text: 'Abcde' + } ); + + expect( getModelData( model ) ).to.equal( + '' + + 'This is ' + + '<$text linkHref="foo" linkIsBar="true" linkIsFoo="true">Abcde[]' + + ' from ' + + '<$text linkHref="bar">Bar' + + '.' + + '' + ); + } ); + + it( 'should not preserve selection attributes when the changes are not caused by typing', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); + + model.change( writer => { + model.deleteContent( model.document.selection ); + model.insertContent( writer.createText( 'Abcde' ) ); + } ); + + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); + } ); + + it( 'should not preserve selection attributes when the changes are not caused by typing (pasting check)', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); + + view.document.fire( 'paste', { + dataTransfer: createDataTransfer( { + 'text/html': '

Abcde

', + 'text/plain': 'Abcde' + } ), + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + } ); + + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); + } ); + + it( 'should not preserve selection attributes when typed after cutting the content', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); + + view.document.fire( 'cut', { + dataTransfer: createDataTransfer(), + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + } ); + + editor.execute( 'insertText', { + text: 'Abcde' + } ); + + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); + } ); + + it( 'should not preserve anything if selected an element instead of text', () => { + setModelData( model, + '[]' + ); + + editor.execute( 'insertText', { + text: 'Abcde' + } ); + + expect( getModelData( model ) ).to.equal( + 'Abcde[]' + ); + } ); + + it( 'should not preserve selection attributes when the entire link is selected and pressed "Backspace"', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => { + } + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + editor.execute( 'insertText', { + text: 'Abcde' + } ); + + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); + } ); + + it( 'should not preserve selection attributes when the entire link is selected and pressed "Delete"', () => { + setModelData( model, + 'This is [<$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => { + } + }, { + direction: 'forward', + selectionToRemove: view.document.selection + } ) ); + + editor.execute( 'insertText', { + text: 'Abcde' + } ); + + expect( getModelData( model ) ).to.equal( + 'This is Abcde[] from <$text linkHref="bar">Bar.' + ); + } ); + + it( 'should preserve selection attributes (from first link) when selected different links', () => { + setModelData( model, + 'This is <$text linkHref="foo">[Foo from <$text linkHref="bar">Bar].' + ); + + editor.execute( 'insertText', { + text: 'Abcde' + } ); + + expect( getModelData( model ) ).to.equal( 'This is <$text linkHref="foo">Abcde[].' ); + } ); + + it( 'should not preserve selection attributes when selected more than single link (start of the selection)', () => { + setModelData( model, + 'This is[ <$text linkHref="foo">Foo] from <$text linkHref="bar">Bar.' + ); + + editor.execute( 'insertText', { + text: 'Abcde' + } ); + + expect( getModelData( model ) ).to.equal( + 'This isAbcde[] from <$text linkHref="bar">Bar.' + ); + } ); + + it( 'should preserve selection attributes when selected more than single link (end of the selection)', () => { + setModelData( model, + 'This is <$text linkHref="foo">[Foo ]from <$text linkHref="bar">Bar.' + ); + + editor.execute( 'insertText', { + text: 'Abcde' + } ); + + expect( getModelData( model ) ).to.equal( + 'This is <$text linkHref="foo">Abcde[]from <$text linkHref="bar">Bar.' + ); + } ); } ); } ); diff --git a/packages/ckeditor5-typing/src/inserttextcommand.ts b/packages/ckeditor5-typing/src/inserttextcommand.ts index 168a4502876..2c7fe88e0f3 100644 --- a/packages/ckeditor5-typing/src/inserttextcommand.ts +++ b/packages/ckeditor5-typing/src/inserttextcommand.ts @@ -85,10 +85,14 @@ export default class InsertTextCommand extends Command { model.enqueueChange( this._buffer.batch, writer => { this._buffer.lock(); + // Store selection attributes before deleting old content to preserve formatting and link. + // This unifies the behavior between DocumentSelection and Selection provided as input option. + const selectionAttributes = Array.from( doc.selection.getAttributes() ); + model.deleteContent( selection ); if ( text ) { - model.insertContent( writer.createText( text, doc.selection.getAttributes() ), selection ); + model.insertContent( writer.createText( text, selectionAttributes ), selection ); } if ( resultRange ) { diff --git a/packages/ckeditor5-typing/src/twostepcaretmovement.ts b/packages/ckeditor5-typing/src/twostepcaretmovement.ts index 7615558494a..99c2e40c209 100644 --- a/packages/ckeditor5-typing/src/twostepcaretmovement.ts +++ b/packages/ckeditor5-typing/src/twostepcaretmovement.ts @@ -20,9 +20,13 @@ import { type Position, type ViewDocumentArrowKeyEvent, type ViewDocumentMouseDownEvent, - type ViewDocumentSelectionChangeEvent + type ViewDocumentSelectionChangeEvent, + type ModelInsertContentEvent, + type ModelDeleteContentEvent } from '@ckeditor/ckeditor5-engine'; +import type { ViewDocumentDeleteEvent } from './deleteobserver'; + /** * This plugin enables the two-step caret (phantom) movement behavior for * {@link module:typing/twostepcaretmovement~TwoStepCaretMovement#registerAttribute registered attributes} @@ -260,7 +264,14 @@ export default class TwoStepCaretMovement extends Plugin { this._restoreGravity(); } ); + // Handle a click at the beginning/end of a two-step element. this._enableClickingAfterNode(); + + // Change the attributes of the selection in certain situations after the two-step node was inserted into the document. + this._enableInsertContentSelectionAttributesFixer(); + + // Handle removing the content after the two-step node. + this._handleDeleteContentAfterNode(); } /** @@ -454,6 +465,8 @@ export default class TwoStepCaretMovement extends Plugin { * if clicked at the beginning/ending of the 2-step node. * * The purpose of this action is to allow typing around the 2-step node directly after a click. + * + * See https://github.com/ckeditor/ckeditor5/issues/1016. */ private _enableClickingAfterNode(): void { const editor = this.editor; @@ -513,6 +526,104 @@ export default class TwoStepCaretMovement extends Plugin { } ); } + /** + * Starts listening to {@link module:engine/model/model~Model#event:insertContent} and corrects the model + * selection attributes if the selection is at the end of a two-step node after inserting the content. + * + * The purpose of this action is to improve the overall UX because the user is no longer "trapped" by the + * two-step attribute of the selection, and they can type a "clean" (`linkHref`–less) text right away. + * + * See https://github.com/ckeditor/ckeditor5/issues/6053. + */ + private _enableInsertContentSelectionAttributesFixer(): void { + const editor = this.editor; + const model = editor.model; + const selection = model.document.selection; + const attributes = this.attributes; + + this.listenTo( model, 'insertContent', () => { + const position = selection.getFirstPosition()!; + + if ( + hasAnyAttribute( selection, attributes ) && + isBetweenDifferentAttributes( position, attributes ) + ) { + clearSelectionAttributes( model, attributes ); + } + }, { priority: 'low' } ); + } + + /** + * Starts listening to {@link module:engine/model/model~Model#deleteContent} and checks whether + * removing a content right after the tow-step attribute. + * + * If so, the selection should not preserve the two-step attribute. However, if + * the {@link module:typing/twostepcaretmovement~TwoStepCaretMovement} plugin is active and + * the selection has the two-step attribute due to overridden gravity (at the end), the two-step attribute should stay untouched. + * + * The purpose of this action is to allow removing the link text and keep the selection outside the link. + * + * See https://github.com/ckeditor/ckeditor5/issues/7521. + */ + private _handleDeleteContentAfterNode(): void { + const editor = this.editor; + const model = editor.model; + const selection = model.document.selection; + const view = editor.editing.view; + + let isBackspace = false; + let shouldPreserveAttributes = false; + + // Detect pressing `Backspace`. + this.listenTo( view.document, 'delete', ( evt, data ) => { + isBackspace = data.direction === 'backward'; + }, { priority: 'high' } ); + + // Before removing the content, check whether the selection is inside a two-step attribute. + // If so, we want to preserve those attributes. + this.listenTo( model, 'deleteContent', () => { + if ( !isBackspace ) { + return; + } + + const position = selection.getFirstPosition()!; + + shouldPreserveAttributes = hasAnyAttribute( selection, this.attributes ) && + !isStepAfterAnyAttributeBoundary( position, this.attributes ); + }, { priority: 'high' } ); + + // After removing the content, check whether the current selection should preserve the `linkHref` attribute. + this.listenTo( model, 'deleteContent', () => { + if ( !isBackspace ) { + return; + } + + isBackspace = false; + + // Do not escape two-step attribute if it was inside it before content deletion. + if ( shouldPreserveAttributes ) { + return; + } + + // Use `model.enqueueChange()` in order to execute the callback at the end of the changes process. + editor.model.enqueueChange( () => { + const position = selection.getFirstPosition()!; + + if ( + hasAnyAttribute( selection, this.attributes ) && + isBetweenDifferentAttributes( position, this.attributes ) + ) { + if ( position.isAtStart || isBetweenDifferentAttributes( position, this.attributes, true ) ) { + clearSelectionAttributes( model, this.attributes ); + } + else if ( !this._isGravityOverridden ) { + this._overrideGravity(); + } + } + } ); + }, { priority: 'low' } ); + } + /** * `true` when the gravity is overridden for the plugin. */ From 1c33d23df0a60be1377cb75487572e1e83a3f7f3 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sun, 19 Nov 2023 12:56:31 +0100 Subject: [PATCH 05/10] Update tests for 2 sibling two-step nodes. --- .../tests/twostepcaretmovement.js | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-typing/tests/twostepcaretmovement.js b/packages/ckeditor5-typing/tests/twostepcaretmovement.js index 355760628e6..eccdec533e7 100644 --- a/packages/ckeditor5-typing/tests/twostepcaretmovement.js +++ b/packages/ckeditor5-typing/tests/twostepcaretmovement.js @@ -114,14 +114,21 @@ describe( 'TwoStepCaretMovement()', () => { it( 'should use two-steps movement when between nodes with the same attribute but different value', () => { setData( model, '<$text a="1">bar[]<$text a="2">foo' ); + expect( selection ).to.have.attribute( 'a', 1 ); testTwoStepCaretMovement( [ { selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0, evtStop: 0, caretPosition: 3 }, '→', + // <$text a="1">bar[]<$text a="2">foo + { selectionAttributes: [], isGravityOverridden: false, preventDefault: 1, evtStop: 1, caretPosition: 3 }, + '→', // <$text a="1">bar<$text a="2">[]foo - { selectionAttributes: [ 'a' ], isGravityOverridden: true, preventDefault: 1, evtStop: 1, caretPosition: 3 }, + { selectionAttributes: [ 'a' ], isGravityOverridden: true, preventDefault: 2, evtStop: 2, caretPosition: 3 } + ] ); + expect( selection ).to.have.attribute( 'a', 2 ); + testTwoStepCaretMovement( [ '→', // <$text a="1">bar<$text a="2">f[]oo - { selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 1, evtStop: 1, caretPosition: 4 } + { selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 2, evtStop: 2, caretPosition: 4 } ] ); } ); @@ -319,20 +326,24 @@ describe( 'TwoStepCaretMovement()', () => { it( 'should require two-steps movement when caret goes between text node with the same attribute but different value', () => { setData( model, '<$text a="2">foo<$text a="1">b[]ar' ); + expect( selection ).to.have.attribute( 'a', 1 ); testTwoStepCaretMovement( [ { selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0, evtStop: 0, caretPosition: 4 }, '←', // <$text a="2">foo<$text a="1">[]bar { selectionAttributes: [ 'a' ], isGravityOverridden: true, preventDefault: 0, evtStop: 0, caretPosition: 3 }, '←', + // <$text a="2">foo[]<$text a="1">bar + { selectionAttributes: [], isGravityOverridden: false, preventDefault: 1, evtStop: 1, caretPosition: 3 }, + '←', // <$text a="2">foo[]<$text a="1">bar - { selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 1, evtStop: 1, caretPosition: 3 } + { selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 2, evtStop: 2, caretPosition: 3 } ] ); expect( selection ).to.have.attribute( 'a', 2 ); testTwoStepCaretMovement( [ '←', // <$text a="2">fo[]o<$text a="1">bar - { selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 1, evtStop: 1, caretPosition: 2 } + { selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 2, evtStop: 2, caretPosition: 2 } ] ); } ); From d60c5c12e87eafb0e8cfdbec708fe377ba0dec9c Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sun, 19 Nov 2023 14:11:57 +0100 Subject: [PATCH 06/10] Code cleaning. --- packages/ckeditor5-typing/src/twostepcaretmovement.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-typing/src/twostepcaretmovement.ts b/packages/ckeditor5-typing/src/twostepcaretmovement.ts index 99c2e40c209..95cdc50c68f 100644 --- a/packages/ckeditor5-typing/src/twostepcaretmovement.ts +++ b/packages/ckeditor5-typing/src/twostepcaretmovement.ts @@ -14,7 +14,7 @@ import { keyCodes } from '@ckeditor/ckeditor5-utils'; import { MouseObserver, type DocumentSelection, - type DocumentSelectionChangeEvent, + type DocumentSelectionChangeRangeEvent, type DomEventData, type Model, type Position, @@ -168,7 +168,7 @@ export default class TwoStepCaretMovement extends Plugin { * {@link module:engine/model/selection~Selection#event:change:range} event. */ - private _isNextGravityRestorationSkipped!: boolean; + private _isNextGravityRestorationSkipped = false; /** * @inheritDoc @@ -238,7 +238,7 @@ export default class TwoStepCaretMovement extends Plugin { this._isNextGravityRestorationSkipped = false; // The automatic gravity restoration logic. - this.listenTo( modelSelection, 'change:range', ( evt, data ) => { + this.listenTo( modelSelection, 'change:range', ( evt, data ) => { // Skipping the automatic restoration is needed if the selection should change // but the gravity must remain overridden afterwards. See the #handleBackwardMovement // to learn more. @@ -443,6 +443,7 @@ export default class TwoStepCaretMovement extends Plugin { return true; } + // Skip the automatic gravity restore upon the next selection#change:range event. // If not skipped, it would automatically restore the gravity, which should remain // overridden. From 30e571824135eb3d8dfdafb889ec8a46e9952784 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sun, 19 Nov 2023 14:51:39 +0100 Subject: [PATCH 07/10] Added tests. --- .../tests/datafilter.js | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/packages/ckeditor5-html-support/tests/datafilter.js b/packages/ckeditor5-html-support/tests/datafilter.js index fd911058612..52af63bab16 100644 --- a/packages/ckeditor5-html-support/tests/datafilter.js +++ b/packages/ckeditor5-html-support/tests/datafilter.js @@ -4045,6 +4045,110 @@ describe( 'DataFilter', () => { '

' ); } ); + + it( 'should remove GHS selection attribute for the same range as a coupled feature attribute was removed', () => { + dataFilter.loadAllowedConfig( [ { + name: /^.*$/, + styles: true, + attributes: true, + classes: true + } ] ); + + editor.setData( '

foobar

' ); + + expect( getModelDataWithAttributes( model ) ).to.deep.equal( { + data: '<$text htmlA="(1)" linkHref="foo">[]foobar', + attributes: { + 1: { + classes: [ 'bar' ] + } + } + } ); + + expect( model.document.selection.getAttribute( 'linkHref' ) ).to.deep.equal( 'foo' ); + expect( model.document.selection.getAttribute( 'htmlA' ) ).to.deep.equal( { classes: [ 'bar' ] } ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + + model.change( writer => { + writer.removeSelectionAttribute( 'linkHref' ); + } ); + + expect( getModelDataWithAttributes( model ) ).to.deep.equal( { + data: '[]<$text htmlA="(1)" linkHref="foo">foobar', + attributes: { + 1: { + classes: [ 'bar' ] + } + } + } ); + + expect( model.document.selection.getAttribute( 'linkHref' ) ).to.be.undefined; + expect( model.document.selection.getAttribute( 'htmlA' ) ).to.be.undefined; + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + it( 'should not remove other GHS selection attribute when other coupled one is removed', () => { + dataFilter.loadAllowedConfig( [ { + name: /^.*$/, + styles: true, + attributes: true, + classes: true + } ] ); + + editor.setData( '

foobar

' ); + + expect( getModelDataWithAttributes( model ) ).to.deep.equal( { + data: '<$text fontColor="red" htmlSpan="(1)" htmlStrong="(2)">[]foobar', + attributes: { + 1: { + styles: { + 'text-transform': 'uppercase' + } + }, + 2: {} + } + } ); + + expect( model.document.selection.getAttribute( 'fontColor' ) ).to.deep.equal( 'red' ); + expect( model.document.selection.getAttribute( 'htmlSpan' ) ).to.deep.equal( { styles: { 'text-transform': 'uppercase' } } ); + expect( model.document.selection.getAttribute( 'htmlStrong' ) ).to.deep.equal( {} ); + + expect( editor.getData() ).to.equal( + '

foobar

' + ); + + model.change( writer => { + writer.removeSelectionAttribute( 'fontColor' ); + } ); + + expect( getModelDataWithAttributes( model ) ).to.deep.equal( { + data: + '' + + '<$text htmlSpan="(1)" htmlStrong="(2)">[]' + + '<$text fontColor="red" htmlSpan="(3)" htmlStrong="(4)">foobar' + + '', + attributes: { + 1: { + styles: { + 'text-transform': 'uppercase' + } + }, + 2: {} + } + } ); + + expect( model.document.selection.getAttribute( 'fontColor' ) ).to.be.undefined; + expect( model.document.selection.getAttribute( 'htmlSpan' ) ).to.deep.equal( { styles: { 'text-transform': 'uppercase' } } ); + expect( model.document.selection.getAttribute( 'htmlStrong' ) ).to.deep.equal( {} ); + + expect( editor.getData() ).to.equal( + '

' + + 'foobar' + + '

' + ); + } ); } ); describe( 'loadAllowedEmptyElementsConfig', () => { From 3e958f6ec9cb91f1cb1004d94a09687d4b42b436 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 20 Nov 2023 14:49:28 +0100 Subject: [PATCH 08/10] Added tests. --- .../src/twostepcaretmovement.ts | 2 - .../tests/twostepcaretmovement.js | 449 +++++++++++++++++- 2 files changed, 446 insertions(+), 5 deletions(-) diff --git a/packages/ckeditor5-typing/src/twostepcaretmovement.ts b/packages/ckeditor5-typing/src/twostepcaretmovement.ts index 95cdc50c68f..b341c3614c8 100644 --- a/packages/ckeditor5-typing/src/twostepcaretmovement.ts +++ b/packages/ckeditor5-typing/src/twostepcaretmovement.ts @@ -235,8 +235,6 @@ export default class TwoStepCaretMovement extends Plugin { } }, { context: '$text', priority: 'highest' } ); - this._isNextGravityRestorationSkipped = false; - // The automatic gravity restoration logic. this.listenTo( modelSelection, 'change:range', ( evt, data ) => { // Skipping the automatic restoration is needed if the selection should change diff --git a/packages/ckeditor5-typing/tests/twostepcaretmovement.js b/packages/ckeditor5-typing/tests/twostepcaretmovement.js index eccdec533e7..c870607505d 100644 --- a/packages/ckeditor5-typing/tests/twostepcaretmovement.js +++ b/packages/ckeditor5-typing/tests/twostepcaretmovement.js @@ -13,11 +13,14 @@ import TwoStepCaretMovement from '../src/twostepcaretmovement'; import Position from '@ckeditor/ckeditor5-engine/src/model/position'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard'; -import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; +import { getData as getModelData, setData as setModelData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import toArray from '@ckeditor/ckeditor5-utils/src/toarray'; import priorities from '@ckeditor/ckeditor5-utils/src/priorities'; -describe( 'TwoStepCaretMovement()', () => { +import Input from '../src/input'; +import Delete from '../src/delete'; + +describe( 'TwoStepCaretMovement', () => { let editor, model, emitter, selection, view, plugin; let preventDefaultSpy, evtStopSpy; @@ -26,7 +29,9 @@ describe( 'TwoStepCaretMovement()', () => { beforeEach( () => { emitter = Object.create( DomEmitterMixin ); - return VirtualTestEditor.create( { plugins: [ TwoStepCaretMovement ] } ).then( newEditor => { + return VirtualTestEditor.create( { + plugins: [ TwoStepCaretMovement, Input, Delete ] + } ).then( newEditor => { editor = newEditor; model = editor.model; selection = model.document.selection; @@ -802,6 +807,444 @@ describe( 'TwoStepCaretMovement()', () => { } ); } ); + // https://github.com/ckeditor/ckeditor5/issues/1016 + describe( 'mouse click a the edge of tow-step node', () => { + it( 'should insert content after the two-step node', () => { + setModelData( model, '<$text a="1">foo[]' ); + + editor.editing.view.document.fire( 'mousedown' ); + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( '<$text a="1">foo[]' ); + + model.change( writer => { + model.insertContent( writer.createText( 'bar', selection.getAttributes() ), selection.getFirstPosition() ); + } ); + + expect( getModelData( model ) ).to.equal( '<$text a="1">foobar[]' ); + } ); + + it( 'should insert content after the two-step node (with following text)', () => { + setModelData( model, '<$text a="1">foo[]<$text b="2">123' ); + + editor.editing.view.document.fire( 'mousedown' ); + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( '<$text a="1">foo<$text b="2">[]123' ); + + model.change( writer => { + model.insertContent( writer.createText( 'bar', selection.getAttributes() ), selection.getFirstPosition() ); + } ); + + expect( getModelData( model ) ).to.equal( '<$text a="1">foo<$text b="2">bar[]123' ); + } ); + + it( 'should insert content before the two-step node', () => { + setModelData( model, '<$text a="1">[]foo' ); + + editor.editing.view.document.fire( 'mousedown' ); + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( '[]<$text a="1">foo' ); + + model.change( writer => { + model.insertContent( writer.createText( 'bar', selection.getAttributes() ), selection.getFirstPosition() ); + } ); + + expect( getModelData( model ) ).to.equal( 'bar[]<$text a="1">foo' ); + } ); + + it( 'should insert content before the two-step node (with preceding text)', () => { + setModelData( model, '<$text b="2">123<$text a="1">[]foo' ); + + editor.editing.view.document.fire( 'mousedown' ); + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( '<$text b="2">123[]<$text a="1">foo' ); + + model.change( writer => { + model.insertContent( writer.createText( 'bar', selection.getAttributes() ), selection.getFirstPosition() ); + } ); + + expect( getModelData( model ) ).to.equal( '<$text b="2">123bar[]<$text a="1">foo' ); + } ); + + it( 'should insert content to the two-step node if clicked inside it', () => { + setModelData( model, '<$text a="1">f[]oo' ); + + editor.editing.view.document.fire( 'mousedown' ); + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( '<$text a="1">f[]oo' ); + + model.change( writer => { + model.insertContent( writer.createText( 'bar', selection.getAttributes() ), selection.getFirstPosition() ); + } ); + + expect( getModelData( model ) ).to.equal( '<$text a="1">fbar[]oo' ); + } ); + + it( 'should insert content between two two-step nodes (selection at the end of the first node)', () => { + setModelData( model, '<$text a="1">foo[]<$text a="2">bar' ); + + editor.editing.view.document.fire( 'mousedown' ); + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( + '<$text a="1">foo[]<$text a="2">bar' + ); + + model.change( writer => { + model.insertContent( writer.createText( '123', selection.getAttributes() ), selection.getFirstPosition() ); + } ); + + expect( getModelData( model ) ).to.equal( + '<$text a="1">foo123[]<$text a="2">bar' + ); + } ); + + it( 'should insert content between two two-step nodes (selection at the beginning of the second node)', () => { + setModelData( model, '<$text a="1">foo<$text a="2">[]bar' ); + + editor.editing.view.document.fire( 'mousedown' ); + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( + '<$text a="1">foo[]<$text a="2">bar' + ); + + model.change( writer => { + model.insertContent( writer.createText( '123', selection.getAttributes() ), selection.getFirstPosition() ); + } ); + + expect( getModelData( model ) ).to.equal( + '<$text a="1">foo123[]<$text a="2">bar' + ); + } ); + + it( 'should do nothing if the text was not clicked', () => { + setModelData( model, '<$text a="1">foo[]' ); + + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( '<$text a="1">foo[]' ); + } ); + + it( 'should do nothing if the selection is not collapsed after the click', () => { + setModelData( model, '[<$text a="1">foo]' ); + + editor.editing.view.document.fire( 'mousedown' ); + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( '[<$text a="1">foo]' ); + } ); + + it( 'should do nothing if the text is not a two-step node', () => { + setModelData( model, '<$text bold="true">foo[]' ); + + editor.editing.view.document.fire( 'mousedown' ); + editor.editing.view.document.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( getModelData( model ) ).to.equal( '<$text bold="true">foo[]' ); + } ); + } ); + + // https://github.com/ckeditor/ckeditor5/issues/6053 + describe( 'selection attribute management on paste', () => { + beforeEach( () => { + model.schema.extend( '$text', { allowAttributes: 'bold' } ); + } ); + + it( 'should remove two-step attributes when pasting two-step content', () => { + setModelData( model, 'foo[]' ); + + model.change( writer => { + model.insertContent( writer.createText( 'INSERTED', { a: 'abc' } ) ); + } ); + + expect( getModelData( model ) ).to.equal( 'foo<$text a="abc">INSERTED[]' ); + + expect( [ ...model.document.selection.getAttributeKeys() ] ).to.be.empty; + } ); + + it( 'should not remove two-step attributes when pasting a non-two-step content', () => { + setModelData( model, '<$text a="abc">foo[]' ); + + model.change( writer => { + model.insertContent( writer.createText( 'INSERTED', { bold: 'true' } ) ); + } ); + + expect( getModelData( model ) ).to.equal( + '' + + '<$text a="abc">foo' + + '<$text bold="true">INSERTED[]' + + '' + ); + + expect( model.document.selection ).to.have.attribute( 'bold' ); + } ); + + it( 'should not remove two-step attributes when pasting in the middle of a two-step with the same value', () => { + setModelData( model, '<$text a="abc">fo[]o' ); + + model.change( writer => { + model.insertContent( writer.createText( 'INSERTED', { a: 'abc' } ) ); + } ); + + expect( getModelData( model ) ).to.equal( '<$text a="abc">foINSERTED[]o' ); + expect( model.document.selection ).to.have.attribute( 'a' ); + } ); + + it( 'should not remove two-step attributes from the selection when pasting before a two-step with overridden gravity', () => { + setModelData( model, 'foo[]<$text a="abc">bar' ); + + view.document.fire( 'keydown', { + keyCode: keyCodes.arrowright, + preventDefault: () => {}, + domTarget: document.body + } ); + + expect( model.document.selection ).to.have.property( 'isGravityOverridden', true ); + + model.change( writer => { + model.insertContent( writer.createText( 'INSERTED', { bold: true } ) ); + } ); + + expect( getModelData( model ) ).to.equal( + '' + + 'foo' + + '<$text bold="true">INSERTED[]' + + '<$text a="abc">bar' + + '' + ); + + expect( model.document.selection ).to.have.property( 'isGravityOverridden', true ); + expect( [ ...model.document.selection.getAttributeKeys() ] ).to.be.empty; + } ); + + it( 'should remove two-step attributes when pasting a two-step into another two-step (different value)', () => { + setModelData( model, '<$text a="abc">f[]oo' ); + + model.change( writer => { + model.insertContent( writer.createText( 'INSERTED', { a: 'def' } ) ); + } ); + + expect( getModelData( model ) ).to.equal( + '' + + '<$text a="abc">f' + + '<$text a="def">INSERTED[]' + + '<$text a="abc">oo' + + '' + ); + + expect( [ ...model.document.selection.getAttributeKeys() ] ).to.be.empty; + } ); + + it( 'should not remove two-step attributes when pasting before another two-step (different value)', () => { + setModelData( model, '[]<$text a="abc">foo' ); + + expect( model.document.selection ).to.have.property( 'isGravityOverridden', false ); + + model.change( writer => { + model.insertContent( writer.createText( 'INSERTED', { a: 'def' } ) ); + } ); + + expect( getModelData( model ) ).to.equal( + '' + + '<$text a="def">INSERTED[]' + + '<$text a="abc">foo' + + '' + ); + + expect( [ ...model.document.selection.getAttributeKeys() ] ).to.be.empty; + } ); + } ); + + // https://github.com/ckeditor/ckeditor5/issues/7521 + describe( 'removing a character before the link element', () => { + beforeEach( () => { + sinon.stub( editor.editing.view, 'scrollToTheSelection' ); + } ); + + it( 'should not preserve the two-step attribute when deleting content after the two-step content', () => { + setModelData( model, 'Foo <$text a="url">Bar []' ); + + expect( model.document.selection.hasAttribute( 'a' ), 'initial state' ).to.equal( false ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( model.document.selection.hasAttribute( 'a' ), 'removing space after the link' ).to.equal( false ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( model.document.selection.hasAttribute( 'a' ), 'removing a character in the link' ).to.equal( false ); + expect( getModelData( model ) ).to.equal( 'Foo <$text a="url">Ba[]' ); + } ); + + it( 'should not preserve the two-step attribute when deleting content at the beginning of the two-step content', () => { + setModelData( model, '<$text a="url">B[]ar' ); + + expect( model.document.selection.hasAttribute( 'a' ), 'initial state' ).to.equal( true ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( model.document.selection.hasAttribute( 'a' ), 'removing first character' ).to.equal( false ); + + expect( getModelData( model ) ).to.equal( '[]<$text a="url">ar' ); + } ); + + it( 'should not preserve the two-step attribute when deleting content before another two-step content', () => { + setModelData( model, '<$text a="1">Bar<$text a="2">B[]ar' ); + + expect( model.document.selection.hasAttribute( 'a' ), 'initial state' ).to.equal( true ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( model.document.selection.hasAttribute( 'a' ), 'removing first character' ).to.equal( false ); + + expect( getModelData( model ) ).to.equal( '<$text a="1">Bar[]<$text a="2">ar' ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( model.document.selection.hasAttribute( 'a' ) ).to.equal( false ); + + expect( getModelData( model ) ).to.equal( '<$text a="1">Ba[]<$text a="2">ar' ); + } ); + + it( 'should preserve the two-step attribute when deleting content and the selection is at the end of the two-step content', () => { + setModelData( model, 'Foo <$text a="url">Bar []' ); + + expect( model.document.selection.hasAttribute( 'a' ), 'initial state' ).to.equal( true ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( model.document.selection.hasAttribute( 'a' ), 'removing space after the link' ).to.equal( true ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( model.document.selection.hasAttribute( 'a' ), 'removing a character in the link' ).to.equal( true ); + expect( getModelData( model ) ).to.equal( 'Foo <$text a="url">Ba[]' ); + } ); + + it( 'should preserve the two-step attribute when deleting content while the selection is inside the two-step content', () => { + setModelData( model, 'Foo <$text a="url">A long URLLs[] description' ); + + expect( model.document.selection.hasAttribute( 'a' ), 'initial state' ).to.equal( true ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( model.document.selection.hasAttribute( 'a' ), 'removing space after the link' ).to.equal( true ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( model.document.selection.hasAttribute( 'a' ), 'removing a character in the link' ).to.equal( true ); + expect( getModelData( model ) ).to.equal( 'Foo <$text a="url">A long URL[] description' ); + } ); + + it( 'should do nothing if there is no two-step attribute', () => { + model.schema.extend( '$text', { allowAttributes: 'bold' } ); + setModelData( model, 'Foo <$text bold="true">Bolded. []Bar' ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'backward', + selectionToRemove: view.document.selection + } ) ); + + expect( getModelData( model ) ).to.equal( 'Foo <$text bold="true">Bolded[]Bar' ); + } ); + + it( 'should preserve the two-step attribute when deleting content using "Delete" key', () => { + setModelData( model, 'Foo <$text a="url">Bar[ ]' ); + + expect( model.document.selection.hasAttribute( 'a' ), 'initial state' ).to.equal( false ); + + view.document.fire( 'delete', new DomEventData( view.document, { + preventDefault: () => {} + }, { + direction: 'forward', + selectionToRemove: view.document.selection + } ) ); + + expect( getModelData( model ) ).to.equal( 'Foo <$text a="url">Bar[]' ); + + expect( model.document.selection.hasAttribute( 'a' ), 'removing space after the link' ).to.equal( true ); + } ); + } ); + it( 'should listen with the higher priority than widget type around', () => { const highestPlusPrioritySpy = sinon.spy().named( 'highestPrioritySpy' ); const highestPrioritySpy = sinon.spy().named( 'highestPrioritySpy' ); From 0612a69a4a5197e7a4432c66e19683d3185601ba Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 20 Nov 2023 18:20:51 +0100 Subject: [PATCH 09/10] Added tests. --- .../tests/inserttextcommand.js | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/packages/ckeditor5-typing/tests/inserttextcommand.js b/packages/ckeditor5-typing/tests/inserttextcommand.js index 4d1d68af72e..8e8a3fbaddf 100644 --- a/packages/ckeditor5-typing/tests/inserttextcommand.js +++ b/packages/ckeditor5-typing/tests/inserttextcommand.js @@ -31,6 +31,7 @@ describe( 'InsertTextCommand', () => { model.schema.register( 'paragraph', { inheritAllFrom: '$block' } ); model.schema.register( 'heading1', { inheritAllFrom: '$block' } ); + model.schema.extend( '$text', { allowAttributes: 'bold' } ); } ); } ); @@ -429,6 +430,62 @@ describe( 'InsertTextCommand', () => { return editor.model.change( writer => writer.batch ); } } ); + + describe( 'applies document selection attributes', () => { + it( 'insert using the DocumentSelection as insertText target', () => { + setData( model, 'foo[]' ); + + model.change( writer => { + writer.setSelectionAttribute( 'bold', true ); + } ); + + expect( getData( model ) ).to.equal( 'foo<$text bold="true">[]' ); + + editor.execute( 'insertText', { + text: 'bar' + } ); + + expect( getData( model ) ).to.equal( 'foo<$text bold="true">bar[]' ); + } ); + + it( 'insert using a static selection as insertText target', () => { + setData( model, 'foo[]' ); + + model.change( writer => { + writer.setSelectionAttribute( 'bold', true ); + } ); + + expect( getData( model ) ).to.equal( 'foo<$text bold="true">[]' ); + + editor.execute( 'insertText', { + text: 'bar', + selection: model.createSelection( doc.getRoot().getChild( 0 ), 0 ) + } ); + + expect( getData( model ) ).to.equal( '<$text bold="true">bar[]foo' ); + } ); + + it( 'replace using the DocumentSelection as insertText target', () => { + setData( model, 'foo<$text bold="true">[bar]' ); + + editor.execute( 'insertText', { + text: 'abc' + } ); + + expect( getData( model ) ).to.equal( 'foo<$text bold="true">abc[]' ); + } ); + + it( 'replace using a static selection as insertText target', () => { + setData( model, 'foo<$text bold="true">[bar]' ); + + editor.execute( 'insertText', { + text: 'abc', + selection: model.createSelection( doc.selection ) + } ); + + expect( getData( model ) ).to.equal( 'foo<$text bold="true">abc[]' ); + } ); + } ); } ); describe( 'destroy()', () => { From 2e858edb694e32287f10e56a77e3441888750130 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 20 Nov 2023 18:29:35 +0100 Subject: [PATCH 10/10] Updated comments. --- packages/ckeditor5-link/src/linkediting.ts | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/ckeditor5-link/src/linkediting.ts b/packages/ckeditor5-link/src/linkediting.ts index 1584fe3ccb3..edd701598fd 100644 --- a/packages/ckeditor5-link/src/linkediting.ts +++ b/packages/ckeditor5-link/src/linkediting.ts @@ -136,8 +136,8 @@ export default class LinkEditing extends Plugin { // Handle link following by CTRL+click or ALT+ENTER this._enableLinkOpen(); - // TODO update comment - Change the attributes of the selection in certain situations after the link was inserted into the document. - this._enableSelectionDecoratorAttributesFixer(); + // Clears the DocumentSelection decorator attributes if the selection is no longer in a link (for example while using 2-SCM). + this._enableSelectionAttributesFixer(); // Handle adding default protocol to pasted links. this._enableClipboardIntegration(); @@ -298,15 +298,11 @@ export default class LinkEditing extends Plugin { } /** - * Starts listening to {@link module:engine/model/model~Model#event:insertContent} and corrects the model - * selection attributes if the selection is at the end of a link after inserting the content. + * Watches the DocumentSelection attribute changes and removes link decorator attributes when the linkHref attribute is removed. * - * The purpose of this action is to improve the overall UX because the user is no longer "trapped" by the - * `linkHref` attribute of the selection and they can type a "clean" (`linkHref`–less) text right away. - * - * TODO update description + * This is to ensure that there is no left-over link decorator attributes on the document selection that is no longer in a link. */ - private _enableSelectionDecoratorAttributesFixer(): void { + private _enableSelectionAttributesFixer(): void { const editor = this.editor; const model = editor.model; const selection = model.document.selection;