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

src/model/schema.js

@@ -582,26 +582,38 @@ export default class Schema {
 		}, { priority: 'high' } );
 	}
 
+	/* eslint-disable max-len */
 	/**
 	 * 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|module:engine/model/range~Range|module:engine/model/position~Position} 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;
-				}
+	/* eslint-enable max-len */
+	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,9 +94,18 @@ 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 );
+		let fixedRanges = ranges;
 
-		writer.setSelection( combinedRanges, { backward: selection.isBackward } );
+		// Fixing selection with many ranges usually breaks the selection in Firefox. As only Firefox supports multiple selection ranges
+		// we simply create one continuous range from fixed selection ranges (even if they are not adjacent).
+		if ( ranges.length > 1 ) {
+			const selectionStart = ranges[ 0 ].start;
+			const selectionEnd = ranges[ ranges.length - 1 ].end;
+
+			fixedRanges = [ new Range( selectionStart, selectionEnd ) ];
+		}
+
+		writer.setSelection( fixedRanges, { backward: selection.isBackward } );
 	}
 }
 
@@ -110,7 +119,7 @@ function tryFixingRange( range, schema ) {
 		return tryFixingCollapsedRange( range, schema );
 	}
 
-	return tryFixingNonCollpasedRage( range, schema );
+	return tryFixingNonCollapsedRage( range, schema );
 }
 
 // Tries to fix collapsed ranges.
@@ -146,27 +155,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' );
+
+			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;
 
-	if ( !start.isEqual( updatedStart ) || !end.isEqual( updatedEnd ) ) {
-		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;
 }
 
@@ -186,50 +225,18 @@ 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 );
 }
 
-// Returns minimal set of continuous ranges.
+// Checks whether both range ends are placed around non-limit elements.
 //
-// @param {Array.<module:engine/model/range~Range>} ranges
-// @returns {Array.<module:engine/model/range~Range>}
-function combineOverlapingRanges( ranges ) {
-	const combinedRanges = [];
-
-	// Seed the state.
-	let previousRange = ranges[ 0 ];
-	combinedRanges.push( previousRange );
-
-	// Go through each ranges and check if it can be merged with previous one.
-	for ( const range of ranges ) {
-		// Do not push same ranges (ie might be created in a table).
-		if ( range.isEqual( previousRange ) ) {
-			continue;
-		}
-
-		// Merge intersecting range into previous one.
-		if ( range.isIntersecting( previousRange ) ) {
-			const newStart = previousRange.start.isBefore( range.start ) ? previousRange.start : range.start;
-			const newEnd = range.end.isAfter( previousRange.end ) ? range.end : previousRange.end;
-			const combinedRange = new Range( newStart, newEnd );
-
-			// Replace previous range with the combined one.
-			combinedRanges.splice( combinedRanges.indexOf( previousRange ), 1, combinedRange );
-
-			previousRange = combinedRange;
-
-			continue;
-		}
-
-		previousRange = range;
-		combinedRanges.push( range );
-	}
+// @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 combinedRanges;
+	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', () => {
@@ -541,19 +541,19 @@ 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 ] ],
+				// table tr#0 td#0 [foo, table tr#0 td#0 bar]
+				[ [ 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>'
 			);
 		} );
 
 		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 ] ],
+				// table tr td#1 f{oo bar, table tr#2 bar]
+				[ [ 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, parse } from '../../src/dev-utils/model';
 
 import AttributeDelta from '../../src/model/delta/attributedelta';
 
@@ -983,6 +983,38 @@ 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 } );
+
+			const data = '<div><section><article><paragraph>foobar</paragraph></article></section></div>';
+			const parsedModel = parse( data, model.schema, { context: [ root.name ] } );
+
+			model.change( writer => {
+				writer.insert( parsedModel, root );
+			} );
+
+			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 } );
+
+			const data = '<div><section><article><paragraph>foobar</paragraph></article></section></div>';
+			const parsedModel = parse( data, model.schema, { context: [ root.name ] } );
+
+			model.change( writer => {
+				writer.insert( parsedModel, root );
+			} );
+
+			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 +1133,13 @@ describe( 'Schema', () => {
 				}
 			} );
 
-			setData( model, '<p>foo<img />bar</p>' );
+			// Parse data string to model.
+			const parsedModel = 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( parsedModel, root );
+			} );
 
 			ranges = [ Range.createOn( root.getChild( 0 ) ) ];
 		} );
@@ -1123,12 +1161,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 +1179,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', () => {