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

Fixed incorrect selection fixing in some multi-cell selection scenarios #7662

Merged
merged 2 commits into from
Jul 22, 2020

Conversation

scofalik
Copy link
Contributor

Suggested merge commit message (convention)

Fix: Fixed incorrect selection fixing in some multi-cell selection scenarios. Closes #7659.


// If nearest valid selection range has been found - add it in the place of old range.
// If range is equal to any other selection ranges then it is probably due to contents
// of a multi-range selection being removed. See ckeditor/ckeditor5#6501.
if ( selectionRange && !isRangeCollidingWithSelection( selectionRange, this ) ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An explanation behind removing isRangeCollidingWithSelection: after the changes I propose, the selection can be fixed only if there are no ranges in it, so there is no possibility that there would be any collision. This is also the reason why splice is gone -- I've changed the flow a little and now the range is deleted from selection when it is detected that it was removed from the content. So at this moment, there's no need to remove anything, we just need to add the new range.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this get simplified.

@niegowski niegowski self-requested a review July 21, 2020 15:04
@niegowski niegowski self-assigned this Jul 21, 2020
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a brief look and the change looks OK. I mean it's magic but the simplification with restoring selection to a position looks better.

@niegowski niegowski merged commit 43862d6 into master Jul 22, 2020
@niegowski niegowski deleted the i/7659 branch July 22, 2020 08:44
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.

Selection is incorrectly restored in some multi-cell scenarios
3 participants