From bc94b5c032790eee1726973a0c508bcbb72e55c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= <jodator@jodator.net>
Date: Mon, 30 Mar 2020 16:08:34 +0200
Subject: [PATCH 1/7] Do not fix graveyard range if it would intersect with
existing selection.
---
src/model/documentselection.js | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/model/documentselection.js b/src/model/documentselection.js
index 0ebf2f1aa..947d1fd69 100644
--- a/src/model/documentselection.js
+++ b/src/model/documentselection.js
@@ -1137,7 +1137,7 @@ 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 ( selectionRange && !isIntersecting( this._ranges, selectionRange ) ) {
// Check the range, convert it to live range, bind events, etc.
const newRange = this._prepareRange( selectionRange );
@@ -1190,3 +1190,15 @@ function clearAttributesStoredInElement( model, batch ) {
}
}
}
+
+// Checks if range intersects with any given ranges.
+function isIntersecting( ranges, selectionRange ) {
+ for ( let i = 0; i < ranges.length; i++ ) {
+ if ( selectionRange.isIntersecting( ranges[ i ] ) ) {
+ // It is also fine - as we can have multiple ranges in the selection.
+ return true;
+ }
+ }
+
+ return false;
+}
From 3e0cfb5442b08baf5c72264295059722daaae4da Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= <jodator@jodator.net>
Date: Tue, 31 Mar 2020 10:43:41 +0200
Subject: [PATCH 2/7] Define range intersection of collapsed ranges.
---
tests/model/range.js | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tests/model/range.js b/tests/model/range.js
index a6ee47035..48541ce54 100644
--- a/tests/model/range.js
+++ b/tests/model/range.js
@@ -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', () => {
From 712efbe6882bbb7f72695006251b4be1246d1688 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= <jodator@jodator.net>
Date: Tue, 31 Mar 2020 10:46:40 +0200
Subject: [PATCH 3/7] Fix test names for fixing selection after move operation
to graveyard.
---
tests/model/documentselection.js | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js
index 84cba4fbc..ca91c1e1c 100644
--- a/tests/model/documentselection.js
+++ b/tests/model/documentselection.js
@@ -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(
@@ -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(
@@ -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(
@@ -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 ] ),
From b0b3b5a4b59b93f42e90a0517ba94b31b927090d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= <jodator@jodator.net>
Date: Tue, 31 Mar 2020 10:47:35 +0200
Subject: [PATCH 4/7] Add test for fixing selection after move operation to
graveyard for multi-range selection in text node.
---
tests/model/documentselection.js | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js
index ca91c1e1c..8e4b72814 100644
--- a/tests/model/documentselection.js
+++ b/tests/model/documentselection.js
@@ -1778,6 +1778,28 @@ 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 moved (inside text nodes - resulting ranges are collapsed)', () => {
+ 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( 2 );
+ expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 1 ] );
+ expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 1 ] );
+ } );
} );
it( '`DocumentSelection#change:range` event should be fire once even if selection contains multi-ranges', () => {
From 1d9d8cee8685e574eabc119c56459977c2eb5fc8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= <jodator@jodator.net>
Date: Tue, 31 Mar 2020 11:45:56 +0200
Subject: [PATCH 5/7] Add test for intersecting ranges after multi-range
selection moved to graveyard.
---
tests/model/documentselection.js | 37 +++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js
index 8e4b72814..6b1da29eb 100644
--- a/tests/model/documentselection.js
+++ b/tests/model/documentselection.js
@@ -1779,7 +1779,7 @@ describe( 'DocumentSelection', () => {
expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] );
} );
- it( 'does not break if multi-range selection is moved (inside text nodes - resulting ranges are collapsed)', () => {
+ it( 'does not break if multi-range selection is inside text nodes', () => {
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 ] ) )
@@ -1800,6 +1800,41 @@ describe( 'DocumentSelection', () => {
expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 1 ] );
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', () => {
+ 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', () => {
From ac8078d13000171cca8e90eedfdbdc9b5da71f97 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= <jodator@jodator.net>
Date: Tue, 31 Mar 2020 12:07:33 +0200
Subject: [PATCH 6/7] Refactor internal helpers in
DocumentSelection#_fixGraveyardSelection.
---
src/model/documentselection.js | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/model/documentselection.js b/src/model/documentselection.js
index 947d1fd69..281d5afe0 100644
--- a/src/model/documentselection.js
+++ b/src/model/documentselection.js
@@ -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 && !isIntersecting( this._ranges, selectionRange ) ) {
+ // If range is intersecting with other selection ranges then it is probably due to contents
+ // of a multi-range selection being removed. See ckeditor/ckeditor5#6501.
+ if ( selectionRange && !isRangeIntersectingWithSelection( 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.
}
}
@@ -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 ) {
@@ -1191,14 +1192,7 @@ function clearAttributesStoredInElement( model, batch ) {
}
}
-// Checks if range intersects with any given ranges.
-function isIntersecting( ranges, selectionRange ) {
- for ( let i = 0; i < ranges.length; i++ ) {
- if ( selectionRange.isIntersecting( ranges[ i ] ) ) {
- // It is also fine - as we can have multiple ranges in the selection.
- return true;
- }
- }
-
- return false;
+// Checks if range intersects with any of selection ranges.
+function isRangeIntersectingWithSelection( range, selection ) {
+ return !selection._ranges.every( selectionRange => !range.isIntersecting( selectionRange ) );
}
From e4fea1664aa3644634a528f1035cee7eafb18cf7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= <jodator@jodator.net>
Date: Tue, 14 Apr 2020 11:18:41 +0200
Subject: [PATCH 7/7] Fixed conditions on checking ranges when fixing graveyard
selection.
---
src/model/documentselection.js | 10 +++++-----
tests/model/documentselection.js | 6 +++---
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/model/documentselection.js b/src/model/documentselection.js
index 281d5afe0..214bfd48b 100644
--- a/src/model/documentselection.js
+++ b/src/model/documentselection.js
@@ -1137,9 +1137,9 @@ class LiveSelection extends Selection {
liveRange.detach();
// If nearest valid selection range has been found - add it in the place of old range.
- // If range is intersecting with other selection ranges then it is probably due to contents
+ // 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 && !isRangeIntersectingWithSelection( selectionRange, this ) ) {
+ if ( selectionRange && !isRangeCollidingWithSelection( selectionRange, this ) ) {
// Check the range, convert it to live range, bind events, etc.
const newRange = this._prepareRange( selectionRange );
@@ -1192,7 +1192,7 @@ function clearAttributesStoredInElement( model, batch ) {
}
}
-// Checks if range intersects with any of selection ranges.
-function isRangeIntersectingWithSelection( range, selection ) {
- return !selection._ranges.every( selectionRange => !range.isIntersecting( selectionRange ) );
+// Checks if range collides with any of selection ranges.
+function isRangeCollidingWithSelection( range, selection ) {
+ return !selection._ranges.every( selectionRange => !range.isEqual( selectionRange ) );
}
diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js
index 6b1da29eb..17c9dd77e 100644
--- a/tests/model/documentselection.js
+++ b/tests/model/documentselection.js
@@ -1779,7 +1779,7 @@ describe( 'DocumentSelection', () => {
expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] );
} );
- 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 (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 ] ) )
@@ -1796,12 +1796,12 @@ describe( 'DocumentSelection', () => {
)
);
- expect( selection.rangeCount ).to.equal( 2 );
+ 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( 'does not break if multi-range selection is set on object nodes and resulting ranges will intersect', () => {
+ 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
} );