Clicking content that has a comment causes content data change (resulting with extra autosave)

[mlewand]

📝 Provide detailed reproduction steps (if any)

1. Open sample with comments e.g. https://ckeditor.com/docs/ckeditor5/28.0.0/features/collaboration/comments/comments.html
2. Execute follwoing code in JavaScript console: document.querySelectorAll( '.ck-editor__editable' )[ 0 ].ckeditorInstance.model.document.on( 'change:data', console.log )
3. Make sure JavaScript console is visible.
4. Place selection in content that has some comments.
5. Move content again to a place where there is no comment applied.
6. Repeat steps 4 and 5 twice.

✔️ Expected result

Moving the selection from comment to a regular place should not trigger change:data event.

❌ Actual result

It triggers change data event. In turn there can be autosave calls, undo snapshots created etc.

❓ Possible solution

Initial words from @scofalik 

Ah, okay, I get it. Comment is "data" marker and it is actually changed (or rather - refreshed) through writer.updateMarker( marker ); when the comment is clicked.

📃 Other details

• Browser: Any
• OS: Any

---

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

[scofalik]

1. It is not trivial at the moment because comment marker does affect data. When we "refresh" it, it makes sense that change:data event is generated.
2. However, marker range did not change and "marker is active" property itself does not affect data.
3. Are there cases, where outside-of-model property could affect data?
4. On the first thought, it may make sense but it will not work in RTC (because the property is synced only on one client).
5. So, we should have a rule that outside-of-model properties can be used only in editing pipeline conversion.
6. So, it looks like we can make this change in differ?
7. If not, another idea was to introduce writer.refreshMarker( marker, refreshAffectsData ) and that would handle whether differ should consider this refresh as affecting data or not.
8. We may want to introduce writer.refreshMarker( marker ) anyway, I don't like it that there's no specific method to refresh/reconvert a marker.

What needs to be done:

1. Think again whether we can fix it straight in Differ#hasDataChanges(). How do we use markers? Think how our customers might use markers.
2. If yes, fix in differ.
3. If no, go with Writer#refreshMarker( marker, refreshAffectsData ).

Bonus: talk about this on architect guild.

[Mgsy]

Related: #5796.

[ma2ciek]

Related: #10643.

[scofalik]

After having an internal discussion, we come up with following conclusions:

1. Conversion can be affected by external, non-model properties, like "active comment" in this example.
2. However, since those properties are non-model and not synced with other clients, they should not be used in data pipeline (they should not affect document data). We are making this a rule.
3. The only thing that can change about the marker and that is kept in model is the marker's range.
4. "Refreshing" a marker without changing its range should not affect document data. In such case change:data event should not be send (change should be send instead).

We should fix this straight in Differ#hasDataChanges() - the method should check whether the changed marker actually changed its range.

So, the goal of this issue is to change Differ#hasDataChanges() so that the reported problem will be fixed.

Other than that, two followups.

1. Should Marker#affectsData = true force Marker#managedUsingOperations = true? #10658
2. New API for #refreshItem() and #refreshMarker() #10659

[Reinmar]

We should fix this straight in Differ#hasDataChanges() - the method should check whether the changed marker actually changed its range.

Will this be fixed in this ticket?

[scofalik]

Yes, that's the goal of this ticket.