Fix inconsistent marker boundary order by arkflpc · Pull Request #19995 · ckeditor/ckeditor5

arkflpc · 2026-03-23T15:32:24Z

🚀 Summary

Fixes incorrect order of adjacent marker boundary elements in editing downcast.

📌 Related issues

Closes Incorrect order of adjacent marker boundary elements in editing downcast #19975

💡 Additional information

Optional: Notes on decisions, edge cases, or anything helpful for reviewers.

🧾 Checklists

Use the following checklists to ensure important areas were not overlooked.
This does not apply to feature-branch merges.
If an item is not relevant to this type of change, simply leave it unchecked.

Author checklist

Reviewer checklist

PR description explains the changes and the chosen approach (especially when performance, API, or UX is affected).
The changelog entry is clear, user‑ or integrator-facing, and it describes any breaking changes.
All new external dependencies have been approved and mentioned in LICENSE.md (if any).
All human-readable, translateable strings in this PR been introduced using t() (if any).
I manually verified the change (e.g., in manual tests or documentation).
The target branch is correct.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix prepared a fix for the issue found in the latest run.

✅ Fixed: Accidentally committed review comment in source file
- Removed the stray internal // REVIEW: note from comparemarkers.ts without altering any functional code.

Or push these changes by commenting:

@cursor push 09d25c103e

Preview (09d25c103e)

diff --git a/packages/ckeditor5-engine/src/conversion/comparemarkers.ts b/packages/ckeditor5-engine/src/conversion/comparemarkers.ts
--- a/packages/ckeditor5-engine/src/conversion/comparemarkers.ts
+++ b/packages/ckeditor5-engine/src/conversion/comparemarkers.ts
@@ -3,7 +3,6 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-licensing-options
  */
 
-// REVIEW: Let's call this file `comparemarkers.ts` (adjust module name and imports)
 /**
  * @module engine/conversion/comparemarkers
  */

_{This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.}

…p. Added more tests.

niegowski · 2026-03-30T19:39:57Z

-	// 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( compareMarkersForDowncast );


This was not needed as the markers map was already sorted in DowncastDispatcher#convert().

scofalik · 2026-03-31T08:54:51Z

+		// 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).


I propose to add an example, and maybe shorten the text then. It will be easier to imagine what is going on here.

scofalik · 2026-03-31T08:55:09Z

+		viewWriter.setCustomProperty( 'markerBoundaryType', 'opening', viewStartElement );
+		viewWriter.setCustomProperty( 'markerBoundaryType', 'closing', viewEndElement );


I propose 'start' and 'end' as values, seems simpler, and we always use these two nouns.

…nd" to better reflect used terms.

charlttsie · 2026-04-08T14:40:39Z

I've tested the setData(getData()) and paste operations for performance regressions using editor content with multiple (692) markers, mostly (665) those converted with markerToElement (insertion/removal suggestion markers). The results look good to me - they don't indicate any significant timing differences:

`setData(getData())`

Chrome

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	1182.7	1191.6	1212.0	1265.2	1212.88
PR	1186.5	1187.9	1189.7	1214.5	1194.65	-1.50%

Firefox

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	1620.7	1641.6	1701.6	1743.3	1676.82
PR	1636.1	1657.3	1660.5	1763.6	1679.37	+0.15%

Safari

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	759.0	767.0	806.0	816.0	787.00
PR	757.0	770.0	776.0	817.0	780.00	-0.89%

Paste

Chrome

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	284.9	285.6	285.7	287.0	285.80
PR	279.0	280.5	281.5	281.6	280.65	-1.80%

Firefox

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	452.5	461.1	495.5	497.8	476.73
PR	440.1	442.9	452.0	518.8	463.43	-2.79%

Safari

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	312.0	325.0	337.0	348.0	330.50
PR	329.0	333.0	334.0	342.0	334.50	+1.21%

niegowski · 2026-04-08T15:54:35Z

those converted with markerToElement (insertion/removal suggestion markers).

I'm not sure if you are mentioning the TC insertions/deletions or some custom plugins? AFAIR, TC uses markerToData, not markerToElement. Are we sure that markerToElement is tested?

charlttsie · 2026-04-09T12:44:24Z

You're right, in the scenarios above markerToData was tested, so I retested with markerToElement. I defined custom markers in a way specified in https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_conversion_downcasthelpers-DowncastHelpers.html#function-markerToElement, using the following code snippets:

editor.conversion.for( 'dataDowncast' ).markerToElement( {
     model: 'marker-to-element',
     view: ( markerData, conversionApi ) => {
         const { writer } = conversionApi;

         return writer.createUIElement( 'marker-to-element', {
             name: markerData.markerName.substr( 'marker-to-element:'.length )
         } );
     }
} );

editor.conversion.for( 'editingDowncast' ).markerToElement( {
   model: 'marker-to-element',
   view: ( markerData, conversionApi ) => {
       const { writer } = conversionApi;

       return writer.createUIElement( 'marker-to-element', {
           name: markerData.markerName.substr( 'marker-to-element:'.length )
       } );
   }
} );

and used them in this HTML data, where each <suggestion-start>/<suggestion-end> marker was replaced with that custom marker, so there are 661 such markers in total. For the paste test, I tested pasting the sample content without markers into the document with markers.

Here are the results:

`setData(getData())`

Chrome

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	896.7	897.4	904.4	904.6	900.78
PR	897.5	904.4	904.7	910.3	904.22	+0.38%

Firefox

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	1239.2	1239.8	1286.5	1295.6	1265.27
PR	1224.5	1262.2	1273.3	1274.6	1258.66	-0.52%

Safari

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	584.0	593.0	596.0	597.0	592.50
PR	592.0	594.0	605.0	608.0	599.75	+1.22%

Paste

Chrome

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	198.2	200.5	200.8	202.4	200.48
PR	199.9	202.1	202.6	206.2	202.70	+1.11%

Firefox

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	365.3	366.2	379.4	382.0	373.20
PR	373.7	374.8	381.2	394.4	381.02	+2.10%

Safari

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	243.0	251.0	254.0	257.0	251.25
PR	247.0	252.0	263.0	266.0	257.00	+2.29%

f1ames · 2026-04-15T09:36:54Z

This PR has one consequence that doesn't seem directly related to what it is supposing to fix (at least looking into #19975). I'm not sure if it's intended or not.

It changes the order of marker opening tags that have the same start position but different end position. Imagine the model like below:

<paragraph>
    [marker1-start]
    [marker2-start]
    Foo
    [marker2-end]
    Bar
    [marker1-end]
</paragraph>

before the change, when converted to view the result was:

<p>
    <span marker1-start></span>
    <span marker2-start></span>
        Foo
    <span marker2-end></span>
        Bar
    <span marker1-end></span>
</p>

so that the "longer" marker opening tag is first. After this change, opening tags order is reversed:

<p>
    <span marker2-start></span>
    <span marker1-start></span>
        Foo
    <span marker2-end></span>
        Bar
    <span marker1-end></span>
</p>

This is directly related to how the sorting function works and comparing marker ends if start position is equal:

				switch ( range1.end.compareWith( range2.end ) ) {
					case 'before':
						return 1;
					case 'after':
						return -1;

I see this function is there for a long time and used in other places, so the question is if and why it makes sense.

This change has a direct impact on AI Review and broke internal logic that was based on markers order in view, see https://github.com/ckeditor/ckeditor5-commercial/pull/9727#issuecomment-4244177041.

Fix inconsistent marker boundary order

niegowski · 2026-04-17T15:03:47Z

Before the PR, marker sorting happened in _getMarkersRelativeToElement() in DataController. This function is only used by DataController#toView() when you pass a model element - document fragments just used markers as-is, no sorting.

The PR moved that sorting out of _getMarkersRelativeToElement() and into DowncastDispatcher - both in convertChanges() (editing pipeline) and convert() (data pipeline). The sort logic was copied as-is into the new compareMarkersForDowncast helper, and the original sort in _getMarkersRelativeToElement() was removed. However, the original sort had a bug - when two markers shared the same start position, it placed the shorter (inner) marker before the longer (outer) one, inverting the expected nesting order.

The code that broke uses DataController#toView() with a document fragment that has its own markers collection. Before the PR, this path had no sorting at all - markers were processed in whatever order they were attached to the fragment. After the PR, the new sort in DowncastDispatcher.convert() kicked in for this path too, changing the marker order and breaking the assumptions the consuming code relied on.

niegowski · 2026-04-17T15:07:03Z

Bug fixed in: #20084
Commit: 0333348

Fix inconsistent marker boundary order

3a1b20b

arkflpc requested review from Mati365 and niegowski March 23, 2026 15:32

Changelog message.

cf2a7d5

cursor Bot reviewed Mar 23, 2026

View reviewed changes

Comment thread packages/ckeditor5-engine/src/conversion/comparemarkers.ts Outdated

arkflpc and others added 3 commits March 26, 2026 12:31

Removed leftover comment.

a643568

Adjusted list tests.

5829a2a

Removed double markers sorting in data pipeline downcast. Code cleanu…

18d11c3

…p. Added more tests.

niegowski reviewed Mar 30, 2026

View reviewed changes

scofalik reviewed Mar 31, 2026

View reviewed changes

niegowski added 2 commits March 31, 2026 10:59

Replaced "opening", "closing" custom property values with "start", "e…

f8adbee

…nd" to better reflect used terms.

Updated code comment.

522621c

scofalik approved these changes Mar 31, 2026

View reviewed changes

Merge branch 'master' into ck/19975-marker-boundary-order

8ea15f7

pomek merged commit b2668d4 into master Apr 14, 2026
12 checks passed

pomek deleted the ck/19975-marker-boundary-order branch April 14, 2026 10:37

pomek mentioned this pull request Apr 15, 2026

Revert "Fix inconsistent marker boundary order" #20075

Merged

f1ames mentioned this pull request Apr 15, 2026

Incorrect order of adjacent marker boundary elements in editing downcast #19975

Closed

niegowski pushed a commit that referenced this pull request Apr 17, 2026

Merge pull request #19995 from ckeditor/ck/19975-marker-boundary-order

26a4394

Fix inconsistent marker boundary order

niegowski mentioned this pull request Apr 21, 2026

Fix inconsistent marker boundary order v2 #20084

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix inconsistent marker boundary order#19995

Fix inconsistent marker boundary order#19995
pomek merged 8 commits intomasterfrom
ck/19975-marker-boundary-order

arkflpc commented Mar 23, 2026 •

edited

Loading

Uh oh!

cursor Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

niegowski Mar 30, 2026

Uh oh!

scofalik Mar 31, 2026

Uh oh!

scofalik Mar 31, 2026

Uh oh!

charlttsie commented Apr 8, 2026

Uh oh!

niegowski commented Apr 8, 2026

Uh oh!

charlttsie commented Apr 9, 2026

Uh oh!

Uh oh!

f1ames commented Apr 15, 2026

Uh oh!

niegowski commented Apr 17, 2026

Uh oh!

niegowski commented Apr 17, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		viewWriter.setCustomProperty( 'markerBoundaryType', 'opening', viewStartElement );
		viewWriter.setCustomProperty( 'markerBoundaryType', 'closing', viewEndElement );

Uh oh!

Conversation

arkflpc commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Summary

📌 Related issues

💡 Additional information

🧾 Checklists

Author checklist

Reviewer checklist

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

niegowski Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

scofalik Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

scofalik Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

charlttsie commented Apr 8, 2026

setData(getData())

Chrome

Firefox

Safari

Paste

Chrome

Firefox

Safari

Uh oh!

niegowski commented Apr 8, 2026

Uh oh!

charlttsie commented Apr 9, 2026

setData(getData())

Chrome

Firefox

Safari

Paste

Chrome

Firefox

Safari

Uh oh!

Uh oh!

f1ames commented Apr 15, 2026

Uh oh!

niegowski commented Apr 17, 2026

Uh oh!

niegowski commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

arkflpc commented Mar 23, 2026 •

edited

Loading

cursor Bot left a comment •

edited

Loading

`setData(getData())`

`setData(getData())`

niegowski commented Apr 17, 2026 •

edited

Loading