Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #1834 from ckeditor/i/6501
Browse files Browse the repository at this point in the history
Fix: Intersecting ranges resulting when fixing graveyard selection no longer break the editor. Closes ckeditor/ckeditor5#6501. Closes ckeditor/ckeditor5#6382.
  • Loading branch information
Reinmar authored Apr 14, 2020
2 parents 08e8294 + c0364a2 commit c208ce1
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 7 deletions.
12 changes: 9 additions & 3 deletions src/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -1137,14 +1137,16 @@ class LiveSelection extends Selection {
liveRange.detach();

// If nearest valid selection range has been found - add it in the place of old range.
if ( selectionRange ) {
// 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 ) ) {
// Check the range, convert it to live range, bind events, etc.
const newRange = this._prepareRange( selectionRange );

// Add new range in the place of old range.
this._ranges.splice( index, 0, newRange );
}
// If nearest valid selection range cannot be found - just removing the old range is fine.
// If nearest valid selection range cannot be found or is intersecting with other selection ranges removing the old range is fine.
}
}

Expand All @@ -1164,7 +1166,6 @@ function getAttrsIfCharacter( node ) {

// Removes selection attributes from element which is not empty anymore.
//
// @private
// @param {module:engine/model/model~Model} model
// @param {module:engine/model/batch~Batch} batch
function clearAttributesStoredInElement( model, batch ) {
Expand All @@ -1190,3 +1191,8 @@ function clearAttributesStoredInElement( model, batch ) {
}
}
}

// Checks if range collides with any of selection ranges.
function isRangeCollidingWithSelection( range, selection ) {
return !selection._ranges.every( selectionRange => !range.isEqual( selectionRange ) );
}
65 changes: 61 additions & 4 deletions tests/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -1710,7 +1710,7 @@ describe( 'DocumentSelection', () => {
} );

describe( 'MoveOperation to graveyard', () => {
it( 'fix selection range if it ends up in graveyard #1', () => {
it( 'fix selection range if it ends up in graveyard - collapsed selection', () => {
selection._setTo( new Position( root, [ 1, 3 ] ) );

model.applyOperation(
Expand All @@ -1725,7 +1725,7 @@ describe( 'DocumentSelection', () => {
expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 2 ] );
} );

it( 'fix selection range if it ends up in graveyard #2', () => {
it( 'fix selection range if it ends up in graveyard - text from non-collapsed selection is moved', () => {
selection._setTo( [ new Range( new Position( root, [ 1, 2 ] ), new Position( root, [ 1, 4 ] ) ) ] );

model.applyOperation(
Expand All @@ -1740,7 +1740,7 @@ describe( 'DocumentSelection', () => {
expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 2 ] );
} );

it( 'fix selection range if it ends up in graveyard #3', () => {
it( 'fix selection range if it ends up in graveyard - parent of non-collapsed selection is moved', () => {
selection._setTo( [ new Range( new Position( root, [ 1, 1 ] ), new Position( root, [ 1, 2 ] ) ) ] );

model.applyOperation(
Expand All @@ -1755,7 +1755,7 @@ describe( 'DocumentSelection', () => {
expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 6 ] );
} );

it( 'fix selection range if it ends up in graveyard #4 - whole content removed', () => {
it( 'fix selection range if it ends up in graveyard - whole content removed', () => {
model.applyOperation(
new MoveOperation(
new Position( root, [ 0 ] ),
Expand All @@ -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( 'handles multi-range selection in a text node by merging it into one range (resulting in collapsed ranges)', () => {
const ranges = [
new Range( new Position( root, [ 1, 1 ] ), new Position( root, [ 1, 2 ] ) ),
new Range( new Position( root, [ 1, 3 ] ), new Position( root, [ 1, 4 ] ) )
];

selection._setTo( ranges );

model.applyOperation(
new MoveOperation(
new Position( root, [ 1, 1 ] ),
4,
new Position( doc.graveyard, [ 0 ] ),
doc.version
)
);

expect( selection.rangeCount ).to.equal( 1 );
expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 1 ] );
expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 1 ] );
} );

it( 'handles multi-range selection on object nodes by merging it into one range (resulting in non-collapsed ranges)', () => {
model.schema.register( 'outer', {
isObject: true
} );
model.schema.register( 'inner', {
isObject: true,
allowIn: 'outer'
} );

root._removeChildren( 0, root.childCount );
root._insertChild( 0, [
new Element( 'outer', [], [ new Element( 'inner' ), new Element( 'inner' ), new Element( 'inner' ) ] )
] );

const ranges = [
new Range( new Position( root, [ 0, 0 ] ), new Position( root, [ 0, 1 ] ) ),
new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 0, 2 ] ) )
];

selection._setTo( ranges );

model.applyOperation(
new MoveOperation(
new Position( root, [ 0, 0 ] ),
2,
new Position( doc.graveyard, [ 0 ] ),
doc.version
)
);

expect( selection.rangeCount ).to.equal( 1 );
expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] );
expect( selection.getLastPosition().path ).to.deep.equal( [ 0, 1 ] );
} );
} );

it( '`DocumentSelection#change:range` event should be fire once even if selection contains multi-ranges', () => {
Expand Down
7 changes: 7 additions & 0 deletions tests/model/range.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,13 @@ describe( 'Range', () => {
const otherRange = new Range( new Position( otherRoot, [ 0 ] ), new Position( otherRoot, [ 1, 4 ] ) );
expect( range.isIntersecting( otherRange ) ).to.be.false;
} );

it( 'should return false if ranges are collapsed and equal', () => {
range = new Range( new Position( root, [ 1, 1 ] ) );
const otherRange = new Range( new Position( otherRoot, [ 1, 1 ] ) );

expect( range.isIntersecting( otherRange ) ).to.be.false;
} );
} );

describe( 'static constructors', () => {
Expand Down

0 comments on commit c208ce1

Please sign in to comment.