Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix adjacent marker downcasting #9455

Merged
merged 4 commits into from
Oct 5, 2021

Conversation

bendemboski
Copy link
Contributor

The downcasting of adjacent markers to data was non-deterministic, and could produce different data depending on the order in which the markers were added.

The particular use case where I ran into this was when replacing text using track changes. Highlighting a range of text and typing some new text to replace it would result in an insertion marker followed by a deletion marker. However, the deletion marker was added to the marker collection first. When downcast to data, it would produce (simplified markup):

<suggestion-start name="insertion:1"></suggestion-start>
new text
<suggestion-end name="insertion:1"></suggestion-end>
<suggestion-start name="deletion:2"></suggestion-start>
old text
<suggestion-end name="deletion:2"></suggestion-end>

But if that data were then upcast, the insertion marker would be put in the marker collection first (swapping the order compared to how the track changes plugin added them to the marker collection), and when that was downcast to data, it would produce:

<suggestion-start name="insertion:1"></suggestion-start>
new text
<suggestion-start name="deletion:2"></suggestion-start>
<suggestion-end name="insertion:1"></suggestion-end>
old text
<suggestion-end name="deletion:2"></suggestion-end>

Note the "adjacent" start/end markers are swapped in the HTML. To my knowledge this doesn't violate any guarantees that CKEditor makes about the data (either form will upcast back to the same model DOM), but it does mean that the following will unexpectedly fail:

enableTrackChangesAndReplaceSomeText(editor);
let data = editor.getData();
editor.setData(data);
assert.equal(editor.getData(), data);

It's expected that getData() often won't return the same HTML that was passed to setData(), but I think much less expected that getData() and getData()/setData()/getData() will produce different results.

So this pull request ensures that the order in which downcast marker HTML elements appear is deterministic, rather than being dependent on the order in which the markers were added to the marker collection, and does so in a way that ensures that in the above case, the HTML markers won't "overlap" right at their point of adjacency.

The downcasting of adjacent markers to data was non-deterministic, and could produce different data depending on the order in which the markers were added.

The particular use case where I ran into this was when replacing text using track changes. Highlighting a range of text and typing some new text to replace it would result in an insertion marker followed by a deletion marker. However, the deletion marker was added to the marker collection first. When downcast to data, it would produce (simplified markup):

```html
<suggestion-start name="insertion:1"></suggestion-start>
new text
<suggestion-end name="insertion:1"></suggestion-end>
<suggestion-start name="deletion:2"></suggestion-start>
old text
<suggestion-end name="deletion:2"></suggestion-end>
```

But if that data were then upcast, the insertion marker would be put in the marker collection first (swapping the order compared to how the track changes plugin added them to the marker collection), and when that was downcast to data, it would produce:

```html
<suggestion-start name="insertion:1"></suggestion-start>
new text
<suggestion-start name="deletion:2"></suggestion-start>
<suggestion-end name="insertion:1"></suggestion-end>
old text
<suggestion-end name="deletion:2"></suggestion-end>
```

Note the "adjacent" start/end markers are swapped in the HTML. To my knowledge this doesn't violate any guarantees that CKEditor makes about the data (either form will upcast back to the same model DOM), but it does mean that the following will unexpectedly fail:

```js
enableTrackChangesAndReplaceSomeText(editor);
let data = editor.getData();
editor.setData(data);
assert.equal(editor.getData(), data);
```

It's expected that `getData()` often won't return the same HTML that was passed to `setData()`, but I think much less expected that `getData()` and `getData()`/`setData()`/`getData()` will produce different results.

So this pull request ensures that the order in which downcast marker HTML elements appear is deterministic, rather than being dependent on the order in which the markers were added to the marker collection, and does so in a way that ensures that in the above case, the HTML markers won't "overlap" right at their point of adjacency.
@Reinmar
Copy link
Member

Reinmar commented May 14, 2021

Hi @bendemboski! Sorry to keep you waiting for such a long time. Unfortunately, both guys who can check it are super busy recently, but we'll get there!

@scofalik
Copy link
Contributor

LGTM although one thing that I am missing is a test for a case when there are multiple roots. compareWith() returns 'different' in such case and it might not be obvious that the sorting will be alright in that case.

@arkflpc arkflpc self-requested a review October 5, 2021 08:44
@arkflpc
Copy link
Contributor

arkflpc commented Oct 5, 2021

@scofalik, it's not possible to have markers from different roots in toView() method invocation.

@arkflpc arkflpc merged commit 5cd38c0 into ckeditor:master Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants