T/1436: Fixing selection that cross limit node boundaries.

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 ) {

src/model/utils/selection-post-fixer.js

@@ -94,7 +94,7 @@ function selectionPostFixer( writer, model ) {
 	if ( wasFixed ) {
 		// The above algorithm might create ranges that intersects each other when selection contains more then one range.
 		// This is case happens mostly on Firefox which creates multiple ranges for selected table.
-		const combinedRanges = combineOverlapingRanges( ranges );
+		const combinedRanges = combineOverlappingRanges( ranges );
 
 		writer.setSelection( combinedRanges, { backward: selection.isBackward } );
 	}
@@ -110,7 +110,7 @@ function tryFixingRange( range, schema ) {
 		return tryFixingCollapsedRange( range, schema );
 	}
 
-	return tryFixingNonCollpasedRage( range, schema );
+	return tryFixingNonCollapsedRage( range, schema );
 }
 
 // Tries to fix collapsed ranges.
@@ -146,27 +146,57 @@ function tryFixingCollapsedRange( range, schema ) {
 	return new Range( fixedPosition );
 }
 
-// Tries to fix a expanded range that overlaps limit nodes.
+// Tries to fix an expanded range.
 //
 // @param {module:engine/model/range~Range} range Expanded range to fix.
 // @param {module:engine/model/schema~Schema} schema
 // @returns {module:engine/model/range~Range|null} Returns fixed range or null if range is valid.
-function tryFixingNonCollpasedRage( range, schema ) {
-	// No need to check flat ranges as they will not cross node boundary.
-	if ( range.isFlat ) {
-		return null;
-	}
-
+function tryFixingNonCollapsedRage( range, schema ) {
 	const start = range.start;
 	const end = range.end;
 
-	const updatedStart = expandSelectionOnIsLimitNode( start, schema, 'start' );
-	const updatedEnd = expandSelectionOnIsLimitNode( end, schema, '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;
+		}
+
+		// 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' );
 
-	if ( !start.isEqual( updatedStart ) || !end.isEqual( updatedEnd ) ) {
-		return new Range( updatedStart, updatedEnd );
+			return new Range( fixedStart ? fixedStart.start : start, fixedEnd ? fixedEnd.start : end );
+		}
 	}
 
+	const isStartInLimit = startLimitElement && !startLimitElement.is( 'rootElement' );
+	const isEndInLimit = endLimitElement && !endLimitElement.is( 'rootElement' );
+
+	// 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( fixedStart, fixedEnd );
+	}
+
+	// Range was not fixed at this point so it is valid - ie it was placed around limit element already.
 	return null;
 }
 
@@ -186,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 );
 }
@@ -199,7 +224,7 @@ function expandSelectionOnIsLimitNode( position, schema, expandToDirection ) {
 //
 // @param {Array.<module:engine/model/range~Range>} ranges
 // @returns {Array.<module:engine/model/range~Range>}
-function combineOverlapingRanges( ranges ) {
+function combineOverlappingRanges( ranges ) {
 	const combinedRanges = [];
 
 	// Seed the state.
@@ -233,3 +258,15 @@ function combineOverlapingRanges( ranges ) {
 
 	return combinedRanges;
 }
+
+// Checks whether both range ends are placed around non-limit elements.
+//
+// @param {module:engine/model/position~Position} start
+// @param {module:engine/model/position~Position} end
+// @param {module:engine/model/schema~Schema} schema
+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' );
+
+	return startIsOnBlock && endIsOnBlock;
+}

tests/conversion/downcast-selection-converters.js

@@ -494,9 +494,9 @@ describe( 'downcast-selection-converters', () => {
 
 	describe( 'table cell selection converter', () => {
 		beforeEach( () => {
-			model.schema.register( 'table' );
-			model.schema.register( 'tr' );
-			model.schema.register( 'td' );
+			model.schema.register( 'table', { isLimit: true } );
+			model.schema.register( 'tr', { isLimit: true } );
+			model.schema.register( 'td', { isLimit: true } );
 
 			model.schema.extend( 'table', { allowIn: '$root' } );
 			model.schema.extend( 'tr', { allowIn: 'table' } );
@@ -519,16 +519,16 @@ describe( 'downcast-selection-converters', () => {
 				}
 
 				for ( const range of selection.getRanges() ) {
-					const node = range.start.nodeAfter;
+					const node = range.start.parent;
 
-					if ( node == range.end.nodeBefore && node instanceof ModelElement && node.name == 'td' ) {
+					if ( node instanceof ModelElement && node.name == 'td' ) {
 						conversionApi.consumable.consume( selection, 'selection' );
 
 						const viewNode = conversionApi.mapper.toViewElement( node );
 						conversionApi.writer.addClass( 'selected', viewNode );
 					}
 				}
-			} );
+			}, { priority: 'high' } );
 		} );
 
 		it( 'should not be used to convert selection that is not on table cell', () => {
@@ -542,7 +542,7 @@ describe( 'downcast-selection-converters', () => {
 		it( 'should add a class to the selected table cell', () => {
 			test(
 				// table tr#0 |td#0, table tr#0 td#0|
-				[ [ 0, 0, 0 ], [ 0, 0, 1 ] ],
+				[ [ 0, 0, 0, 0 ], [ 0, 0, 0, 3 ] ],
 				'<table><tr><td>foo</td></tr><tr><td>bar</td></tr></table>',
 				'<table><tr><td class="selected">foo</td></tr><tr><td>bar</td></tr></table>'
 			);
@@ -551,9 +551,9 @@ describe( 'downcast-selection-converters', () => {
 		it( 'should not be used if selection contains more than just a table cell', () => {
 			test(
 				// table tr td#1, table tr#2
-				[ [ 0, 0, 0, 1 ], [ 0, 0, 2 ] ],
+				[ [ 0, 0, 0, 1 ], [ 0, 0, 1, 3 ] ],
 				'<table><tr><td>foo</td><td>bar</td></tr></table>',
-				'<table><tr><td>f{oo</td><td>bar</td>]</tr></table>'
+				'[<table><tr><td>foo</td><td>bar</td></tr></table>]'
 			);
 		} );
 	} );

tests/model/schema.js

@@ -17,7 +17,7 @@ import Position from '../../src/model/position';
 import Range from '../../src/model/range';
 import Selection from '../../src/model/selection';
 
-import { setData, getData, stringify } from '../../src/dev-utils/model';
+import { getData, setData, stringify } from '../../src/dev-utils/model';
 
 import AttributeDelta from '../../src/model/delta/attributedelta';
 
@@ -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()', () => {
@@ -1101,7 +1123,13 @@ describe( 'Schema', () => {
 				}
 			} );
 
-			setData( model, '<p>foo<img />bar</p>' );
+			// Parse data string to model.
+			const parsedResult = setData._parse( '[<p>foo<img />bar</p>]', model.schema, { context: [ root.name ] } );
+
+			// Set parsed model data to prevent selection post-fixer from running.
+			model.change( writer => {
+				writer.insert( parsedResult.model, root );
+			} );
 
 			ranges = [ Range.createOn( root.getChild( 0 ) ) ];
 		} );
@@ -1123,12 +1151,12 @@ describe( 'Schema', () => {
 			schema.extend( 'img', { allowAttributes: 'bold' } );
 			schema.extend( '$text', { allowIn: 'img' } );
 
-			setData( model, '[<p>foo<img>xxx</img>bar</p>]' );
+			setData( model, '<p>[foo<img>xxx</img>bar]</p>' );
 
 			const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute );
 			const sel = new Selection( validRanges );
 
-			expect( stringify( root, sel ) ).to.equal( '[<p>foo<img>]xxx[</img>bar</p>]' );
+			expect( stringify( root, sel ) ).to.equal( '<p>[foo<img>]xxx[</img>bar]</p>' );
 		} );
 
 		it( 'should return three ranges when attribute is not allowed on one element but is allowed on its child', () => {
@@ -1141,12 +1169,12 @@ describe( 'Schema', () => {
 				}
 			} );
 
-			setData( model, '[<p>foo<img>xxx</img>bar</p>]' );
+			setData( model, '<p>[foo<img>xxx</img>bar]</p>' );
 
 			const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute );
 			const sel = new Selection( validRanges );
 
-			expect( stringify( root, sel ) ).to.equal( '[<p>foo]<img>[xxx]</img>[bar</p>]' );
+			expect( stringify( root, sel ) ).to.equal( '<p>[foo]<img>[xxx]</img>[bar]</p>' );
 		} );
 
 		it( 'should not leak beyond the given ranges', () => {

tests/model/utils/deletecontent.js

@@ -562,7 +562,7 @@ describe( 'DataController utils', () => {
 				schema.register( 'image', { allowWhere: '$text' } );
 				schema.register( 'paragraph', { inheritAllFrom: '$block' } );
 				schema.register( 'heading1', { inheritAllFrom: '$block' } );
-				schema.register( 'blockWidget' );
+				schema.register( 'blockWidget', { isLimit: true } );
 				schema.register( 'restrictedRoot', {
 					isLimit: true
 				} );
@@ -621,7 +621,7 @@ describe( 'DataController utils', () => {
 				deleteContent( model, selection );
 
 				expect( getData( model, { rootName: 'bodyRoot' } ) )
-					.to.equal( '[<paragraph>x</paragraph>]<paragraph></paragraph><paragraph>z</paragraph>' );
+					.to.equal( '<paragraph>[x]</paragraph><paragraph></paragraph><paragraph>z</paragraph>' );
 			} );
 
 			it( 'creates a paragraph when text is not allowed (block widget selected)', () => {
@@ -644,7 +644,16 @@ describe( 'DataController utils', () => {
 					{ rootName: 'bodyRoot' }
 				);
 
-				deleteContent( model, doc.selection );
+				model.change( writer => {
+					// Set selection to[<heading1>yyy</heading1>] in change() block due to selection post-fixer.
+					const range = new Range(
+						new Position( doc.getRoot( 'bodyRoot' ), [ 1 ] ),
+						new Position( doc.getRoot( 'bodyRoot' ), [ 2 ] )
+					);
+					writer.setSelection( range );
+
+					deleteContent( model, doc.selection );
+				} );
 
 				expect( getData( model, { rootName: 'bodyRoot' } ) )
 					.to.equal( '<paragraph>x</paragraph><paragraph>[]</paragraph><paragraph>z</paragraph>' );
@@ -657,7 +666,16 @@ describe( 'DataController utils', () => {
 					{ rootName: 'bodyRoot' }
 				);
 
-				deleteContent( model, doc.selection );
+				model.change( writer => {
+					// Set selection to[<heading1>yyy</heading1><paragraph>yyy</paragraph>] in change() block due to selection post-fixer.
+					const range = new Range(
+						new Position( doc.getRoot( 'bodyRoot' ), [ 1 ] ),
+						new Position( doc.getRoot( 'bodyRoot' ), [ 3 ] )
+					);
+					writer.setSelection( range );
+
+					deleteContent( model, doc.selection );
+				} );
 
 				expect( getData( model, { rootName: 'bodyRoot' } ) )
 					.to.equal( '<paragraph>x</paragraph><paragraph>[]</paragraph><paragraph>z</paragraph>' );

tests/model/utils/getselectedcontent.js

@@ -129,7 +129,7 @@ describe( 'DataController utils', () => {
 
 				schema.register( 'paragraph', { inheritAllFrom: '$block' } );
 				schema.register( 'heading1', { inheritAllFrom: '$block' } );
-				schema.register( 'blockImage' );
+				schema.register( 'blockImage', { isObject: true } );
 				schema.register( 'caption' );
 				schema.register( 'image', { allowWhere: '$text' } );