Kaynağa Gözat

Other: Update selection post-fixer to cover more cases.

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

+ 90 - 9
packages/ckeditor5-engine/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,20 +146,45 @@ 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 ) {
+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;
 	}
 
-	const start = range.start;
-	const end = range.end;
+	// 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 ) ) {
+			return null;
+		}
+
+		return new Range( fixedStart ? fixedStart.start : start, fixedEnd ? fixedEnd.start : end );
+	}
 
+	// 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' );
 
@@ -167,6 +192,41 @@ function tryFixingNonCollpasedRage( range, schema ) {
 		return new Range( updatedStart, updatedEnd );
 	}
 
+	// If the flat range was not fixed at this point the range is valid.
+	if ( range.isFlat ) {
+		return null;
+	}
+
+	// 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' );
+		}
+
+		if ( endParentLimitNode ) {
+			updatedEnd = expandSelectionOnIsLimitNode( endParentLimitNode, schema, 'end' );
+		}
+
+		return new Range( updatedStart, updatedEnd );
+	}
+
 	return null;
 }
 
@@ -199,7 +259,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 +293,24 @@ function combineOverlapingRanges( ranges ) {
 
 	return combinedRanges;
 }
+
+// Goes up to the root trying to find any `isLimit=true` parent elements. Returns null if not found.
+//
+// @param {module:engine/model/position~Position} position
+// @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 );
+		}
+
+		parent = parent.parent;
+	}
+}

+ 9 - 9
packages/ckeditor5-engine/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>]'
 			);
 		} );
 	} );

+ 4 - 4
packages/ckeditor5-engine/tests/model/schema.js

@@ -1123,12 +1123,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 +1141,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', () => {

+ 22 - 4
packages/ckeditor5-engine/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>' );

+ 1 - 1
packages/ckeditor5-engine/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' } );
 

+ 88 - 1
packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js

@@ -484,6 +484,46 @@ describe( 'Selection post-fixer', () => {
 					'<paragraph>bar</paragraph>'
 				);
 			} );
+
+			it( 'should not fix #3 (inside a limit - partial text selection)', () => {
+				model.change( writer => {
+					const caption = modelRoot.getChild( 1 ).getChild( 0 );
+
+					// <paragraph>foo</paragraph><image><caption>[xx]x</caption></image>...
+					writer.setSelection( ModelRange.createFromParentsAndOffsets(
+						caption, 0,
+						caption, 2
+					) );
+				} );
+
+				expect( getModelData( model ) ).to.equal(
+					'<paragraph>foo</paragraph>' +
+					'<image>' +
+						'<caption>[xx]x</caption>' +
+					'</image>' +
+					'<paragraph>bar</paragraph>'
+				);
+			} );
+
+			it( 'should not fix #4 (inside a limit - partial text selection)', () => {
+				model.change( writer => {
+					const caption = modelRoot.getChild( 1 ).getChild( 0 );
+
+					// <paragraph>foo</paragraph><image><caption>x[xx]</caption></image>...
+					writer.setSelection( ModelRange.createFromParentsAndOffsets(
+						caption, 1,
+						caption, 3
+					) );
+				} );
+
+				expect( getModelData( model ) ).to.equal(
+					'<paragraph>foo</paragraph>' +
+					'<image>' +
+						'<caption>x[xx]</caption>' +
+					'</image>' +
+					'<paragraph>bar</paragraph>'
+				);
+			} );
 		} );
 
 		describe( 'non-collapsed selection - other scenarios', () => {
@@ -515,7 +555,7 @@ describe( 'Selection post-fixer', () => {
 				);
 			} );
 
-			it( 'should fix #3 (selection must not cross a limit element; starts in a non-limit)', () => {
+			it( 'should fix #3 (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' } );
@@ -527,6 +567,53 @@ 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)', () => {
+				model.schema.register( 'a', { isLimit: true, allowIn: '$root' } );
+				model.schema.register( 'b', { isLimit: true, allowIn: 'a' } );
+				model.schema.register( 'c', { allowIn: 'b' } );
+				model.schema.extend( '$text', { allowIn: 'c' } );
+
+				setModelData( model,
+					'[<a><b><c>]</c></b></a>'
+				);
+
+				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)', () => {
+				model.schema.register( 'div', { allowIn: '$root' } );
+				model.schema.register( 'a', { isLimit: true, allowIn: 'div' } );
+				model.schema.register( 'b', { isLimit: true, allowIn: 'a' } );
+				model.schema.register( 'c', { allowIn: 'b' } );
+				model.schema.extend( '$text', { allowIn: 'c' } );
+
+				setModelData( model,
+					'<div>[<a><b><c>]</c></b></a></div>'
+				);
+
+				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)', () => {
+				model.schema.register( 'div', { allowIn: '$root' } );
+				model.schema.register( 'a', { isLimit: true, allowIn: 'div' } );
+				model.schema.register( 'b', { isLimit: true, allowIn: 'a' } );
+				model.schema.register( 'c', { allowIn: 'b' } );
+				model.schema.extend( '$text', { allowIn: 'c' } );
+
+				setModelData( model,
+					'<div><a><b><c>[</c></b></a>]</div>'
+				);
+
+				expect( getModelData( model ) ).to.equal( '<div>[<a><b><c></c></b></a>]</div>' );
+			} );
+
+			it( 'should not fix #7 (selection on text node)', () => {
+				setModelData( model, '<paragraph>foob[a]r</paragraph>', { lastRangeBackward: true } );
+
+				expect( getModelData( model ) ).to.equal( '<paragraph>foob[a]r</paragraph>' );
+			} );
 		} );
 
 		describe( 'collapsed selection', () => {