Fix inconsistent marker boundary order

.changelog/20260323163157_ck_19975_marker_boundary_order.md

@@ -0,0 +1,11 @@
+---
+type: Fix
+scope:
+  - ckeditor5-engine
+closes:
+  - 19975
+---
+
+Fixed the editing downcast order of adjacent marker UI boundaries so marker ends and starts are rendered consistently with the model and data output.
+
+The editing pipeline now uses stable marker ordering and preserves the expected boundary order when adjacent markers are added together or when the second adjacent marker is added later.

packages/ckeditor5-engine/src/controller/datacontroller.ts

@@ -669,45 +669,5 @@ function _getMarkersRelativeToElement( element: ModelElement ): Map<string, Mode
 		}
 	}
 
-	// Sort the markers in a stable fashion to ensure that the order in which they are
-	// added to the model's marker collection does not affect how they are
-	// downcast. One particular use case that we are targeting here, is one where
-	// two markers are adjacent but not overlapping, such as an insertion/deletion
-	// suggestion pair representing the replacement of a range of text. In this
-	// case, putting the markers in DOM order causes the first marker's end to be
-	// serialized right after the second marker's start, while putting the markers
-	// in reverse DOM order causes it to be right before the second marker's
-	// start. So, we sort these in a way that ensures non-intersecting ranges are in
-	// reverse DOM order, and intersecting ranges are in something approximating
-	// reverse DOM order (since reverse DOM order doesn't have a precise meaning
-	// when working with intersecting ranges).
-	result.sort( ( [ n1, r1 ], [ n2, r2 ] ) => {
-		if ( r1.end.compareWith( r2.start ) !== 'after' ) {
-			// m1.end <= m2.start -- m1 is entirely <= m2
-			return 1;
-		} else if ( r1.start.compareWith( r2.end ) !== 'before' ) {
-			// m1.start >= m2.end -- m1 is entirely >= m2
-			return -1;
-		} else {
-			// they overlap, so use their start positions as the primary sort key and
-			// end positions as the secondary sort key
-			switch ( r1.start.compareWith( r2.start ) ) {
-				case 'before':
-					return 1;
-				case 'after':
-					return -1;
-				default:
-					switch ( r1.end.compareWith( r2.end ) ) {
-						case 'before':
-							return 1;
-						case 'after':
-							return -1;
-						default:
-							return n2.localeCompare( n1 );
-					}
-			}
-		}
-	} );
-
 	return new Map( result );
 }

packages/ckeditor5-engine/src/conversion/comparemarkers.ts

@@ -0,0 +1,49 @@
+/**
+ * @license Copyright (c) 2003-2026, CKSource Holding sp. z o.o. All rights reserved.
+ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-licensing-options
+ */
+
+/**
+ * @module engine/conversion/comparemarkers
+ */
+
+import type { ModelRange } from '../model/range.js';
+
+/**
+ * Sorts markers in a stable fashion so their addition order does not affect downcast output.
+ *
+ * Markers are ordered in reverse DOM order for non-intersecting ranges. For intersecting ranges,
+ * the start position is the primary sort key and the end position is the secondary sort key.
+ *
+ * @internal
+ */
+export function compareMarkersForDowncast(
+	[ name1, range1 ]: readonly [ string, ModelRange ],
+	[ name2, range2 ]: readonly [ string, ModelRange ]
+): number {
+	if ( range1.end.compareWith( range2.start ) !== 'after' ) {
+		// m1.end <= m2.start -- m1 is entirely <= m2.
+		return 1;
+	} else if ( range1.start.compareWith( range2.end ) !== 'before' ) {
+		// m1.start >= m2.end -- m1 is entirely >= m2.
+		return -1;
+	} else {
+		// They overlap, so use their start positions as the primary sort key and
+		// end positions as the secondary sort key.
+		switch ( range1.start.compareWith( range2.start ) ) {
+			case 'before':
+				return 1;
+			case 'after':
+				return -1;
+			default:
+				switch ( range1.end.compareWith( range2.end ) ) {
+					case 'before':
+						return 1;
+					case 'after':
+						return -1;
+					default:
+						return name2.localeCompare( name1 );
+				}
+		}
+	}
+}

packages/ckeditor5-engine/src/conversion/downcastdispatcher.ts

@@ -8,6 +8,7 @@
  */
 
 import { ModelConsumable } from './modelconsumable.js';
+import { compareMarkersForDowncast } from './comparemarkers.js';
 import { ModelRange } from '../model/range.js';
 
 import { EmitterMixin } from '@ckeditor/ckeditor5-utils';
@@ -200,8 +201,23 @@ export class DowncastDispatcher extends /* #__PURE__ */ EmitterMixin() {
 			this._convertMarkerAdd( markerName, markerRange, conversionApi );
 		}
 
+		// Sort the markers in a stable fashion to ensure that the order in which they are
+		// added to the model's marker collection does not affect how they are
+		// downcast. One particular use case that we are targeting here, is one where
+		// two markers are adjacent but not overlapping, such as an insertion/deletion
+		// suggestion pair representing the replacement of a range of text. In this
+		// case, putting the markers in DOM order causes the first marker's end to be
+		// serialized right after the second marker's start, while putting the markers
+		// in reverse DOM order causes it to be right before the second marker's
+		// start. So, we sort these in a way that ensures non-intersecting ranges are in
+		// reverse DOM order, and intersecting ranges are in something approximating
+		// reverse DOM order (since reverse DOM order doesn't have a precise meaning
+		// when working with intersecting ranges).
+		const markersToAdd = differ.getMarkersToAdd()
+			.sort( ( a, b ) => compareMarkersForDowncast( [ a.name, a.range ], [ b.name, b.range ] ) );
+
 		// After the view is updated, convert markers which have changed.
-		for ( const change of differ.getMarkersToAdd() ) {
+		for ( const change of markersToAdd ) {
 			this._convertMarkerAdd( change.name, change.range, conversionApi );
 		}
 
@@ -230,7 +246,19 @@ export class DowncastDispatcher extends /* #__PURE__ */ EmitterMixin() {
 
 		this._convertInsert( range, conversionApi );
 
-		for ( const [ name, range ] of markers ) {
+		// Sort the markers in a stable fashion to ensure that the order in which they are
+		// added to the model's marker collection does not affect how they are
+		// downcast. One particular use case that we are targeting here, is one where
+		// two markers are adjacent but not overlapping, such as an insertion/deletion
+		// suggestion pair representing the replacement of a range of text. In this
+		// case, putting the markers in DOM order causes the first marker's end to be
+		// serialized right after the second marker's start, while putting the markers
+		// in reverse DOM order causes it to be right before the second marker's
+		// start. So, we sort these in a way that ensures non-intersecting ranges are in
+		// reverse DOM order, and intersecting ranges are in something approximating
+		// reverse DOM order (since reverse DOM order doesn't have a precise meaning
+		// when working with intersecting ranges).
+		for ( const [ name, range ] of Array.from( markers ).sort( compareMarkersForDowncast ) ) {
 			this._convertMarkerAdd( name, range, conversionApi );
 		}
 

packages/ckeditor5-engine/src/conversion/downcasthelpers.ts

@@ -1335,16 +1335,24 @@ export function insertUIElement( elementCreator: DowncastMarkerElementCreatorFun
 		const mapper = conversionApi.mapper;
 		const viewWriter = conversionApi.writer;
 
-		// Add "opening" element.
-		viewWriter.insert( mapper.toViewPosition( markerRange.start ), viewStartElement );
-		conversionApi.mapper.bindElementToMarker( viewStartElement, data.markerName );
+		viewWriter.setCustomProperty( 'markerBoundaryType', 'opening', viewStartElement );
+		viewWriter.setCustomProperty( 'markerBoundaryType', 'closing', viewEndElement );
 
 		// Add "closing" element only if range is not collapsed.
 		if ( !markerRange.isCollapsed ) {
 			viewWriter.insert( mapper.toViewPosition( markerRange.end ), viewEndElement );
 			conversionApi.mapper.bindElementToMarker( viewEndElement, data.markerName );
 		}
 
+		// Jump over closing UI elements to find a proper position for "opening" element.
+		// It should be after all marker closing UI elements as markers conversion should be triggered in reverse DOM order.
+		const startViewPosition = mapper.toViewPosition( markerRange.start ).getLastMatchingPosition( ( { item } ) =>
+			item.is( 'uiElement' ) && item.getCustomProperty( 'markerBoundaryType' ) === 'closing'
+		);
+
+		viewWriter.insert( startViewPosition, viewStartElement );
+		conversionApi.mapper.bindElementToMarker( viewStartElement, data.markerName );
+
 		evt.stop();
 	};
 }

packages/ckeditor5-engine/tests/conversion/comparemarkers.js

@@ -0,0 +1,159 @@
+/**
+ * @license Copyright (c) 2003-2026, CKSource Holding sp. z o.o. All rights reserved.
+ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-licensing-options
+ */
+
+import { Model } from '../../src/model/model.js';
+import { ModelText } from '../../src/model/text.js';
+import { compareMarkersForDowncast } from '../../src/conversion/comparemarkers.js';
+
+describe( 'compareMarkersForDowncast()', () => {
+	let model, root;
+
+	beforeEach( () => {
+		model = new Model();
+		root = model.document.createRoot();
+
+		root._appendChild( [
+			new ModelText( 'abcdefghij' )
+		] );
+	} );
+
+	function range( startOffset, endOffset ) {
+		return model.createRange(
+			model.createPositionFromPath( root, [ startOffset ] ),
+			model.createPositionFromPath( root, [ endOffset ] )
+		);
+	}
+
+	function sortedNames( markers ) {
+		return markers.sort( compareMarkersForDowncast ).map( ( [ name ] ) => name );
+	}
+
+	describe( 'non-overlapping ranges', () => {
+		it( 'should sort in reverse DOM order', () => {
+			expect( sortedNames( [
+				[ 'a', range( 0, 2 ) ],
+				[ 'b', range( 4, 6 ) ],
+				[ 'c', range( 7, 9 ) ]
+			] ) ).to.deep.equal( [ 'c', 'b', 'a' ] );
+		} );
+
+		it( 'should sort in reverse DOM order regardless of initial order', () => {
+			expect( sortedNames( [
+				[ 'c', range( 7, 9 ) ],
+				[ 'a', range( 0, 2 ) ],
+				[ 'b', range( 4, 6 ) ]
+			] ) ).to.deep.equal( [ 'c', 'b', 'a' ] );
+		} );
+
+		it( 'should treat adjacent ranges (end == start) as non-overlapping', () => {
+			expect( sortedNames( [
+				[ 'first', range( 0, 3 ) ],
+				[ 'second', range( 3, 6 ) ],
+				[ 'third', range( 6, 9 ) ]
+			] ) ).to.deep.equal( [ 'third', 'second', 'first' ] );
+		} );
+	} );
+
+	describe( 'overlapping ranges', () => {
+		it( 'should sort outer marker after inner marker (outer starts earlier)', () => {
+			expect( sortedNames( [
+				[ 'inner', range( 3, 5 ) ],
+				[ 'outer', range( 1, 7 ) ]
+			] ) ).to.deep.equal( [ 'inner', 'outer' ] );
+		} );
+
+		it( 'should sort by start position first for partially overlapping ranges', () => {
+			expect( sortedNames( [
+				[ 'earlier', range( 1, 5 ) ],
+				[ 'later', range( 3, 7 ) ]
+			] ) ).to.deep.equal( [ 'later', 'earlier' ] );
+		} );
+
+		it( 'should use end position as secondary key when starts are equal', () => {
+			// Same start — the longer range (ending later) sorts first, shorter after.
+			expect( sortedNames( [
+				[ 'shorter', range( 2, 4 ) ],
+				[ 'longer', range( 2, 6 ) ]
+			] ) ).to.deep.equal( [ 'longer', 'shorter' ] );
+		} );
+
+		it( 'should sort three nested markers from innermost to outermost', () => {
+			expect( sortedNames( [
+				[ 'outer', range( 0, 9 ) ],
+				[ 'mid', range( 2, 7 ) ],
+				[ 'inner', range( 4, 5 ) ]
+			] ) ).to.deep.equal( [ 'inner', 'mid', 'outer' ] );
+		} );
+
+		it( 'should sort three nested markers from innermost to outermost regardless of initial order', () => {
+			expect( sortedNames( [
+				[ 'inner', range( 4, 5 ) ],
+				[ 'outer', range( 0, 9 ) ],
+				[ 'mid', range( 2, 7 ) ]
+			] ) ).to.deep.equal( [ 'inner', 'mid', 'outer' ] );
+		} );
+	} );
+
+	describe( 'identical ranges', () => {
+		it( 'should fall back to reverse name comparison for identical ranges', () => {
+			expect( sortedNames( [
+				[ 'alpha', range( 2, 5 ) ],
+				[ 'charlie', range( 2, 5 ) ],
+				[ 'bravo', range( 2, 5 ) ]
+			] ) ).to.deep.equal( [ 'charlie', 'bravo', 'alpha' ] );
+		} );
+
+		it( 'should preserve order for markers with identical ranges and names', () => {
+			const markers = [
+				[ 'same', range( 2, 5 ) ],
+				[ 'same', range( 2, 5 ) ]
+			];
+
+			const result = compareMarkersForDowncast( markers[ 0 ], markers[ 1 ] );
+
+			expect( result ).to.equal( 0 );
+		} );
+	} );
+
+	describe( 'mixed scenarios', () => {
+		it( 'should correctly sort a mix of non-overlapping and overlapping ranges', () => {
+			expect( sortedNames( [
+				[ 'solo', range( 8, 9 ) ],
+				[ 'outer', range( 0, 6 ) ],
+				[ 'inner', range( 2, 4 ) ]
+			] ) ).to.deep.equal( [ 'solo', 'inner', 'outer' ] );
+		} );
+
+		it( 'should correctly sort overlapping ranges sharing the same start with a non-overlapping range', () => {
+			expect( sortedNames( [
+				[ 'short', range( 0, 3 ) ],
+				[ 'long', range( 0, 7 ) ],
+				[ 'separate', range( 8, 9 ) ]
+			] ) ).to.deep.equal( [ 'separate', 'long', 'short' ] );
+		} );
+
+		it( 'should sort many markers consistently regardless of initial order', () => {
+			const expected = [ 'e', 'd', 'c', 'b', 'a' ];
+
+			// Reversed initial order.
+			expect( sortedNames( [
+				[ 'a', range( 0, 2 ) ],
+				[ 'b', range( 2, 4 ) ],
+				[ 'c', range( 4, 6 ) ],
+				[ 'd', range( 6, 8 ) ],
+				[ 'e', range( 8, 10 ) ]
+			] ) ).to.deep.equal( expected );
+
+			// Random initial order.
+			expect( sortedNames( [
+				[ 'c', range( 4, 6 ) ],
+				[ 'e', range( 8, 10 ) ],
+				[ 'a', range( 0, 2 ) ],
+				[ 'd', range( 6, 8 ) ],
+				[ 'b', range( 2, 4 ) ]
+			] ) ).to.deep.equal( expected );
+		} );
+	} );
+} );