diff --git a/.changelog/20260306121909_ck_19853_softBreakAndSelection.md b/.changelog/20260306121909_ck_19853_softBreakAndSelection.md new file mode 100644 index 00000000000..ec35f8a390c --- /dev/null +++ b/.changelog/20260306121909_ck_19853_softBreakAndSelection.md @@ -0,0 +1,12 @@ +--- +type: Fix +scope: + - ckeditor5-engine + - ckeditor5-enter +closes: + - 19853 +--- + +Improved document selection attribute inheritance around `` so returning the caret after a soft break restores expected formatting. + +When selection attributes are recalculated across ``, only attributes marked with `copyOnEnter` are inherited. Other inline non-object elements still act as hard boundaries. diff --git a/.changelog/20260407170702_ck_19853_softBreakAndSelection.md b/.changelog/20260407170702_ck_19853_softBreakAndSelection.md new file mode 100644 index 00000000000..513663c8e0c --- /dev/null +++ b/.changelog/20260407170702_ck_19853_softBreakAndSelection.md @@ -0,0 +1,10 @@ +--- +type: Fix +scope: + - ckeditor5-engine + - ckeditor5-enter +closes: + - https://github.com/ckeditor/ckeditor5/issues/1068 +--- + +Inline formatting attributes (such as bold or link) are no longer split by a soft break (`
`). The `
` element now inherits applicable text attributes so that attribute elements in the view can wrap around it without being broken into separate segments. diff --git a/packages/ckeditor5-engine/src/model/documentselection.ts b/packages/ckeditor5-engine/src/model/documentselection.ts index 876f3434cb7..d7d77999fb7 100644 --- a/packages/ckeditor5-engine/src/model/documentselection.ts +++ b/packages/ckeditor5-engine/src/model/documentselection.ts @@ -15,7 +15,6 @@ import { type ModelSelectionChangeRangeEvent } from './selection.js'; import { ModelText } from './text.js'; -import { ModelTextProxy } from './textproxy.js'; import type { ModelDocumentChangeEvent, ModelDocument } from './document.js'; import type { Model, ModelApplyOperationEvent } from './model.js'; @@ -23,6 +22,7 @@ import type { Marker, MarkerCollectionUpdateEvent } from './markercollection.js' import { type Batch } from './batch.js'; import { type ModelElement } from './element.js'; import { type ModelItem } from './item.js'; +import { type ModelNode } from './node.js'; import type { ModelPosition, ModelPositionOffset } from './position.js'; import { type ModelRange } from './range.js'; import { type ModelSchema } from './schema.js'; @@ -1157,7 +1157,7 @@ class LiveSelection extends ModelSelection { return null; } - let attrs = null; + let attrs: Iterable<[string, unknown]> | null = null; if ( !this.isCollapsed ) { // 1. If selection is a range... @@ -1197,22 +1197,12 @@ class LiveSelection extends ModelSelection { // 4. If not, try to find the first character on the left, that is in the same node. // When gravity is overridden then don't take node before into consideration. if ( !this.isGravityOverridden && !attrs ) { - let node = nodeBefore; - - while ( node && !attrs ) { - node = node.previousSibling; - attrs = getTextAttributes( node, schema ); - } + attrs = getTextAttributes( nodeBefore, schema, 'backward' ); } // 5. If not found, try to find the first character on the right, that is in the same node. if ( !attrs ) { - let node = nodeAfter; - - while ( node && !attrs ) { - node = node.nextSibling; - attrs = getTextAttributes( node, schema ); - } + attrs = getTextAttributes( nodeAfter, schema, 'forward' ); } // 6. If not found, selection should retrieve attributes from parent. @@ -1244,40 +1234,70 @@ class LiveSelection extends ModelSelection { /** * Helper function for {@link module:engine/model/liveselection~LiveSelection#_updateAttributes}. * - * It checks if the passed model item is a text node (or text proxy) and, if so, returns it's attributes. - * If not, it checks if item is an inline object and does the same. Otherwise it returns `null`. + * It checks if the passed model node is a text node and, if so, returns its attributes. + * If not, it checks if item is an inline element and does the same. Otherwise, it returns `null`. */ -function getTextAttributes( node: ModelItem | null, schema: ModelSchema ): Iterable<[string, unknown]> | null { - if ( !node ) { +function getTextAttributes( + startNode: ModelNode | null, + schema: ModelSchema, + scan: 'self' | 'backward' | 'forward' = 'self' +): Iterable<[string, unknown]> | null { + if ( !startNode ) { return null; } - if ( node instanceof ModelTextProxy || node instanceof ModelText ) { - return node.getAttributes(); - } + for ( const node of siblingNodes( startNode, scan ) ) { + if ( !node ) { + return null; + } - if ( !schema.isInline( node ) ) { - return null; + if ( node instanceof ModelText ) { + return node.getAttributes(); + } + + if ( !schema.isInline( node ) ) { + continue; + } + + const isObject = schema.isObject( node ); + const attributes: Array<[string, unknown]> = []; + + // Collect all attributes that can be applied to the text node. + for ( const [ key, value ] of node.getAttributes() ) { + if ( + schema.checkAttribute( '$text', key ) && + ( !isObject || schema.getAttributeProperties( key ).copyFromObject !== false ) + ) { + attributes.push( [ key, value ] ); + } + } + + return attributes; } - // Stop on inline elements (such as ``) that are not objects (such as `` or ``). - if ( !schema.isObject( node ) ) { - return []; + return null; +} + +/** + * Returns sibling nodes from the given start node. + */ +function* siblingNodes( startNode: ModelNode, scan: 'self' | 'backward' | 'forward' ) { + if ( scan == 'self' ) { + yield startNode; } + else { + let node: ModelNode | null = startNode; - const attributes: Array<[string, unknown]> = []; + while ( node ) { + node = scan == 'backward' ? + node.previousSibling : + node.nextSibling; - // Collect all attributes that can be applied to the text node. - for ( const [ key, value ] of node.getAttributes() ) { - if ( - schema.checkAttribute( '$text', key ) && - schema.getAttributeProperties( key ).copyFromObject !== false - ) { - attributes.push( [ key, value ] ); + yield node; } } - return attributes; + return null; } /** diff --git a/packages/ckeditor5-engine/src/view/containerelement.ts b/packages/ckeditor5-engine/src/view/containerelement.ts index ff9120a307e..6a3fee724cd 100644 --- a/packages/ckeditor5-engine/src/view/containerelement.ts +++ b/packages/ckeditor5-engine/src/view/containerelement.ts @@ -92,13 +92,22 @@ ViewContainerElement.prototype.is = function( this: ViewContainerElement, type: */ export function getViewFillerOffset( this: ViewContainerElement ): number | null { const children = [ ...this.getChildren() ]; - const lastChild = children[ this.childCount - 1 ]; + let lastChild: ViewNode | undefined = children[ this.childCount - 1 ]; // Block filler is required after a `
` if it's the last element in its container. See #1422. if ( lastChild && lastChild.is( 'element', 'br' ) ) { return this.childCount; } + // Check if there is a `
` inside an attribute element at the end of container. + while ( lastChild && lastChild.is( 'attributeElement' ) ) { + lastChild = lastChild.getChild( lastChild.childCount - 1 ); + + if ( lastChild && lastChild.is( 'element', 'br' ) ) { + return this.childCount; + } + } + for ( const child of children ) { // If there's any non-UI element – don't render the bogus. if ( !child.is( 'uiElement' ) ) { diff --git a/packages/ckeditor5-engine/tests/model/documentselection.js b/packages/ckeditor5-engine/tests/model/documentselection.js index 48463eb45eb..6c6563c9925 100644 --- a/packages/ckeditor5-engine/tests/model/documentselection.js +++ b/packages/ckeditor5-engine/tests/model/documentselection.js @@ -1122,6 +1122,30 @@ describe( 'DocumentSelection', () => { new ModelElement( 'p', { p: true } ), new ModelText( 'e', { e: true } ) ] ); + + // Just to make sure how model looks. + expect( _getModelData( model, { withoutSelection: true } ) ).to.equal( + '

' + + + '<$text a="true">a' + + + '

' + + + '<$text b="true">b' + + '<$text c="true">c' + + + '

' + + '<$text d="true">d' + + '

' + + + '

' + + + '<$text e="true">e' + + + // Left-overs from parent describe. + '

foobar

' + + '

' + ); } ); it( 'if selection is a range, should find first character in it and copy it\'s attributes', () => { @@ -1407,10 +1431,11 @@ describe( 'DocumentSelection', () => { } ); // #7459 - describe( 'ignores inline elements while reading surrounding attributes', () => { + describe( 'softBreak integration', () => { beforeEach( () => { model.schema.register( 'softBreak', { allowWhere: '$text', + allowAttributesOf: '$text', isInline: true } ); } ); @@ -1430,6 +1455,22 @@ describe( 'DocumentSelection', () => { selection._restoreGravity( overrideGravityUid ); } ); + + it( 'should inherit attributes from an inline element', () => { + _setModelData( model, '

<$text bold="true">Foo Bar.[]

' ); + + expect( selection.hasAttribute( 'bold' ) ).to.equal( true ); + } ); + + it( 'should inherit attributes from an inline element (override gravity)', () => { + _setModelData( model, '

[]<$text bold="true">Foo Bar.

' ); + + const overrideGravityUid = selection._overrideGravity(); + + expect( selection.hasAttribute( 'bold' ) ).to.equal( true ); + + selection._restoreGravity( overrideGravityUid ); + } ); } ); // #14106 diff --git a/packages/ckeditor5-engine/tests/view/containerelement.js b/packages/ckeditor5-engine/tests/view/containerelement.js index ab7fe1d9f2c..83df2595ab7 100644 --- a/packages/ckeditor5-engine/tests/view/containerelement.js +++ b/packages/ckeditor5-engine/tests/view/containerelement.js @@ -79,36 +79,87 @@ describe( 'ContainerElement', () => { // Block filler is required after the `
` element if the element is the last child in the container. See #1422. describe( 'for
elements in container', () => { - it( 'returns null because container does not need the block filler', () => { + it( 'should return null for a container with text content only', () => { expect( _parseView( 'Foo.' ).getFillerOffset() ).to.equals( null ); } ); - it( 'returns offset of the last child which is the
element (1)', () => { - expect( _parseView( '' ).getFillerOffset() ).to.equals( 1 ); + it( 'should return offset after a
element that is the only child', () => { + expect( _parseView( '' ).getFillerOffset() ).to.equals( 1 ); } ); - it( 'returns offset of the last child which is the
element (2)', () => { - expect( _parseView( 'Foo.' ).getFillerOffset() ).to.equals( 2 ); + it( 'should return offset after a
element that follows text', () => { + expect( _parseView( 'Foo.' ).getFillerOffset() ).to.equals( 2 ); } ); - it( 'always returns the last
element in the container', () => { - expect( _parseView( 'Foo.' ).getFillerOffset() ) + it( 'should return offset after the last
element when there are consecutive
elements', () => { + expect( _parseView( 'Foo.' ).getFillerOffset() ) .to.equals( 3 ); } ); - it( 'works fine with non-empty container with multi
elements', () => { - expect( _parseView( 'Foo.Bar.' ).getFillerOffset() ) - .to.equals( 4 ); + it( 'should return offset after the last
element when text nodes are between
elements', () => { + expect( _parseView( 'Foo.Bar.' ).getFillerOffset() ).to.equals( 4 ); } ); - it( 'ignores the ui elements', () => { - expect( _parseView( '' ).getFillerOffset() ) - .to.equals( 2 ); + it( 'should return offset after a
element ignoring preceding ui elements', () => { + expect( _parseView( '' ).getFillerOffset() ).to.equals( 2 ); } ); - it( 'empty element must be the
element', () => { - expect( _parseView( 'Foo' ).getFillerOffset() ) - .to.equals( null ); + it( 'should return null when the last empty element is not a
element', () => { + expect( _parseView( 'Foo' ).getFillerOffset() ).to.equals( null ); + } ); + } ); + + // Block filler is required after the `
` element if the element is the last child in the container + // even when nested in an attribute element. + describe( 'for
elements in container inside an attribute element', () => { + it( 'should return null when the attribute element contains text only', () => { + expect( + _parseView( 'Foo.' ).getFillerOffset() + ).to.equals( null ); + } ); + + it( 'should return offset after the attribute element that contains only a
element', () => { + expect( + _parseView( '' ).getFillerOffset() + ).to.equals( 1 ); + } ); + + it( 'should return offset after the attribute element that ends with a
element', () => { + expect( + _parseView( 'Foo.' ).getFillerOffset() + ).to.equals( 1 ); + } ); + + it( 'should return offset after the attribute element with deeply nested
element', () => { + expect( + _parseView( + 'Foo.' + ).getFillerOffset() + ).to.equals( 1 ); + } ); + + it( 'should return offset after the attribute element that ends with consecutive
elements', () => { + expect( + _parseView( 'Foo.' ).getFillerOffset() + ).to.equals( 1 ); + } ); + + it( 'should return offset after the attribute element with text nodes between
elements', () => { + expect( + _parseView( 'Foo.Bar.' ).getFillerOffset() + ).to.equals( 1 ); + } ); + + it( 'should return offset after the attribute element with a
element ignoring ui elements', () => { + expect( + _parseView( '' ).getFillerOffset() + ).to.equals( 1 ); + } ); + + it( 'should return null when the last empty element inside the attribute element is not a
element', () => { + expect( + _parseView( 'Foo' ).getFillerOffset() + ).to.equals( null ); } ); } ); } ); diff --git a/packages/ckeditor5-enter/src/shiftenter.ts b/packages/ckeditor5-enter/src/shiftenter.ts index 3fe889595fa..5f9e4d0aef0 100644 --- a/packages/ckeditor5-enter/src/shiftenter.ts +++ b/packages/ckeditor5-enter/src/shiftenter.ts @@ -11,6 +11,12 @@ import { ShiftEnterCommand } from './shiftentercommand.js'; import { EnterObserver, type ViewDocumentEnterEvent } from './enterobserver.js'; import { Plugin } from '@ckeditor/ckeditor5-core'; +import type { + ModelElement, + ModelNode, + ModelWriter +} from '@ckeditor/ckeditor5-engine'; + /** * This plugin handles the Shift+Enter keystroke (soft line break) in the editor. * @@ -44,6 +50,7 @@ export class ShiftEnter extends Plugin { // Configure the schema. schema.register( 'softBreak', { allowWhere: '$text', + allowAttributesOf: '$text', isInline: true } ); @@ -63,6 +70,7 @@ export class ShiftEnter extends Plugin { view.addObserver( EnterObserver ); editor.commands.add( 'shiftEnter', new ShiftEnterCommand( editor ) ); + editor.model.document.registerPostFixer( writer => removeStaleSoftBreakAttributes( writer ) ); this.listenTo( viewDocument, 'enter', ( evt, data ) => { // When not in composition, we handle the action, so prevent the default one. @@ -91,3 +99,59 @@ export class ShiftEnter extends Plugin { } ); } } + +/** + * The post-fixer that removes attributes from stale soft-break elements. + */ +function removeStaleSoftBreakAttributes( writer: ModelWriter ): boolean { + const parentsToCheck = new Set(); + + for ( const change of writer.model.document.differ.getChanges() ) { + if ( change.type == 'insert' || change.type == 'remove' ) { + if ( change.position.parent.is( 'element' ) ) { + parentsToCheck.add( change.position.parent ); + } + } else if ( change.type == 'attribute' ) { + if ( change.range.start.parent.is( 'element' ) ) { + parentsToCheck.add( change.range.start.parent ); + } + } + } + + let wasChanged = false; + + for ( const parent of parentsToCheck ) { + // Just a sibling nodes check. We do not need to check attributes of nested elements. + for ( const child of parent.getChildren() ) { + // Find a softBreak element. + if ( !child.is( 'element', 'softBreak' ) ) { + continue; + } + + const nextSibling = child.nextSibling; + + // Do not remove attributes when softBreak is the last element or there is some element after it. + if ( !nextSibling || nextSibling.is( 'element' ) ) { + continue; + } + + // Remove the attribute if the next sibling does not have the same attribute. + const attributesToRemove = Array.from( child.getAttributes() ) + .filter( ( [ key, value ] ) => !hasSameAttribute( nextSibling, key, value ) ); + + for ( const [ key ] of attributesToRemove ) { + writer.removeAttribute( key, child ); + wasChanged = true; + } + } + } + + return wasChanged; +} + +/** + * Returns `true` if the given node has the same attribute as the one provided in the parameters. + */ +function hasSameAttribute( node: ModelNode | null, key: string, value: unknown ): boolean { + return node?.getAttribute( key ) === value; +} diff --git a/packages/ckeditor5-enter/src/shiftentercommand.ts b/packages/ckeditor5-enter/src/shiftentercommand.ts index ad2d00e06e1..375f361b005 100644 --- a/packages/ckeditor5-enter/src/shiftentercommand.ts +++ b/packages/ckeditor5-enter/src/shiftentercommand.ts @@ -98,12 +98,7 @@ function softBreakAction( model: Model, writer: ModelWriter, selection: ModelDoc const isContainedWithinOneElement = ( startElement == endElement ); if ( isSelectionEmpty ) { - const attributesToCopy = getCopyOnEnterAttributes( model.schema, selection.getAttributes() ); - - insertBreak( model, writer, range.end ); - - writer.removeSelectionAttribute( selection.getAttributeKeys() ); - writer.setSelectionAttribute( attributesToCopy ); + insertBreak( model, writer, range.end, selection.getAttributes() ); } else { const leaveUnmerged = !( range.start.isAtStart && range.end.isAtEnd ); @@ -113,7 +108,7 @@ function softBreakAction( model: Model, writer: ModelWriter, selection: ModelDoc // // x[xx]x -> x^x -> x
^x if ( isContainedWithinOneElement ) { - insertBreak( model, writer, selection.focus! ); + insertBreak( model, writer, selection.focus!, selection.getAttributes() ); } // Selection over multiple elements. // @@ -134,8 +129,17 @@ function softBreakAction( model: Model, writer: ModelWriter, selection: ModelDoc } } -function insertBreak( model: Model, writer: ModelWriter, position: ModelPosition ): void { - const breakLineElement = writer.createElement( 'softBreak' ); +/** + * Inserts a softBreak with applicable attributes. + */ +function insertBreak( + model: Model, + writer: ModelWriter, + position: ModelPosition, + selectionAttributes: IterableIterator<[ string, unknown ]> +): void { + const attributes = Array.from( getCopyOnEnterAttributes( model.schema, selectionAttributes ) ); + const breakLineElement = writer.createElement( 'softBreak', attributes ); model.insertContent( breakLineElement, position ); writer.setSelection( breakLineElement, 'after' ); diff --git a/packages/ckeditor5-enter/tests/shiftenter-integration.js b/packages/ckeditor5-enter/tests/shiftenter-integration.js index 633beee2d04..ddb95d655d8 100644 --- a/packages/ckeditor5-enter/tests/shiftenter-integration.js +++ b/packages/ckeditor5-enter/tests/shiftenter-integration.js @@ -59,11 +59,11 @@ describe( 'ShiftEnter integration', () => { ); } ); - it( 'should not inherit text attributes before the "softBreak" element', () => { + it( 'should inherit only copyOnEnter text attributes before the "softBreak" element', () => { _setModelData( model, '' + '<$text linkHref="foo" bold="true">Bolded link' + - '' + + '' + 'F[]' + '' ); @@ -73,6 +73,6 @@ describe( 'ShiftEnter integration', () => { const selection = model.document.selection; expect( selection.hasAttribute( 'linkHref' ) ).to.equal( false ); - expect( selection.hasAttribute( 'bold' ) ).to.equal( false ); + expect( selection.hasAttribute( 'bold' ) ).to.equal( true ); } ); } ); diff --git a/packages/ckeditor5-enter/tests/shiftentercommand.js b/packages/ckeditor5-enter/tests/shiftentercommand.js index 3d294f32634..0dc60ab8f47 100644 --- a/packages/ckeditor5-enter/tests/shiftentercommand.js +++ b/packages/ckeditor5-enter/tests/shiftentercommand.js @@ -110,7 +110,7 @@ describe( 'ShiftEnterCommand', () => { test( 'allowed attributes are copied', '

<$text foo="true">test[]

', - '

<$text foo="true">test<$text foo="true">[]

' + '

<$text foo="true">test<$text foo="true">[]

' ); test( @@ -122,8 +122,63 @@ describe( 'ShiftEnterCommand', () => { test( 'only allowed attributes are copied from mix set', '

<$text bar="true" foo="true">test[]

', - '

<$text bar="true" foo="true">test<$text foo="true">[]

' + '

<$text bar="true" foo="true">test<$text foo="true">[]

' ); + + it( 'removes stale attributes from the last softBreak when the following text no longer has them', () => { + _setModelData( + model, + '

<$text foo="true">foo<$text foo="true">bar[]

' + ); + + const paragraph = doc.getRoot().getChild( 0 ); + + model.change( writer => { + writer.removeAttribute( 'foo', paragraph.getChild( 0 ) ); + writer.removeAttribute( 'foo', paragraph.getChild( 2 ) ); + } ); + + expect( _getModelData( model ) ).to.equal( '

foobar[]

' ); + } ); + + it( 'keeps attributes on a softBreak followed by another softBreak', () => { + _setModelData( + model, + '

<$text foo="true">foo' + + '' + + '<$text foo="true">bar[]

' + ); + + const paragraph = doc.getRoot().getChild( 0 ); + + model.change( writer => { + writer.removeAttribute( 'foo', paragraph.getChild( 0 ) ); + writer.removeAttribute( 'foo', paragraph.getChild( 3 ) ); + } ); + + expect( _getModelData( model ) ).to.equal( + '

foobar[]

' + ); + } ); + + it( 'keeps softBreak attributes when there is no following text node', () => { + _setModelData( + model, + '

<$text foo="true">foo[]

bar

' + ); + + editor.execute( 'shiftEnter' ); + + const firstParagraph = doc.getRoot().getChild( 0 ); + + model.change( writer => { + writer.removeAttribute( 'foo', firstParagraph.getChild( 0 ) ); + } ); + + expect( _getModelData( model ) ).to.equal( + '

foo<$text foo="true">[]

bar

' + ); + } ); } ); } ); diff --git a/packages/ckeditor5-paste-from-office/tests/_data/link/two-line/model.word2016.html b/packages/ckeditor5-paste-from-office/tests/_data/link/two-line/model.word2016.html index f2555a6ff5e..c2fe3e622f8 100644 --- a/packages/ckeditor5-paste-from-office/tests/_data/link/two-line/model.word2016.html +++ b/packages/ckeditor5-paste-from-office/tests/_data/link/two-line/model.word2016.html @@ -1 +1 @@ -<$text linkHref="https://cksource.com/">Long link <$text linkHref="https://cksource.com/">WITH spaces +<$text linkHref="https://cksource.com/">Long link <$text linkHref="https://cksource.com/">WITH spaces