Fix inconsistent marker boundary order v2

.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 produces a deterministic marker order 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,81 @@
+/**
+ * @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 so the downcast result is deterministic regardless of the order
+ * markers were added to the marker collection.
+ *
+ * The sort key is the marker's range, ordered "right-to-left" through the document so that
+ * a marker's opening boundary is processed *after* any markers nested inside it. This way
+ * the outer marker wraps the inner ones at conversion time.
+ *
+ * Cases (positions shown as `0123456789`, sort result top-to-bottom):
+ *
+ * 1. Non-overlapping ranges — sorted by position, last range first:
+ *
+ *        a: [--]               →   c, b, a
+ *        b:     [--]
+ *        c:        [--]
+ *
+ * 2. Adjacent ranges (end === start) — treated as non-overlapping:
+ *
+ *        first:  [---]         →   third, second, first
+ *        second:    [---]
+ *        third:        [---]
+ *
+ * 3. Nested ranges (same start, different ends) — inner first, outer last:
+ *
+ *        shorter: [-]          →   shorter, longer
+ *        longer:  [---]
+ *
+ * 4. Partially overlapping ranges — sorted by start position:
+ *
+ *        earlier: [---]        →   later, earlier
+ *        later:     [---]
+ *
+ * 5. Identical ranges — fall back to reverse name comparison:
+ *
+ *        alpha:   [---]        →   charlie, bravo, alpha
+ *        bravo:   [---]
+ *        charlie: [---]
+ *
+ * @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,24 @@ export class DowncastDispatcher extends /* #__PURE__ */ EmitterMixin() {
 			this._convertMarkerAdd( markerName, markerRange, conversionApi );
 		}
 
+		// Sort markers so the downcast result is deterministic regardless of the order
+		// markers were added to the collection.
+		//
+		// "Reverse DOM order" = markers ending later in the document come first, so each
+		// marker's opening boundary is processed after any markers nested inside it.
+		// For overlapping ranges this is best-effort (start position wins, then end position).
+		//
+		// Example: replacing "old" with "new" creates two adjacent markers (delete + insert).
+		// With `markerToElement`, each boundary is a self-closing tag, so the processing
+		// order directly controls where they land at the shared boundary point:
+		//
+		//   Sorted (reverse DOM order):  <DEL-START/>old<DEL-END/><INS-START/>new<INS-END/>
+		//   Insertion order (legacy):    <DEL-START/>old<INS-START/><DEL-END/>new<INS-END/>
+		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 +247,9 @@ export class DowncastDispatcher extends /* #__PURE__ */ EmitterMixin() {
 
 		this._convertInsert( range, conversionApi );
 
-		for ( const [ name, range ] of markers ) {
+		// Sort markers in reverse DOM order for deterministic downcast output.
+		// See the analogous sort in `convertChanges()` for a detailed rationale and examples.
+		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', 'start', viewStartElement );
+		viewWriter.setCustomProperty( 'markerBoundaryType', 'end', viewEndElement );
 
-		// Add "closing" element only if range is not collapsed.
+		// Add "end" 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 end UI elements to find a proper position for "start" element.
+		// It should be after all marker "end" 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' ) === 'end'
+		);
+
+		viewWriter.insert( startViewPosition, viewStartElement );
+		conversionApi.mapper.bindElementToMarker( viewStartElement, data.markerName );
+
 		evt.stop();
 	};
 }

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

@@ -0,0 +1,160 @@
+/**
+ * @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 shorter range (ending earlier) sorts first so that the
+			// longer (outer) marker is processed last and its opening tag wraps the inner one.
+			expect( sortedNames( [
+				[ 'shorter', range( 2, 4 ) ],
+				[ 'longer', range( 2, 6 ) ]
+			] ) ).to.deep.equal( [ 'shorter', 'longer' ] );
+		} );
+
+		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', 'short', 'long' ] );
+		} );
+
+		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 );
+		} );
+	} );
+} );