Procházet zdrojové kódy

Fixed situation when selection is set to the same selection instance.

Maciej Bukowski před 7 roky
rodič
revize
0afed1122f

+ 5 - 0
packages/ckeditor5-engine/src/view/selection.js

@@ -490,6 +490,7 @@ export default class Selection {
 			this._setFakeOptions( options );
 		} else if ( isIterable( selectable ) ) {
 			// We assume that the selectable is an iterable of ranges.
+			// Array.from() is used to prevent setting ranges to the old iterable
 			this._setRanges( selectable, optionsOrPlaceOrOffset && optionsOrPlaceOrOffset.backward );
 			this._setFakeOptions( optionsOrPlaceOrOffset );
 		} else {
@@ -515,6 +516,10 @@ export default class Selection {
 	 * (`false`) or backward - from end to start (`true`). Defaults to `false`.
 	 */
 	_setRanges( newRanges, isLastBackward = false ) {
+		// New ranges should be copied to prevent removing them by setting them to `[]` first.
+		// Only applies to situations when selection is set to the same selection or same selection's ranges.
+		newRanges = Array.from( newRanges );
+
 		this._ranges = [];
 
 		for ( const range of newRanges ) {

+ 1 - 1
packages/ckeditor5-engine/tests/view/observer/fakeselectionobserver.js

@@ -41,7 +41,7 @@ describe( 'FakeSelectionObserver', () => {
 	} );
 
 	it( 'should do nothing if selection is not fake', () => {
-		viewDocument.selection._setTo( null, { fake: true } );
+		viewDocument.selection._setTo( null, { fake: false } );
 
 		return checkEventPrevention( keyCodes.arrowleft, false );
 	} );

+ 13 - 14
packages/ckeditor5-engine/tests/view/renderer.js

@@ -122,7 +122,6 @@ describe( 'Renderer', () => {
 			renderer.markedChildren.clear();
 
 			selection._setTo( null );
-			selection._setFake( false );
 
 			selectionEditable = viewRoot;
 
@@ -1369,7 +1368,7 @@ describe( 'Renderer', () => {
 
 			it( 'should render fake selection', () => {
 				const label = 'fake selection label';
-				selection._setFake( true, { label } );
+				selection._setTo( selection.getRanges(), { fake: true, label } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );
@@ -1386,7 +1385,7 @@ describe( 'Renderer', () => {
 			} );
 
 			it( 'should render   if no selection label is provided', () => {
-				selection._setFake( true );
+				selection._setTo( selection.getRanges(), { fake: true } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );
@@ -1402,10 +1401,10 @@ describe( 'Renderer', () => {
 			} );
 
 			it( 'should remove fake selection container when selection is no longer fake', () => {
-				selection._setFake( true );
+				selection._setTo( selection.getRanges(), { fake: true } );
 				renderer.render();
 
-				selection._setFake( false );
+				selection._setTo( selection.getRanges(), { fake: false } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 1 );
@@ -1421,14 +1420,14 @@ describe( 'Renderer', () => {
 			it( 'should reuse fake selection container #1', () => {
 				const label = 'fake selection label';
 
-				selection._setFake( true, { label } );
+				selection._setTo( selection.getRanges(), { fake: true, label } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );
 
 				const container = domRoot.childNodes[ 1 ];
 
-				selection._setFake( true, { label } );
+				selection._setTo( selection.getRanges(), { fake: true, label } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );
@@ -1442,19 +1441,19 @@ describe( 'Renderer', () => {
 			} );
 
 			it( 'should reuse fake selection container #2', () => {
-				selection._setFake( true, { label: 'label 1' } );
+				selection._setTo( selection.getRanges(), { fake: true, label: 'label 1' } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );
 
 				const container = domRoot.childNodes[ 1 ];
 
-				selection._setFake( false );
+				selection._setTo( selection.getRanges(), { fake: false } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 1 );
 
-				selection._setFake( true, { label: 'label 2' } );
+				selection._setTo( selection.getRanges(), { fake: true, label: 'label 2' } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );
@@ -1468,14 +1467,14 @@ describe( 'Renderer', () => {
 			} );
 
 			it( 'should reuse fake selection container #3', () => {
-				selection._setFake( true, { label: 'label 1' } );
+				selection._setTo( selection.getRanges(), { fake: true, label: 'label 1' } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );
 
 				const container = domRoot.childNodes[ 1 ];
 
-				selection._setFake( true, { label: 'label 2' } );
+				selection._setTo( selection.getRanges(), { fake: true, label: 'label 2' } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );
@@ -1489,7 +1488,7 @@ describe( 'Renderer', () => {
 			} );
 
 			it( 'should style fake selection container properly', () => {
-				selection._setFake( true, { label: 'fake selection' } );
+				selection._setTo( selection.getRanges(), { fake: true, label: 'fake selection' } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );
@@ -1502,7 +1501,7 @@ describe( 'Renderer', () => {
 			} );
 
 			it( 'should bind fake selection container to view selection', () => {
-				selection._setFake( true, { label: 'fake selection' } );
+				selection._setTo( selection.getRanges(), { fake: true, label: 'fake selection' } );
 				renderer.render();
 
 				expect( domRoot.childNodes.length ).to.equal( 2 );

+ 24 - 26
packages/ckeditor5-engine/tests/view/selection.js

@@ -764,15 +764,6 @@ describe( 'Selection', () => {
 
 				selection._setTo( selection.getFirstPosition() );
 			} );
-
-			it( 'should do nothing if no ranges present', () => {
-				const fireSpy = sinon.spy( selection, 'fire' );
-
-				selection._setTo( selection.getFirstPosition() );
-
-				fireSpy.restore();
-				expect( fireSpy.notCalled ).to.be.true;
-			} );
 		} );
 
 		describe( 'setting collapsed selection to end', () => {
@@ -787,15 +778,6 @@ describe( 'Selection', () => {
 
 				selection._setTo( selection.getLastPosition() );
 			} );
-
-			it( 'should do nothing if no ranges present', () => {
-				const fireSpy = sinon.spy( selection, 'fire' );
-
-				selection._setTo( selection.getLastPosition() );
-
-				fireSpy.restore();
-				expect( fireSpy.notCalled ).to.be.true;
-			} );
 		} );
 
 		describe( 'removing all ranges', () => {
@@ -809,14 +791,6 @@ describe( 'Selection', () => {
 
 				selection._setTo( null );
 			} );
-
-			it( 'should do nothing when no ranges are present', () => {
-				const fireSpy = sinon.spy( selection, 'fire' );
-				selection._setTo( null );
-
-				fireSpy.restore();
-				expect( fireSpy.notCalled ).to.be.true;
-			} );
 		} );
 
 		describe( 'setting fake selection', () => {
@@ -867,6 +841,30 @@ describe( 'Selection', () => {
 			} );
 		} );
 
+		describe( 'setting selection to itself', () => {
+			it( 'should correctly set ranges when setting to the same selection', () => {
+				selection._setTo( [ range1, range2 ] );
+				selection._setTo( selection );
+
+				const ranges = Array.from( selection.getRanges() );
+				expect( ranges.length ).to.equal( 2 );
+
+				expect( ranges[ 0 ].isEqual( range1 ) ).to.be.true;
+				expect( ranges[ 1 ].isEqual( range2 ) ).to.be.true;
+			} );
+
+			it( 'should correctly set ranges when setting to the same selection\'s ranges', () => {
+				selection._setTo( [ range1, range2 ] );
+				selection._setTo( selection.getRanges() );
+
+				const ranges = Array.from( selection.getRanges() );
+				expect( ranges.length ).to.equal( 2 );
+
+				expect( ranges[ 0 ].isEqual( range1 ) ).to.be.true;
+				expect( ranges[ 1 ].isEqual( range2 ) ).to.be.true;
+			} );
+		} );
+
 		describe( 'throwing errors', () => {
 			it( 'should throw an error when range is invalid', () => {
 				expect( () => {