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

DocumentSelection validates its ranges before they are updated #4265

Closed
Reinmar opened this issue Feb 6, 2018 · 1 comment · Fixed by ckeditor/ckeditor5-engine#1286
Closed
Assignees
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Feb 6, 2018

DocumentSelection contains live ranges. Every live range is updated at model#applyOperation. Then, when it's updated, on change:range document selection validates that new range positions are correct.

This works fine with a single range in the selection, but it breaks badly with multiple ranges, because this happens:

  1. First selection range gets updated on model#applyOperation.
  2. This triggers range1#change:range (BTW, this event name is a bit odd).
  3. This is delegated to selection#change:range.
  4. On selection#change:range the selection starts validating its ranges.
  5. All ranges... at once. So, also the second range which wasn't updated yet.

It seems to me that the selection should validate only the range which has changed.

cc @scofalik

@scofalik
Copy link
Contributor

scofalik commented Feb 6, 2018

I guess it is better to validate only the changed range. DocumentSelection#event:change:range should get the range that has changed.

BTW. There's one more caveat to it, which has a very old issue. If you have two ranges in selection and move content with the one range into the other range, you'll have incorrect state in a selection (where one range includes the other one). But this is not urgent.

scofalik referenced this issue in ckeditor/ckeditor5-engine Feb 19, 2018
Fix: `DocumenSelection#change:range` event will be fired only once after multiple selection live ranges have changed. Closes #1281.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed 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 type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
4 participants