Selaa lähdekoodia

Fixed: deleteContents should not create RemoveDelta if there is nothing to
be removed. Changed: Range transformation bases on isSticky flag when it is collapsed.

Szymon Cofalik 9 vuotta sitten
vanhempi
commit
3c2d56631d

+ 8 - 4
packages/ckeditor5-engine/src/model/composer/deletecontents.js

@@ -25,11 +25,15 @@ export default function deleteContents( batch, selection, options = {} ) {
 		return;
 	}
 
-	const startPos = selection.getFirstRange().start;
-	const endPos = LivePosition.createFromPosition( selection.getFirstRange().end );
+	const selRange = selection.getFirstRange();
 
-	// 1. Remove the contents.
-	batch.remove( selection.getFirstRange() );
+	const startPos = selRange.start;
+	const endPos = LivePosition.createFromPosition( selRange.end );
+
+	// 1. Remove the contents if there are any.
+	if ( !selRange.isEmpty ) {
+		batch.remove( selRange );
+	}
 
 	// 2. Merge elements in the right branch to the elements in the left branch.
 	// The only reasonable (in terms of data and selection correctness) case in which we need to do that is:

+ 2 - 2
packages/ckeditor5-engine/src/model/liverange.js

@@ -114,7 +114,7 @@ function fixBoundaries( type, range, position ) {
 
 	switch ( type ) {
 		case 'insert':
-			updated = this.getTransformedByInsertion( range.start, howMany )[ 0 ];
+			updated = this.getTransformedByInsertion( range.start, howMany, false, true )[ 0 ];
 			break;
 
 		case 'move':
@@ -127,7 +127,7 @@ function fixBoundaries( type, range, position ) {
 			// We have to revert `range.start` to the state before the move.
 			const targetPosition = range.start.getTransformedByInsertion( sourcePosition, howMany );
 
-			const result = this.getTransformedByMove( sourcePosition, targetPosition, howMany );
+			const result = this.getTransformedByMove( sourcePosition, targetPosition, howMany, false, true );
 
 			// First item in the array is the "difference" part, so a part of the range
 			// that did not get moved. We use it as reference range and expand if possible.

+ 37 - 30
packages/ckeditor5-engine/src/model/range.js

@@ -41,6 +41,29 @@ export default class Range {
 	}
 
 	/**
+	 * Returns an iterator that iterates over all {@link engine.model.Item items} that are in this range and returns
+	 * them together with additional information like length or {@link engine.model.Position positions},
+	 * grouped as {@link engine.model.TreeWalkerValue}. It iterates over all {@link engine.model.TextProxy texts}
+	 * that are inside the range and all the {@link engine.model.Element}s we enter into when iterating over this
+	 * range.
+	 *
+	 * **Note:** iterator will not return a parent node of start position. This is in contrary to
+	 * {@link engine.model.TreeWalker} which will return that node with `'ELEMENT_END'` type. Iterator also
+	 * returns each {@link engine.model.Element} once, while simply used {@link engine.model.TreeWalker} might
+	 * return it twice: for `'ELEMENT_START'` and `'ELEMENT_END'`.
+	 *
+	 * **Note:** because iterator does not return {@link engine.model.TreeWalkerValue values} with the type of
+	 * `'ELEMENT_END'`, you can use {@link engine.model.TreeWalkerValue.previousPosition} as a position before the
+	 * item.
+	 *
+	 * @see engine.model.TreeWalker
+	 * @returns {Iterable.<engine.model.TreeWalkerValue>}
+	 */
+	*[ Symbol.iterator ]() {
+		yield* new TreeWalker( { boundaries: this, ignoreElementEnd: true } );
+	}
+
+	/**
 	 * Returns whether the range is collapsed, that is it start and end positions are equal.
 	 *
 	 * @type {Boolean}
@@ -59,6 +82,15 @@ export default class Range {
 	}
 
 	/**
+	 * Returns whether this range has any nodes in it.
+	 *
+	 * @type {Boolean}
+	 */
+	get isEmpty() {
+		return this.start.isTouching( this.end );
+	}
+
+	/**
 	 * Range root element.
 	 *
 	 * Equals to the root of start position (which should be same as root of end position).
@@ -254,29 +286,6 @@ export default class Range {
 	}
 
 	/**
-	 * Returns an iterator that iterates over all {@link engine.model.Item items} that are in this range and returns
-	 * them together with additional information like length or {@link engine.model.Position positions},
-	 * grouped as {@link engine.model.TreeWalkerValue}. It iterates over all {@link engine.model.TextProxy texts}
-	 * that are inside the range and all the {@link engine.model.Element}s we enter into when iterating over this
-	 * range.
-	 *
-	 * **Note:** iterator will not return a parent node of start position. This is in contrary to
-	 * {@link engine.model.TreeWalker} which will return that node with `'ELEMENT_END'` type. Iterator also
-	 * returns each {@link engine.model.Element} once, while simply used {@link engine.model.TreeWalker} might
-	 * return it twice: for `'ELEMENT_START'` and `'ELEMENT_END'`.
-	 *
-	 * **Note:** because iterator does not return {@link engine.model.TreeWalkerValue values} with the type of
-	 * `'ELEMENT_END'`, you can use {@link engine.model.TreeWalkerValue.previousPosition} as a position before the
-	 * item.
-	 *
-	 * @see engine.model.TreeWalker
-	 * @returns {Iterable.<engine.model.TreeWalkerValue>}
-	 */
-	*[ Symbol.iterator ]() {
-		yield* new TreeWalker( { boundaries: this, ignoreElementEnd: true } );
-	}
-
-	/**
 	 * Creates a {@link engine.model.TreeWalker} instance with this range as a boundary.
 	 *
 	 * @param {Object} options Object with configuration options. See {@link engine.model.TreeWalker}.
@@ -371,9 +380,7 @@ export default class Range {
 	 * range boundary. Defaults to `false`.
 	 * @returns {Array.<engine.model.Range>} Result of the transformation.
 	 */
-	getTransformedByInsertion( insertPosition, howMany, spread, isSticky ) {
-		isSticky = !!isSticky;
-
+	getTransformedByInsertion( insertPosition, howMany, spread = false, isSticky = false ) {
 		if ( spread && this.containsPosition( insertPosition ) ) {
 			// Range has to be spread. The first part is from original start to the spread point.
 			// The other part is from spread point to the original end, but transformed by
@@ -389,8 +396,8 @@ export default class Range {
 		} else {
 			const range = Range.createFromRange( this );
 
-			let insertBeforeStart = range.isCollapsed ? true : !isSticky;
-			let insertBeforeEnd = range.isCollapsed ? true : isSticky;
+			let insertBeforeStart = range.isCollapsed ? isSticky : !isSticky;
+			let insertBeforeEnd = isSticky;
 
 			range.start = range.start.getTransformedByInsertion( insertPosition, howMany, insertBeforeStart );
 			range.end = range.end.getTransformedByInsertion( insertPosition, howMany, insertBeforeEnd );
@@ -410,7 +417,7 @@ export default class Range {
 	 * was inside the range. Defaults to `false`.
 	 * @returns {Array.<engine.model.Range>} Result of the transformation.
 	 */
-	getTransformedByMove( sourcePosition, targetPosition, howMany, spread ) {
+	getTransformedByMove( sourcePosition, targetPosition, howMany, spread, isSticky = false ) {
 		let result;
 
 		const moveRange = new Range( sourcePosition, sourcePosition.getShiftedBy( howMany ) );
@@ -438,7 +445,7 @@ export default class Range {
 		const insertPosition = targetPosition.getTransformedByDeletion( sourcePosition, howMany );
 
 		if ( difference ) {
-			result = difference.getTransformedByInsertion( insertPosition, howMany, spread );
+			result = difference.getTransformedByInsertion( insertPosition, howMany, spread, isSticky );
 		} else {
 			result = [];
 		}

+ 26 - 2
packages/ckeditor5-engine/tests/model/range.js

@@ -441,9 +441,17 @@ describe( 'Range', () => {
 			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 4, 4 ] );
 		} );
 
-		it( 'should move after inserted nodes if the range is collapsed', () => {
+		it( 'should not change if the range is collapsed and isSticky is false', () => {
 			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 3, 2 ] ) );
-			const transformed = range.getTransformedByInsertion( new Position( root, [ 3, 2 ] ), 3 );
+			const transformed = range.getTransformedByInsertion( new Position( root, [ 3, 2 ] ), 3, false, false );
+
+			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 2 ] );
+			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 3, 2 ] );
+		} );
+
+		it( 'should move after inserted nodes if the range is collapsed and isSticky is true', () => {
+			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 3, 2 ] ) );
+			const transformed = range.getTransformedByInsertion( new Position( root, [ 3, 2 ] ), 3, false, true );
 
 			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 5 ] );
 			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 3, 5 ] );
@@ -710,6 +718,22 @@ describe( 'Range', () => {
 		} );
 	} );
 
+	describe( 'isEmpty', () => {
+		beforeEach( () => {
+			prepareRichRoot( root );
+		} );
+
+		it( 'should be true if there are no nodes between range start and end', () => {
+			let range = new Range( new Position( root, [ 0, 0, 5 ] ), new Position( root, [ 0, 1, 0 ] ) );
+			expect( range.isEmpty ).to.be.true;
+		} );
+
+		it( 'should be false if there are nodes between range start and end', () => {
+			let range = new Range( new Position( root, [ 0, 0, 5 ] ), new Position( root, [ 0, 1, 1 ] ) );
+			expect( range.isEmpty ).to.be.false;
+		} );
+	} );
+
 	function mapNodesToNames( nodes ) {
 		return nodes.map( ( node ) => {
 			return ( node instanceof Element ) ? 'E:' + node.name : 'T:' + ( node.text || node.character );