Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changelog/20260306121909_ck_19853_softBreakAndSelection.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
type: Fix
scope:
- ckeditor5-engine
closes:
- 19853
---

Improved document selection attribute inheritance around `<softBreak>` so returning the caret after a soft break restores expected formatting.

When selection attributes are recalculated across `<softBreak>`, only attributes marked with `copyOnEnter` are inherited. Other inline non-object elements still act as hard boundaries.
99 changes: 68 additions & 31 deletions packages/ckeditor5-engine/src/model/documentselection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,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';
Expand Down Expand Up @@ -1157,7 +1158,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...
Expand Down Expand Up @@ -1197,22 +1198,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, 'left' );
}

// 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, 'right' );
}

// 6. If not found, selection should retrieve attributes from parent.
Expand Down Expand Up @@ -1247,37 +1238,83 @@ class LiveSelection extends ModelSelection {
* 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`.
*/
function getTextAttributes( node: ModelItem | null, schema: ModelSchema ): Iterable<[string, unknown]> | null {
if ( !node ) {
function getTextAttributes(
startNode: ModelItem | null,
schema: ModelSchema,
scan: 'single' | 'left' | 'right' = 'single'
): Iterable<[string, unknown]> | null {
if ( !startNode ) {
return null;
}

if ( node instanceof ModelTextProxy || node instanceof ModelText ) {
return node.getAttributes();
// This case can happen only for non-collapsed selection.
// It is already handled in `_getSurroundingAttributes` as `if ( value.type == 'text' )`).
// However this condition was also in `getTextAttributes` before. It's kept as a paranoid check.
/* istanbul ignore if -- @preserve */
if ( startNode instanceof ModelTextProxy ) {
return startNode.getAttributes();
}

if ( !schema.isInline( node ) ) {
return null;
let crossedSoftBreak = false;

for ( let node: ModelNode | null = startNode; node; node = getNextNode( node ) ) {
if ( node.is( 'element', 'softBreak' ) ) {
crossedSoftBreak = true;
continue;
}
Comment on lines +1260 to +1264
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether the engine should be aware of the existence of the softBreak element. It's registered in an external plugin. Would a schema flag be better? What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed with @niegowski, and it might be good idea to store some info in schema / attribute properties. However, there might be another workaround related to storing attributes on soft-breaks itself. We need to discuss it a little bit more.


const attrs = getTextAttributesFromSingleNode( node );

if ( attrs ) {
return crossedSoftBreak ? collectCopyOnEnterAttributes( attrs ) : attrs;
}
}

// Stop on inline elements (such as `<softBreak>`) that are not objects (such as `<imageInline>` or `<mathml>`).
if ( !schema.isObject( node ) ) {
return [];
return null;

function getNextNode( node: ModelNode ) {
switch ( scan ) {
case 'single': return null;
case 'left': return node.previousSibling;
case 'right': return node.nextSibling;
}
}

const attributes: Array<[string, unknown]> = [];
function getTextAttributesFromSingleNode( node: ModelNode ) {
if ( node instanceof ModelText ) {
return node.getAttributes();
}

if ( !schema.isInline( node ) ) {
return null;
}

// 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 ] );
if ( !schema.isObject( node ) ) {
return [];
}

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 ) &&
schema.getAttributeProperties( key ).copyFromObject !== false
) {
attributes.push( [ key, value ] );
}
}

return attributes;
}

return attributes;
function* collectCopyOnEnterAttributes( attrs: Iterable<[string, unknown]> ): Iterable<[string, unknown]> {
for ( const [ key, value ] of attrs ) {
if ( schema.getAttributeProperties( key ).copyOnEnter ) {
yield [ key, value ];
}
}
}
}

/**
Expand Down
55 changes: 50 additions & 5 deletions packages/ckeditor5-engine/tests/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -1406,26 +1406,71 @@ describe( 'DocumentSelection', () => {
} );
} );

// #7459
describe( 'ignores inline elements while reading surrounding attributes', () => {
// #7459 and #19853
describe( 'handles inline non-object elements while reading surrounding attributes', () => {
beforeEach( () => {
model.schema.extend( '$text', { allowAttributes: 'foo' } );

model.schema.register( 'softBreak', {
allowWhere: '$text',
isInline: true
} );

model.schema.register( 'inlineBoundary', {
allowWhere: '$text',
isInline: true
} );
} );

it( 'should not inherit attributes from a node before a softBreak when copyOnEnter is disabled', () => {
model.schema.setAttributeProperties( 'foo', { copyOnEnter: false } );
_setModelData( model, '<p><$text foo="true">Foo Bar.</$text><softBreak></softBreak>[]</p>' );

expect( selection.hasAttribute( 'foo' ) ).to.equal( false );
} );

it( 'should not inherit attributes from a node before an inline element', () => {
it( 'should inherit attributes from a node before a softBreak when copyOnEnter is enabled', () => {
model.schema.setAttributeProperties( 'bold', { copyOnEnter: true } );
_setModelData( model, '<p><$text bold="true">Foo Bar.</$text><softBreak></softBreak>[]</p>' );

expect( selection.hasAttribute( 'bold' ) ).to.equal( false );
expect( selection.hasAttribute( 'bold' ) ).to.equal( true );
} );

it( 'should not inherit attributes from a node after an inline element (override gravity)', () => {
it( 'should not inherit attributes from a node after a softBreak when copyOnEnter is disabled (override gravity)', () => {
model.schema.setAttributeProperties( 'foo', { copyOnEnter: false } );
_setModelData( model, '<p>[]<softBreak></softBreak><$text foo="true">Foo Bar.</$text></p>' );

const overrideGravityUid = selection._overrideGravity();

expect( selection.hasAttribute( 'foo' ) ).to.equal( false );

selection._restoreGravity( overrideGravityUid );
} );

it( 'should inherit attributes from a node after a softBreak when copyOnEnter is enabled (override gravity)', () => {
model.schema.setAttributeProperties( 'bold', { copyOnEnter: true } );
_setModelData( model, '<p>[]<softBreak></softBreak><$text bold="true">Foo Bar.</$text></p>' );

const overrideGravityUid = selection._overrideGravity();

expect( selection.hasAttribute( 'bold' ) ).to.equal( true );

selection._restoreGravity( overrideGravityUid );
} );

it( 'should not inherit attributes from a node before a generic inline element even when copyOnEnter is enabled', () => {
model.schema.setAttributeProperties( 'bold', { copyOnEnter: true } );
_setModelData( model, '<p><$text bold="true">Foo Bar.</$text><inlineBoundary></inlineBoundary>[]</p>' );

expect( selection.hasAttribute( 'bold' ) ).to.equal( false );
} );

it( 'should not inherit attributes after generic inline element with override gravity even when copyOnEnter is enabled', () => {
model.schema.setAttributeProperties( 'bold', { copyOnEnter: true } );
_setModelData( model, '<p>[]<inlineBoundary></inlineBoundary><$text bold="true">Foo Bar.</$text></p>' );

const overrideGravityUid = selection._overrideGravity();

expect( selection.hasAttribute( 'bold' ) ).to.equal( false );

selection._restoreGravity( overrideGravityUid );
Expand Down
4 changes: 2 additions & 2 deletions packages/ckeditor5-enter/tests/shiftenter-integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ 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,
'<paragraph>' +
'<$text linkHref="foo" bold="true">Bolded link</$text>' +
Expand All @@ -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 );
} );
} );