Kaynağa Gözat

Code style: Change the selection post-fixer internals for better readability.

Maciej Gołaszewski 7 yıl önce
ebeveyn
işleme
959eec408d

+ 22 - 12
packages/ckeditor5-engine/src/model/schema.js

@@ -586,22 +586,32 @@ export default class Schema {
 	 * Returns the lowest {@link module:engine/model/schema~Schema#isLimit limit element} containing the entire
 	 * selection or the root otherwise.
 	 *
-	 * @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection
+	 * @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selectionOrRangeOrPosition
 	 * Selection which returns the common ancestor.
 	 * @returns {module:engine/model/element~Element}
 	 */
-	getLimitElement( selection ) {
-		// Find the common ancestor for all selection's ranges.
-		let element = Array.from( selection.getRanges() )
-			.reduce( ( element, range ) => {
-				const rangeCommonAncestor = range.getCommonAncestor();
-
-				if ( !element ) {
-					return rangeCommonAncestor;
-				}
+	getLimitElement( selectionOrRangeOrPosition ) {
+		let element;
+
+		if ( selectionOrRangeOrPosition instanceof Position ) {
+			element = selectionOrRangeOrPosition.parent;
+		} else {
+			const ranges = selectionOrRangeOrPosition instanceof Range ?
+				[ selectionOrRangeOrPosition ] :
+				Array.from( selectionOrRangeOrPosition.getRanges() );
 
-				return element.getCommonAncestor( rangeCommonAncestor, { includeSelf: true } );
-			}, null );
+			// Find the common ancestor for all selection's ranges.
+			element = ranges
+				.reduce( ( element, range ) => {
+					const rangeCommonAncestor = range.getCommonAncestor();
+
+					if ( !element ) {
+						return rangeCommonAncestor;
+					}
+
+					return element.getCommonAncestor( rangeCommonAncestor, { includeSelf: true } );
+				}, null );
+		}
 
 		while ( !this.isLimit( element ) ) {
 			if ( element.parent ) {

+ 39 - 83
packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js

@@ -155,78 +155,48 @@ function tryFixingNonCollapsedRage( range, schema ) {
 	const start = range.start;
 	const end = range.end;
 
-	// Flat range on the same text node is always valid - no need to fix:
-	// - <limit>f[o]o</limit>
-	// - <limit>f[oo]</limit>
-	// - <limit>[fo]o</limit>
-	if ( range.isFlat && ( start.textNode || end.textNode ) ) {
-		return null;
-	}
-
-	// Try to fix selection that is not on limit nodes:
-	// - [<p>foo</p>]            ->  <p>[foo]</p>
-	// - [<p>foo</p><p>bar</p>]  ->  <p>[foo</p><p>bar]</p>
-	if ( ( start.nodeAfter && !schema.isLimit( start.nodeAfter ) ) && ( end.nodeBefore && !schema.isLimit( end.nodeBefore ) ) ) {
-		const fixedStart = schema.getNearestSelectionRange( start, 'forward' );
-		const fixedEnd = schema.getNearestSelectionRange( end, 'backward' );
-
-		// This might be null ie when editor data is empty or selection is already properly set.
-		// In such cases there is no need to fix the selection range.
-		if ( fixedStart && fixedStart.start.isEqual( start ) && fixedEnd && fixedEnd.start.isEqual( end ) ) {
+	const isTextAllowedOnStart = schema.checkChild( start, '$text' );
+	const isTextAllowedOnEnd = schema.checkChild( end, '$text' );
+
+	const startLimitElement = schema.getLimitElement( start );
+	const endLimitElement = schema.getLimitElement( end );
+
+	// Ranges which both end are inside the same limit element (or root) might needs only minor fix.
+	if ( startLimitElement === endLimitElement ) {
+		// Range is valid when both position allows to place a text:
+		// - <block>f[oobarba]z</block>
+		// This would be "fixed" by a next check but as it will be the same it's better to return null so the selection stays the same.
+		if ( isTextAllowedOnStart && isTextAllowedOnEnd ) {
 			return null;
 		}
 
-		return new Range( fixedStart ? fixedStart.start : start, fixedEnd ? fixedEnd.start : end );
-	}
+		// Range that is on non-limit element (or is partially) must be fixed so it is placed inside the block around $text:
+		// - [<block>foo</block>]    ->    <block>[foo]</block>
+		// - [<block>foo]</block>    ->    <block>[foo]</block>
+		// - <block>f[oo</block>]    ->    <block>f[oo]</block>
+		if ( checkSelectionOnNonLimitElements( start, end, schema ) ) {
+			const fixedStart = schema.getNearestSelectionRange( start, 'forward' );
+			const fixedEnd = schema.getNearestSelectionRange( end, 'backward' );
 
-	// This will fix selection on limit elements:
-	// - <table>[<tableRow><tableCell></tableCell></tableRow>]<table>   ->   [<table><tableRow><tableCell></tableCell></tableRow><table>]
-	// - <image>[<caption>xxx</caption>]</image>                        ->   [<image><caption>xxx</caption></image>]
-	//
-	// And when selection crosses limit element:
-	// - <image>[<caption>xx]x</caption></image>                        ->   [<image><caption>xxx</caption></image>]
-	const updatedStart = expandSelectionOnIsLimitNode( start, schema, 'start' );
-	const updatedEnd = expandSelectionOnIsLimitNode( end, schema, 'end' );
-
-	if ( !start.isEqual( updatedStart ) || !end.isEqual( updatedEnd ) ) {
-		return new Range( updatedStart, updatedEnd );
-	}
-
-	// If the flat range was not fixed at this point the range is valid.
-	if ( range.isFlat ) {
-		return null;
+			return new Range( fixedStart ? fixedStart.start : start, fixedEnd ? fixedEnd.start : end );
+		}
 	}
 
-	// Check if selection crosses limit node boundaries.
-	// - <table>                                             [<table>
-	//       <tableRow>                                          <tableRow>
-	//           <tableCell><p>f[oo</p></tableCell>    ->            <tableCell><p>foo</p></tableCell>
-	//           <tableCell><p>b]ar</p></tableCell>                  <tableCell><p>bar</p></tableCell>
-	//       </tableRow>                                         </tableRow>
-	//   </table>                                            </table>]
-	// -[<table>                                             [<table>
-	//       <tableRow>                                          <tableRow>
-	//           <tableCell><p>fo]o</p></tableCell>    ->            <tableCell><p>foo</p></tableCell>
-	//       </tableRow>                                         </tableRow>
-	//   </table>                                            </table>]
-	const startParentLimitNode = findParentLimitNodePosition( start, schema );
-	const endParentLimitNode = findParentLimitNodePosition( end, schema );
-
-	if ( startParentLimitNode || endParentLimitNode ) {
-		let updatedStart = start;
-		let updatedEnd = end;
-
-		if ( startParentLimitNode ) {
-			updatedStart = expandSelectionOnIsLimitNode( startParentLimitNode, schema, 'start' );
-		}
+	const isStartInLimit = startLimitElement && !startLimitElement.is( 'rootElement' );
+	const isEndInLimit = endLimitElement && !endLimitElement.is( 'rootElement' );
 
-		if ( endParentLimitNode ) {
-			updatedEnd = expandSelectionOnIsLimitNode( endParentLimitNode, schema, 'end' );
-		}
+	// At this point we eliminated valid positions on text nodes so if one of range positions is placed inside a limit element
+	// then the range crossed limit element boundaries and needs to be fixed.
+	if ( isStartInLimit || isEndInLimit ) {
+		// Although we've already found limit element on start/end positions we must find the outer-most limit element.
+		// as limit elements might be nested directly inside (ie table > tableRow > tableCell).
+		const fixedStart = isStartInLimit ? expandSelectionOnIsLimitNode( Position.createAt( startLimitElement ), schema, 'start' ) : start;
+		const fixedEnd = isEndInLimit ? expandSelectionOnIsLimitNode( Position.createAt( endLimitElement ), schema, 'end' ) : end;
 
-		return new Range( updatedStart, updatedEnd );
+		return new Range( fixedStart, fixedEnd );
 	}
 
+	// Range was not fixed at this point so it is valid - ie it was placed around limit element already.
 	return null;
 }
 
@@ -246,11 +216,6 @@ function expandSelectionOnIsLimitNode( position, schema, expandToDirection ) {
 		parent = parent.parent;
 	}
 
-	if ( node === parent ) {
-		// If there is not is limit block the return original position.
-		return position;
-	}
-
 	// Depending on direction of expanding selection return position before or after found node.
 	return expandToDirection === 'start' ? Position.createBefore( node ) : Position.createAfter( node );
 }
@@ -294,23 +259,14 @@ function combineOverlappingRanges( ranges ) {
 	return combinedRanges;
 }
 
-// Goes up to the root trying to find any `isLimit=true` parent elements. Returns null if not found.
+// Checks whether both range ends are placed around non-limit elements.
 //
-// @param {module:engine/model/position~Position} position
+// @param {module:engine/model/position~Position} start
+// @param {module:engine/model/position~Position} end
 // @param {module:engine/model/schema~Schema} schema
-// @returns {module:engine/model/position~Position|null}
-function findParentLimitNodePosition( position, schema ) {
-	let parent = position.parent;
-
-	while ( parent ) {
-		if ( parent === parent.root ) {
-			return null;
-		}
-
-		if ( schema.isLimit( parent ) ) {
-			return Position.createAt( parent );
-		}
+function checkSelectionOnNonLimitElements( start, end, schema ) {
+	const startIsOnBlock = ( start.nodeAfter && !schema.isLimit( start.nodeAfter ) ) || schema.checkChild( start, '$text' );
+	const endIsOnBlock = ( end.nodeBefore && !schema.isLimit( end.nodeBefore ) ) || schema.checkChild( end, '$text' );
 
-		parent = parent.parent;
-	}
+	return startIsOnBlock && endIsOnBlock;
 }

+ 22 - 0
packages/ckeditor5-engine/tests/model/schema.js

@@ -983,6 +983,28 @@ describe( 'Schema', () => {
 
 			expect( schema.getLimitElement( doc.selection ) ).to.equal( root );
 		} );
+
+		it( 'accepts range as an argument', () => {
+			schema.extend( 'article', { isLimit: true } );
+			schema.extend( 'section', { isLimit: true } );
+
+			setData( model, '<div><section><article><paragraph>foo[]bar</paragraph></article></section></div>' );
+
+			const article = root.getNodeByPath( [ 0, 0, 0 ] );
+
+			expect( schema.getLimitElement( new Range( new Position( root, [ 0, 0, 0, 0, 2 ] ) ) ) ).to.equal( article );
+		} );
+
+		it( 'accepts position as an argument', () => {
+			schema.extend( 'article', { isLimit: true } );
+			schema.extend( 'section', { isLimit: true } );
+
+			setData( model, '<div><section><article><paragraph>foo[]bar</paragraph></article></section></div>' );
+
+			const article = root.getNodeByPath( [ 0, 0, 0 ] );
+
+			expect( schema.getLimitElement( new Position( root, [ 0, 0, 0, 0, 2 ] ) ) ).to.equal( article );
+		} );
 	} );
 
 	describe( 'checkAttributeInSelection()', () => {

+ 87 - 5
packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js

@@ -54,6 +54,16 @@ describe( 'Selection post-fixer', () => {
 				allowContentOf: '$block',
 				isLimit: true
 			} );
+
+			model.schema.register( 'inlineWidget', {
+				isObject: true,
+				allowIn: [ '$block', '$clipboardHolder' ]
+			} );
+
+			model.schema.register( 'figure', {
+				allowIn: '$root',
+				allowAttributes: [ 'name', 'title' ]
+			} );
 		} );
 
 		it( 'should not crash if there is no correct position for model selection', () => {
@@ -524,6 +534,16 @@ describe( 'Selection post-fixer', () => {
 					'<paragraph>bar</paragraph>'
 				);
 			} );
+
+			it( 'should not fix #5 (selection in root on non limit element that doesn\'t allow text)', () => {
+				setModelData( model,
+					'[<figure></figure>]'
+				);
+
+				expect( getModelData( model ) ).to.equal(
+					'[<figure></figure>]'
+				);
+			} );
 		} );
 
 		describe( 'non-collapsed selection - other scenarios', () => {
@@ -555,7 +575,49 @@ describe( 'Selection post-fixer', () => {
 				);
 			} );
 
-			it( 'should fix #3 (selection must not cross a limit element; starts in a root)', () => {
+			it( 'should fix #3 (partial selection of not an object)', () => {
+				setModelData( model,
+					'<paragraph>aaa</paragraph>' +
+					'[<paragraph>bbb</paragraph>' +
+					'<paragraph>ccc]</paragraph>'
+				);
+
+				expect( getModelData( model ) ).to.equal(
+					'<paragraph>aaa</paragraph>' +
+					'<paragraph>[bbb</paragraph>' +
+					'<paragraph>ccc]</paragraph>'
+				);
+			} );
+
+			it( 'should fix #4 (partial selection of not an object)', () => {
+				setModelData( model,
+					'<paragraph>aaa</paragraph>' +
+					'<paragraph>b[bb</paragraph>]' +
+					'<paragraph>ccc</paragraph>'
+				);
+
+				expect( getModelData( model ) ).to.equal(
+					'<paragraph>aaa</paragraph>' +
+					'<paragraph>b[bb]</paragraph>' +
+					'<paragraph>ccc</paragraph>'
+				);
+			} );
+
+			it( 'should fix #5 (partial selection of not an object)', () => {
+				setModelData( model,
+					'<paragraph>aaa</paragraph>' +
+					'[<paragraph>bb]b</paragraph>' +
+					'<paragraph>ccc</paragraph>'
+				);
+
+				expect( getModelData( model ) ).to.equal(
+					'<paragraph>aaa</paragraph>' +
+					'<paragraph>[bb]b</paragraph>' +
+					'<paragraph>ccc</paragraph>'
+				);
+			} );
+
+			it( 'should fix #6 (selection must not cross a limit element; starts in a root)', () => {
 				model.schema.register( 'a', { isLimit: true, allowIn: '$root' } );
 				model.schema.register( 'b', { isLimit: true, allowIn: 'a' } );
 				model.schema.register( 'c', { allowIn: 'b' } );
@@ -568,7 +630,7 @@ describe( 'Selection post-fixer', () => {
 				expect( getModelData( model ) ).to.equal( '[<a><b><c></c></b></a>]' );
 			} );
 
-			it( 'should fix #5 (selection must not cross a limit element; ends in a root)', () => {
+			it( 'should fix #7 (selection must not cross a limit element; ends in a root)', () => {
 				model.schema.register( 'a', { isLimit: true, allowIn: '$root' } );
 				model.schema.register( 'b', { isLimit: true, allowIn: 'a' } );
 				model.schema.register( 'c', { allowIn: 'b' } );
@@ -581,7 +643,7 @@ describe( 'Selection post-fixer', () => {
 				expect( getModelData( model ) ).to.equal( '[<a><b><c></c></b></a>]' );
 			} );
 
-			it( 'should fix #4 (selection must not cross a limit element; starts in a non-limit)', () => {
+			it( 'should fix #8 (selection must not cross a limit element; starts in a non-limit)', () => {
 				model.schema.register( 'div', { allowIn: '$root' } );
 				model.schema.register( 'a', { isLimit: true, allowIn: 'div' } );
 				model.schema.register( 'b', { isLimit: true, allowIn: 'a' } );
@@ -595,7 +657,7 @@ describe( 'Selection post-fixer', () => {
 				expect( getModelData( model ) ).to.equal( '<div>[<a><b><c></c></b></a>]</div>' );
 			} );
 
-			it( 'should fix #6 (selection must not cross a limit element; ends in a non-limit)', () => {
+			it( 'should fix #9 (selection must not cross a limit element; ends in a non-limit)', () => {
 				model.schema.register( 'div', { allowIn: '$root' } );
 				model.schema.register( 'a', { isLimit: true, allowIn: 'div' } );
 				model.schema.register( 'b', { isLimit: true, allowIn: 'a' } );
@@ -609,11 +671,31 @@ describe( 'Selection post-fixer', () => {
 				expect( getModelData( model ) ).to.equal( '<div>[<a><b><c></c></b></a>]</div>' );
 			} );
 
-			it( 'should not fix #7 (selection on text node)', () => {
+			it( 'should not fix #1 (selection on text node)', () => {
 				setModelData( model, '<paragraph>foob[a]r</paragraph>', { lastRangeBackward: true } );
 
 				expect( getModelData( model ) ).to.equal( '<paragraph>foob[a]r</paragraph>' );
 			} );
+
+			it( 'should not fix #2', () => {
+				setModelData( model,
+					'<paragraph>[<inlineWidget></inlineWidget>]</paragraph>'
+				);
+
+				expect( getModelData( model ) ).to.equal(
+					'<paragraph>[<inlineWidget></inlineWidget>]</paragraph>'
+				);
+			} );
+
+			it( 'should not fix #3', () => {
+				setModelData( model,
+					'<paragraph>fo[o<inlineWidget></inlineWidget>b]ar</paragraph>'
+				);
+
+				expect( getModelData( model ) ).to.equal(
+					'<paragraph>fo[o<inlineWidget></inlineWidget>b]ar</paragraph>'
+				);
+			} );
 		} );
 
 		describe( 'collapsed selection', () => {

+ 2 - 2
packages/ckeditor5-engine/tests/model/writer.js

@@ -2375,12 +2375,12 @@ describe( 'Writer', () => {
 		} );
 
 		it( 'should change document selection ranges', () => {
-			const range = new Range( new Position( root, [ 1 ] ), new Position( root, [ 2, 2 ] ) );
+			const range = new Range( new Position( root, [ 1, 0 ] ), new Position( root, [ 2, 2 ] ) );
 
 			setSelection( range, { backward: true } );
 
 			expect( model.document.selection._ranges.length ).to.equal( 1 );
-			expect( model.document.selection._ranges[ 0 ].start.path ).to.deep.equal( [ 1 ] );
+			expect( model.document.selection._ranges[ 0 ].start.path ).to.deep.equal( [ 1, 0 ] );
 			expect( model.document.selection._ranges[ 0 ].end.path ).to.deep.equal( [ 2, 2 ] );
 			expect( model.document.selection.isBackward ).to.be.true;
 		} );