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

Transforming collapsed range and stickiness #4194

Closed
scofalik opened this issue Oct 6, 2017 · 1 comment · Fixed by ckeditor/ckeditor5-engine#1160
Closed

Transforming collapsed range and stickiness #4194

scofalik opened this issue Oct 6, 2017 · 1 comment · Fixed by ckeditor/ckeditor5-engine#1160
Assignees
Labels
package:engine status:discussion type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@scofalik
Copy link
Contributor

scofalik commented Oct 6, 2017

There is a problem with transforming collapsed ranges. I have a feeling that we already discussed it, but I am not sure whether we discussed this case exactly.

You probably remember an issue, a long time ago, range was incorrectly transformed when it was collapsed and nodes were inserted at that position:
foo[]bar --> foo]xxx[bar.
Not only the range got expanded but also the boundaries were reversed.

This was fixed so that when range is collapsed, it doesn't get expanded and it goes after the inserted content:
foo[]bar --> fooxxx[]bar.

But this is incorrect in OT, when the range is "sticky". This creates problems when transforming MergeDelta by MergeDelta. Since the move operation is "sticky", the nodes inserted next to range should be included. So what we expect in this case is:
[] -> [xxx] instead of [] -> xxx[].

So I propose that instead of ignoring isSticky for collapsed comment, we handle it correctly. Of course with isSticky == false, we go with current handling, so foo[]bar becomes fooxxx[]bar.

@scofalik
Copy link
Contributor Author

scofalik commented Oct 6, 2017

I've checked tests and as far as unit tests goes, this change affect just one test, which checks exactly the outcome, so it seems that in unit tests we never use collapsed range + isSticky = true.

Reinmar referenced this issue in ckeditor/ckeditor5-engine Oct 11, 2017
Fix: `model.Range` will now be extended if it was collapsed and it was transformed by insertion. Closes #1159.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 13 milestone Oct 9, 2019
@mlewand mlewand added module:model status:discussion type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine status:discussion type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
2 participants