Merge pull request #1645 from ckeditor/t/ckeditor5-table/163

Fix: `Selection#getTopMostBlocks()` should not leak from limit elements. Closes ckeditor/ckeditor5-table#163.
ckeditor · Feb 12, 2019 · 7bc0338 · 7bc0338
2 parents 4f9ac0e + 5c51493
commit 7bc0338
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 17 deletions.
diff --git a/src/model/selection.js b/src/model/selection.js
@@ -826,10 +826,25 @@ function isUnvisitedBlockContainer( element, visited ) {
 }
 
 // Finds the lowest element in position's ancestors which is a block.
+// It will search until first ancestor that is a limit element.
 // Marks all ancestors as already visited to not include any of them later on.
 function getParentBlock( position, visited ) {
+	const schema = position.parent.document.model.schema;
+
 	const ancestors = position.parent.getAncestors( { parentFirst: true, includeSelf: true } );
-	const block = ancestors.find( element => isUnvisitedBlockContainer( element, visited ) );
+
+	let hasParentLimit = false;
+
+	const block = ancestors.find( element => {
+		// Stop searching after first parent node that is limit element.
+		if ( hasParentLimit ) {
+			return false;
+		}
+
+		hasParentLimit = schema.isLimit( element );
+
+		return !hasParentLimit && isUnvisitedBlockContainer( element, visited );
+	} );
 
 	// Mark all ancestors of this position's parent, because find() might've stopped early and
 	// the found block may be a child of another block.

diff --git a/tests/model/selection.js b/tests/model/selection.js
@@ -1117,6 +1117,20 @@ describe( 'Selection', () => {
 			);
 		} );
 
+		it( 'does not go beyond limit elements', () => {
+			model.schema.register( 'table', { isBlock: true, isLimit: true, isObject: true, allowIn: '$root' } );
+			model.schema.register( 'tableRow', { allowIn: 'table', isLimit: true } );
+			model.schema.register( 'tableCell', { allowIn: 'tableRow', isObject: true } );
+
+			model.schema.register( 'blk', { allowIn: [ '$root', 'tableCell' ], isObject: true, isBlock: true } );
+
+			model.schema.extend( 'p', { allowIn: 'tableCell' } );
+
+			setData( model, '<table><tableRow><tableCell><p>foo</p>[<blk></blk><p>bar]</p></tableCell></tableRow></table>' );
+
+			expect( stringifyBlocks( doc.selection.getTopMostBlocks() ) ).to.deep.equal( [ 'blk', 'p#bar' ] );
+		} );
+
 		// Map all elements to data of its first child text node.
 		function toText( elements ) {
 			return Array.from( elements ).map( el => {
@@ -1128,11 +1142,11 @@ describe( 'Selection', () => {
 	describe( 'getTopMostBlocks()', () => {
 		beforeEach( () => {
 			model.schema.register( 'p', { inheritAllFrom: '$block' } );
-			model.schema.register( 'lvl0', { isBlock: true, isLimit: true, isObject: true, allowIn: '$root' } );
-			model.schema.register( 'lvl1', { allowIn: 'lvl0', isLimit: true } );
-			model.schema.register( 'lvl2', { allowIn: 'lvl1', isObject: true } );
+			model.schema.register( 'table', { isBlock: true, isLimit: true, isObject: true, allowIn: '$root' } );
+			model.schema.register( 'tableRow', { allowIn: 'table', isLimit: true } );
+			model.schema.register( 'tableCell', { allowIn: 'tableRow', isObject: true } );
 
-			model.schema.extend( 'p', { allowIn: 'lvl2' } );
+			model.schema.extend( 'p', { allowIn: 'tableCell' } );
 		} );
 
 		it( 'returns an iterator', () => {
@@ -1169,28 +1183,24 @@ describe( 'Selection', () => {
 		} );
 
 		it( 'returns only top most blocks', () => {
-			setData( model, '[<p>foo</p><lvl0><lvl1><lvl2><p>bar</p></lvl2></lvl1></lvl0><p>baz</p>]' );
+			setData( model, '[<p>foo</p><table><tableRow><tableCell><p>bar</p></tableCell></tableRow></table><p>baz</p>]' );
 
-			expect( stringifyBlocks( doc.selection.getTopMostBlocks() ) ).to.deep.equal( [ 'p#foo', 'lvl0', 'p#baz' ] );
+			expect( stringifyBlocks( doc.selection.getTopMostBlocks() ) ).to.deep.equal( [ 'p#foo', 'table', 'p#baz' ] );
 		} );
 
 		it( 'returns only selected blocks even if nested in other blocks', () => {
-			setData( model, '<p>foo</p><lvl0><lvl1><lvl2><p>[b]ar</p></lvl2></lvl1></lvl0><p>baz</p>' );
+			setData( model, '<p>foo</p><table><tableRow><tableCell><p>[b]ar</p></tableCell></tableRow></table><p>baz</p>' );
 
 			expect( stringifyBlocks( doc.selection.getTopMostBlocks() ) ).to.deep.equal( [ 'p#bar' ] );
 		} );
 
-		// Map all elements to names. If element contains child text node it will be appended to name with '#'.
-		function stringifyBlocks( elements ) {
-			return Array.from( elements ).map( el => {
-				const name = el.name;
+		it( 'returns only selected blocks even if nested in other blocks (selection on the block)', () => {
+			model.schema.register( 'blk', { allowIn: [ '$root', 'tableCell' ], isObject: true, isBlock: true } );
 
-				const firstChild = el.getChild( 0 );
-				const hasText = firstChild && firstChild.data;
+			setData( model, '<table><tableRow><tableCell><p>foo</p>[<blk></blk><p>bar]</p></tableCell></tableRow></table>' );
 
-				return hasText ? `${ name }#${ firstChild.data }` : name;
-			} );
-		}
+			expect( stringifyBlocks( doc.selection.getTopMostBlocks() ) ).to.deep.equal( [ 'blk', 'p#bar' ] );
+		} );
 	} );
 
 	describe( 'attributes interface', () => {
@@ -1366,4 +1376,16 @@ describe( 'Selection', () => {
 			expect( doc.selection.containsEntireContent() ).to.equal( false );
 		} );
 	} );
+
+	// Map all elements to names. If element contains child text node it will be appended to name with '#'.
+	function stringifyBlocks( elements ) {
+		return Array.from( elements ).map( el => {
+			const name = el.name;
+
+			const firstChild = el.getChild( 0 );
+			const hasText = firstChild && firstChild.data;
+
+			return hasText ? `${ name }#${ firstChild.data }` : name;
+		} );
+	}
 } );