-
Notifications
You must be signed in to change notification settings - Fork 40
I/6501: Handle intersecting ranges when fixing graveyard selection #1834
Conversation
…ulti-range selection in text node.
tests/model/documentselection.js
Outdated
@@ -1778,6 +1778,63 @@ describe( 'DocumentSelection', () => { | |||
// Now it's clear that it's the default range. | |||
expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] ); | |||
} ); | |||
|
|||
it( 'does not break if multi-range selection is inside text nodes', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Does not break" does not explain what should actually happen. I prefer explaining what's the expected result.
it( 'does not break if multi-range selection is inside text nodes', () => { | |
it( 'handles multi-range selection in a text node by merging it into one range', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, as mentioned in the test description – I'd actually expect the ranges to be merged if they are identical. Would that require a lot of work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually expect the ranges to be merged if they are identical.
I don't see a need to merging two identical ranges. With intersecting ones I would have to check.
tests/model/documentselection.js
Outdated
expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 1 ] ); | ||
} ); | ||
|
||
it( 'does not break if multi-range selection is set on object nodes and resulting ranges will intersect', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again – explain what's expected.
tests/model/documentselection.js
Outdated
expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 1 ] ); | ||
} ); | ||
|
||
it( 'does not break if multi-range selection is set on object nodes and resulting ranges will intersect', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again – explain what's expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented – I think that in the text node case, the ranges should be merged too. Also, test descriptions could be better.
Suggested merge commit message (convention)
Fix: Intersecting ranges resulting when fixing graveyard selection no longer breaks the editor. Closes ckeditor/ckeditor5#6501. Closes ckeditor/ckeditor5#6382.
Additional information
PR to merge together: ckeditor/ckeditor5-table#293.
Well, it looks like intersecting ranges will be created on such modifications as in tables: multi-range selection set on table cells and their parent is removed. This would result in many ranges being fixed to the same range (thus intersecting with any other fixed range).
As with other ranges/positions fixes, I am not sure if I have a broad view of this problem. The proposed solution fixes this case and does not break other CKEditor 5 tests buuuut we do not have every case tested.