Преглед изворни кода

Merge branch 'master' into t/841

Piotrek Koszuliński пре 8 година
родитељ
комит
18b386ecdd

+ 2 - 3
packages/ckeditor5-engine/src/conversion/model-to-view-converters.js

@@ -424,7 +424,7 @@ export function remove() {
 		const modelRange = ModelRange.createFromPositionAndShift( data.sourcePosition, data.item.offsetSize );
 		const viewRange = conversionApi.mapper.toViewRange( modelRange );
 
-		viewWriter.remove( viewRange );
+		viewWriter.remove( viewRange.getTrimmed() );
 		conversionApi.mapper.unbindModelElement( data.item );
 	};
 }
@@ -452,9 +452,8 @@ export function removeUIElement( elementCreator ) {
 		}
 
 		const viewRange = conversionApi.mapper.toViewRange( data.range );
-		const enlargedViewRange = viewRange.getEnlarged();
 
-		viewWriter.clear( enlargedViewRange, viewElement );
+		viewWriter.clear( viewRange.getEnlarged(), viewElement );
 	};
 }
 

+ 9 - 3
packages/ckeditor5-engine/src/model/range.js

@@ -453,8 +453,14 @@ export default class Range {
 			const ranges = this._getTransformedByMove( sourcePosition, targetPosition, howMany );
 
 			if ( deltaType == 'split' && this.containsPosition( sourcePosition ) ) {
+				// Special case for splitting element inside range.
+				// <p>f[ooba]r</p> -> <p>f[oo</p><p>ba]r</p>
 				ranges[ 0 ].end = ranges[ 1 ].end;
 				ranges.pop();
+			} else if ( deltaType == 'merge' && type == 'move' && this.isCollapsed && ranges[ 0 ].start.isEqual( sourcePosition ) ) {
+				// Special case when collapsed range is in merged element.
+				// <p>foo</p><p>[]bar{}</p> -> <p>foo[]bar{}</p>
+				ranges[ 0 ] = new Range( targetPosition.getShiftedBy( this.start.offset ) );
 			}
 			// Don't ask. Just debug.
 			// Like this: https://github.com/ckeditor/ckeditor5-engine/issues/841#issuecomment-282706488.
@@ -555,7 +561,7 @@ export default class Range {
 	 */
 	_getTransformedByMove( sourcePosition, targetPosition, howMany ) {
 		if ( this.isCollapsed ) {
-			const newPos = this.start._getTransformedByMove( sourcePosition, targetPosition, howMany, true, true );
+			const newPos = this.start._getTransformedByMove( sourcePosition, targetPosition, howMany, true, false );
 
 			return [ new Range( newPos ) ];
 		}
@@ -570,7 +576,7 @@ export default class Range {
 		const common = this.getIntersection( moveRange );
 
 		if ( differenceSet.length == 1 ) {
-			// `moveRange` and this range intersects.
+			// `moveRange` and this range may intersect.
 			difference = new Range(
 				differenceSet[ 0 ].start._getTransformedByDeletion( sourcePosition, howMany ),
 				differenceSet[ 0 ].end._getTransformedByDeletion( sourcePosition, howMany )
@@ -581,7 +587,7 @@ export default class Range {
 				this.start,
 				this.end._getTransformedByDeletion( sourcePosition, howMany )
 			);
-		} // else, `moveRange` wholly contains this range.
+		} // else, `moveRange` contains this range.
 
 		const insertPosition = targetPosition._getTransformedByDeletion( sourcePosition, howMany );
 

+ 8 - 3
packages/ckeditor5-engine/src/view/matcher.js

@@ -203,9 +203,14 @@ export default class Matcher {
 	 * @returns {String|null} Element name trying to match.
 	 */
 	getElementName() {
-		return this._patterns.length == 1 && this._patterns[ 0 ].name && !( this._patterns[ 0 ].name instanceof RegExp ) ?
-			this._patterns[ 0 ].name :
-			null;
+		if ( this._patterns.length !== 1 ) {
+			return null;
+		}
+
+		const pattern = this._patterns[ 0 ];
+		const name = pattern.name;
+
+		return ( typeof pattern != 'function' && name && !( name instanceof RegExp ) ) ? name : null;
 	}
 
 }

+ 4 - 4
packages/ckeditor5-engine/src/view/observer/mutationobserver.js

@@ -247,10 +247,10 @@ export default class MutationObserver extends Observer {
  *
  * @see module:engine/view/observer/mutationobserver~MutationObserver
  * @event module:engine/view/document~Document#event:mutations
- * @param {Array.<module:engine/view/observer/mutationobserver~MutatedText|module:engine/view/observer/mutationobserver~MutatatedChildren>}
+ * @param {Array.<module:engine/view/observer/mutationobserver~MutatedText|module:engine/view/observer/mutationobserver~MutatedChildren>}
  * viewMutations Array of mutations.
  * For mutated texts it will be {@link module:engine/view/observer/mutationobserver~MutatedText} and for mutated elements it will be
- * {@link module:engine/view/observer/mutationobserver~MutatatedChildren}. You can recognize the type based on the `type` property.
+ * {@link module:engine/view/observer/mutationobserver~MutatedChildren}. You can recognize the type based on the `type` property.
  * @param {module:engine/view/selection~Selection|null} viewSelection View selection that is a result of converting DOM selection to view.
  * Keep in
  * mind that the DOM selection is already "updated", meaning that it already acknowledges changes done in mutation.
@@ -260,7 +260,7 @@ export default class MutationObserver extends Observer {
  * Mutation item for text.
  *
  * @see module:engine/view/document~Document#event:mutations
- * @see module:engine/view/observer/mutationobserver~MutatatedChildren
+ * @see module:engine/view/observer/mutationobserver~MutatedChildren
  *
  * @typedef {Object} module:engine/view/observer/mutationobserver~MutatedText
  *
@@ -276,7 +276,7 @@ export default class MutationObserver extends Observer {
  * @see module:engine/view/document~Document#event:mutations
  * @see module:engine/view/observer/mutationobserver~MutatedText
  *
- * @typedef {Object} module:engine/view/observer/mutationobserver~MutatatedChildren
+ * @typedef {Object} module:engine/view/observer/mutationobserver~MutatedChildren
  *
  * @property {String} type For child nodes mutations it is always 'children'.
  * @property {module:engine/view/element~Element} node Parent of the mutated children.

+ 11 - 2
packages/ckeditor5-engine/src/view/range.js

@@ -98,8 +98,17 @@ export default class Range {
 	 * @returns {module:engine/view/range~Range} Enlarged range.
 	 */
 	getEnlarged() {
-		const start = this.start.getLastMatchingPosition( enlargeShrinkSkip, { direction: 'backward' } );
-		const end = this.end.getLastMatchingPosition( enlargeShrinkSkip );
+		let start = this.start.getLastMatchingPosition( enlargeShrinkSkip, { direction: 'backward' } );
+		let end = this.end.getLastMatchingPosition( enlargeShrinkSkip );
+
+		// Fix positions, in case if they are in Text node.
+		if ( start.parent.is( 'text' ) && start.isAtStart ) {
+			start = Position.createBefore( start.parent );
+		}
+
+		if ( end.parent.is( 'text' ) && end.isAtEnd ) {
+			end = Position.createAfter( end.parent );
+		}
 
 		return new Range( start, end );
 	}

+ 14 - 20
packages/ckeditor5-engine/src/view/treewalker.js

@@ -129,8 +129,6 @@ export default class TreeWalker {
 		 * @member {module:engine/view/node~Node} module:engine/view/treewalker~TreeWalker#_boundaryEndParent
 		 */
 		this._boundaryEndParent = this.boundaries ? this.boundaries.end.parent : null;
-
-		this._fixStartPositionInText();
 	}
 
 	/**
@@ -208,6 +206,13 @@ export default class TreeWalker {
 
 		// Text is a specific parent because it contains string instead of child nodes.
 		if ( parent instanceof Text ) {
+			if ( position.isAtEnd ) {
+				// Prevent returning "elementEnd" for Text node. Skip that value and return the next walker step.
+				this.position = Position.createAfter( parent );
+
+				return this._next();
+			}
+
 			node = parent.data[ position.offset ];
 		} else {
 			node = parent.getChild( position.offset );
@@ -306,6 +311,13 @@ export default class TreeWalker {
 
 		// Text {@link module:engine/view/text~Text} element is a specific parent because contains string instead of child nodes.
 		if ( parent instanceof Text ) {
+			if ( position.isAtStart ) {
+				// Prevent returning "elementStart" for Text node. Skip that value and return the next walker step.
+				this.position = Position.createBefore( parent );
+
+				return this._previous();
+			}
+
 			node = parent.data[ position.offset - 1 ];
 		} else {
 			node = parent.getChild( position.offset - 1 );
@@ -432,24 +444,6 @@ export default class TreeWalker {
 			}
 		};
 	}
-
-	/**
-	 * Fixes tree walker start position if it is at the beginning or at the end of a {@link module:engine/view/text~Text text node}.
-	 *
-	 * Without the fix, the first returned value by tree walker would have type `elementEnd` or `elementStart` but the
-	 * item would be a {@link module:engine/view/text~Text text node}.
-	 *
-	 * @private
-	 */
-	_fixStartPositionInText() {
-		const parent = this.position.parent;
-
-		if ( parent instanceof Text && this.position.isAtStart ) {
-			this.position = Position.createBefore( parent );
-		} else if ( parent instanceof Text && this.position.isAtEnd ) {
-			this.position = Position.createAfter( parent );
-		}
-	}
 }
 
 /**

+ 24 - 0
packages/ckeditor5-engine/tests/conversion/model-to-view-converters.js

@@ -848,5 +848,29 @@ describe( 'model-to-view-converters', () => {
 
 			expect( viewToString( viewRoot ) ).to.equal( '<div><div>கு</div></div>' );
 		} );
+
+		it( 'should not remove view ui elements that are placed next to removed content', () => {
+			modelRoot.appendChildren( new ModelText( 'foobar' ) );
+			viewRoot.appendChildren( [
+				new ViewText( 'foo' ),
+				new ViewUIElement( 'span' ),
+				new ViewText( 'bar' )
+			] );
+
+			dispatcher.on( 'remove', remove() );
+
+			// Remove 'b'.
+			modelWriter.move(
+				ModelRange.createFromParentsAndOffsets( modelRoot, 3, modelRoot, 4 ),
+				ModelPosition.createAt( modelDoc.graveyard, 'end' )
+			);
+
+			dispatcher.convertRemove(
+				ModelPosition.createFromParentAndOffset( modelRoot, 3 ),
+				ModelRange.createFromParentsAndOffsets( modelDoc.graveyard, 0, modelDoc.graveyard, 1 )
+			);
+
+			expect( viewToString( viewRoot ) ).to.equal( '<div>foo<span></span>ar</div>' );
+		} );
 	} );
 } );

+ 28 - 7
packages/ckeditor5-engine/tests/model/range.js

@@ -18,7 +18,8 @@ import {
 	getMoveDelta,
 	getRemoveDelta,
 	getRenameDelta,
-	getSplitDelta
+	getSplitDelta,
+	getMergeDelta
 } from '../../tests/model/delta/transform/_utils/utils';
 
 describe( 'Range', () => {
@@ -593,20 +594,22 @@ describe( 'Range', () => {
 			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 4, 7 ] );
 		} );
 
-		it( 'should stick to moved range, if the transformed range is collapsed #1', () => {
+		it( 'should not stick to moved range, if the transformed range is collapsed #1', () => {
 			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 3, 2 ] ) );
 			const transformed = range._getTransformedByMove( new Position( root, [ 3, 0 ] ), new Position( root, [ 6 ] ), 2 );
 
-			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 8 ] );
-			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 8 ] );
+			expect( transformed.length ).to.equal( 1 );
+			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 0 ] );
+			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 3, 0 ] );
 		} );
 
-		it( 'should stick to moved range, if the transformed range is collapsed #2', () => {
+		it( 'should not stick to moved range, if the transformed range is collapsed #2', () => {
 			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 3, 2 ] ) );
 			const transformed = range._getTransformedByMove( new Position( root, [ 3, 2 ] ), new Position( root, [ 6 ] ), 2 );
 
-			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 6 ] );
-			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 6 ] );
+			expect( transformed.length ).to.equal( 1 );
+			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 2 ] );
+			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 3, 2 ] );
 		} );
 	} );
 
@@ -836,6 +839,24 @@ describe( 'Range', () => {
 				expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 2 ] );
 				expect( transformed[ 0 ].end.path ).to.deep.equal( [ 1, 1 ] );
 			} );
+
+			describe( 'by MergeDelta', () => {
+				it( 'merge element with collapsed range', () => {
+					root.removeChildren( root.childCount );
+					root.appendChildren( [ new Element( 'p', null, new Text( 'foo' ) ), new Element( 'p', null, new Text( 'bar' ) ) ] );
+
+					range.start = new Position( root, [ 1, 0 ] );
+					range.end = new Position( root, [ 1, 0 ] );
+
+					const delta = getMergeDelta( new Position( root, [ 1 ] ), 3, 3, 1 );
+
+					const transformed = range.getTransformedByDelta( delta );
+
+					expect( transformed.length ).to.equal( 1 );
+					expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 3 ] );
+					expect( transformed[ 0 ].end.path ).to.deep.equal( [ 0, 3 ] );
+				} );
+			} );
 		} );
 	} );
 

+ 12 - 0
packages/ckeditor5-engine/tests/view/matcher.js

@@ -401,5 +401,17 @@ describe( 'Matcher', () => {
 
 			expect( matcher.getElementName() ).to.be.null;
 		} );
+
+		it( 'should return null for matching function', () => {
+			const matcher = new Matcher( () => {} );
+
+			expect( matcher.getElementName() ).to.be.null;
+		} );
+
+		it( 'should return null for matching named function', () => {
+			const matcher = new Matcher( function matchFunction() {} );
+
+			expect( matcher.getElementName() ).to.be.null;
+		} );
 	} );
 } );

+ 13 - 1
packages/ckeditor5-engine/tests/view/treewalker.js

@@ -976,7 +976,7 @@ describe( 'TreeWalker', () => {
 		} );
 	} );
 
-	it( 'should not return elementEnd for a text node when iteration begins at the start or the end of that text node', () => {
+	it( 'should not return elementEnd for a text node when iteration begins at the end of that text node', () => {
 		let iterator = new TreeWalker( {
 			startPosition: Position.createAt( textAbcd, 'end' )
 		} );
@@ -987,6 +987,18 @@ describe( 'TreeWalker', () => {
 		expect( step.value.item ).to.equal( bold );
 	} );
 
+	it( 'should not return elementStart for a text node when iteration begins at the start of that text node', () => {
+		let iterator = new TreeWalker( {
+			startPosition: Position.createAt( textAbcd, 0 ),
+			direction: 'backward'
+		} );
+
+		const step = iterator.next();
+
+		expect( step.value.type ).to.equal( 'elementStart' );
+		expect( step.value.item ).to.equal( bold );
+	} );
+
 	it( 'should iterate over document fragment', () => {
 		const foo = new Text( 'foo' );
 		const bar = new Text( 'bar' );