Merge pull request #985 from ckeditor/t/984

Other: The `Selection#getSelectedBlocks()` method will not return a block in which selection ends if no content of that block is selected. Closes #984.

For example, in the following case only the first two paragraphs will be returned:

```html
<paragraph>[Foo</paragraph>
<paragraph>Bar</paragraph>
<paragraph>]Baz</paragraph>
```

The reasoning behind this change is that the user doesn't consider the last block as selected in such a case (as its selection isn't even visible).

Author: szymonkups

src/model/selection.js

@@ -576,7 +576,7 @@ export default class Selection {
 	 *
 	 * This method's result can be used for example to apply block styling to all blocks covered by this selection.
 	 *
-	 * *Note:* `getSelectedBlocks` always returns the deepest block.
+	 * **Note:** `getSelectedBlocks()` always returns the deepest block.
 	 *
 	 * In this case the function will return exactly all 3 paragraphs:
 	 *
@@ -590,6 +590,13 @@ export default class Selection {
 	 *
 	 *		<paragraph>[]a</paragraph>
 	 *
+	 * **Special case**: If a selection ends at the beginning of a block, that block is not returned as from user perspective
+	 * this block wasn't selected. See [#984](https://github.com/ckeditor/ckeditor5-engine/issues/984) for more details.
+	 *
+	 *		<paragraph>[a</paragraph>
+	 *		<paragraph>b</paragraph>
+	 *		<paragraph>]c</paragraph> // this block will not be returned
+	 *
 	 * @returns {Iterator.<module:engine/model/element~Element>}
 	 */
 	* getSelectedBlocks() {
@@ -610,7 +617,8 @@ export default class Selection {
 
 			const endBlock = getParentBlock( range.end, visited );
 
-			if ( endBlock ) {
+			// #984. Don't return the end block if the range ends right at its beginning.
+			if ( endBlock && !range.end.isTouching( Position.createAt( endBlock ) ) ) {
 				yield endBlock;
 			}
 		}

tests/model/position.js

@@ -15,7 +15,7 @@ import { jsonParseStringify } from '../../tests/model/_utils/utils';
 
 testUtils.createSinonSandbox();
 
-describe( 'position', () => {
+describe( 'Position', () => {
 	let doc, root, otherRoot, p, ul, li1, li2, f, o, z, b, a, r, foz, bar;
 
 	// root
@@ -118,7 +118,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'createFromParentAndOffset', () => {
+	describe( 'createFromParentAndOffset()', () => {
 		it( 'should create positions form node and offset', () => {
 			expect( Position.createFromParentAndOffset( root, 0 ) ).to.have.property( 'path' ).that.deep.equals( [ 0 ] );
 			expect( Position.createFromParentAndOffset( root, 1 ) ).to.have.property( 'path' ).that.deep.equals( [ 1 ] );
@@ -149,7 +149,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'createAt', () => {
+	describe( 'createAt()', () => {
 		it( 'should create positions from positions', () => {
 			const spy = testUtils.sinon.spy( Position, 'createFromPosition' );
 
@@ -177,7 +177,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'createBefore', () => {
+	describe( 'createBefore()', () => {
 		it( 'should create positions before elements', () => {
 			expect( Position.createBefore( p ) ).to.have.property( 'path' ).that.deep.equals( [ 0 ] );
 
@@ -203,7 +203,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'createAfter', () => {
+	describe( 'createAfter()', () => {
 		it( 'should create positions after elements', () => {
 			expect( Position.createAfter( p ) ).to.have.property( 'path' ).that.deep.equals( [ 1 ] );
 
@@ -229,7 +229,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'createFromPosition', () => {
+	describe( 'createFromPosition()', () => {
 		it( 'should create a copy of given position', () => {
 			const original = new Position( root, [ 1, 2, 3 ] );
 			const position = Position.createFromPosition( original );
@@ -361,7 +361,7 @@ describe( 'position', () => {
 		expect( position.getParentPath() ).to.deep.equal( [ 1, 2 ] );
 	} );
 
-	describe( 'isBefore', () => {
+	describe( 'isBefore()', () => {
 		it( 'should return true if given position has same root and is before this position', () => {
 			const position = new Position( root, [ 1, 1, 2 ] );
 			const beforePosition = new Position( root, [ 1, 0 ] );
@@ -384,7 +384,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'isEqual', () => {
+	describe( 'isEqual()', () => {
 		it( 'should return true if given position has same path and root', () => {
 			const position = new Position( root, [ 1, 1, 2 ] );
 			const samePosition = new Position( root, [ 1, 1, 2 ] );
@@ -407,7 +407,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'isAfter', () => {
+	describe( 'isAfter()', () => {
 		it( 'should return true if given position has same root and is after this position', () => {
 			const position = new Position( root, [ 1, 1, 2 ] );
 			const afterPosition = new Position( root, [ 1, 2 ] );
@@ -430,7 +430,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'isTouching', () => {
+	describe( 'isTouching()', () => {
 		it( 'should return true if positions are same', () => {
 			const position = new Position( root, [ 1, 1, 1 ] );
 			const result = position.isTouching( new Position( root, [ 1, 1, 1 ] ) );
@@ -470,6 +470,14 @@ describe( 'position', () => {
 			expect( positionB.isTouching( positionA ) ).to.be.false;
 		} );
 
+		it( 'should return false if there are whole nodes between positions (same depth)', () => {
+			const positionA = new Position( root, [ 1, 0 ] );
+			const positionB = new Position( root, [ 1, 1 ] );
+
+			expect( positionA.isTouching( positionB ) ).to.be.false;
+			expect( positionB.isTouching( positionA ) ).to.be.false;
+		} );
+
 		it( 'should return false if there are whole nodes between positions', () => {
 			const positionA = new Position( root, [ 1, 0, 3 ] );
 			const positionB = new Position( root, [ 1, 1, 1 ] );
@@ -487,21 +495,21 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'isAtStart', () => {
+	describe( 'isAtStart()', () => {
 		it( 'should return true if position is at the beginning of its parent', () => {
 			expect( new Position( root, [ 0 ] ).isAtStart ).to.be.true;
 			expect( new Position( root, [ 1 ] ).isAtStart ).to.be.false;
 		} );
 	} );
 
-	describe( 'isAtEnd', () => {
+	describe( 'isAtEnd()', () => {
 		it( 'should return true if position is at the end of its parent', () => {
 			expect( new Position( root, [ root.maxOffset ] ).isAtEnd ).to.be.true;
 			expect( new Position( root, [ 0 ] ).isAtEnd ).to.be.false;
 		} );
 	} );
 
-	describe( 'getAncestors', () => {
+	describe( 'getAncestors()', () => {
 		it( 'should return position parent element and it\'s ancestors', () => {
 			expect( new Position( root, [ 1, 1, 1 ] ).getAncestors() ).to.deep.equal( [ root, ul, li2 ] );
 		} );
@@ -513,7 +521,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'getCommonPath', () => {
+	describe( 'getCommonPath()', () => {
 		it( 'returns the common part', () => {
 			const pos1 = new Position( root, [ 1, 0, 0 ] );
 			const pos2 = new Position( root, [ 1, 0, 1 ] );
@@ -548,7 +556,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'compareWith', () => {
+	describe( 'compareWith()', () => {
 		it( 'should return same if positions are same', () => {
 			const position = new Position( root, [ 1, 2, 3 ] );
 			const compared = new Position( root, [ 1, 2, 3 ] );
@@ -578,7 +586,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'getLastMatchingPosition', () => {
+	describe( 'getLastMatchingPosition()', () => {
 		it( 'should skip forward', () => {
 			let position = new Position( root, [ 1, 0, 0 ] );
 
@@ -596,7 +604,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( '_getTransformedByInsertion', () => {
+	describe( '_getTransformedByInsertion()', () => {
 		it( 'should return a new Position instance', () => {
 			const position = new Position( root, [ 0 ] );
 			const transformed = position._getTransformedByInsertion( new Position( root, [ 2 ] ), 4, false );
@@ -656,7 +664,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( '_getTransformedByDeletion', () => {
+	describe( '_getTransformedByDeletion()', () => {
 		it( 'should return a new Position instance', () => {
 			const position = new Position( root, [ 0 ] );
 			const transformed = position._getTransformedByDeletion( new Position( root, [ 2 ] ), 4 );
@@ -716,7 +724,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( '_getTransformedByMove', () => {
+	describe( '_getTransformedByMove()', () => {
 		it( 'should increment offset if a range was moved to the same parent and closer offset', () => {
 			const position = new Position( root, [ 1, 2, 3 ] );
 			const transformed = position._getTransformedByMove( new Position( root, [ 2 ] ), new Position( root, [ 1, 2, 0 ] ), 3, false );
@@ -758,7 +766,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( '_getCombined', () => {
+	describe( '_getCombined()', () => {
 		it( 'should return correct combination of this and given positions', () => {
 			const position = new Position( root, [ 1, 3, 4, 2 ] );
 			const sourcePosition = new Position( root, [ 1, 1 ] );
@@ -770,7 +778,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'getShiftedBy', () => {
+	describe( 'getShiftedBy()', () => {
 		it( 'should return a new instance of Position with offset changed by shift value', () => {
 			const position = new Position( root, [ 1, 2, 3 ] );
 			const shifted = position.getShiftedBy( 2 );
@@ -795,7 +803,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'toJSON', () => {
+	describe( 'toJSON()', () => {
 		it( 'should serialize position', () => {
 			const position = new Position( root, [ 0 ] );
 
@@ -813,7 +821,7 @@ describe( 'position', () => {
 		} );
 	} );
 
-	describe( 'fromJSON', () => {
+	describe( 'fromJSON()', () => {
 		it( 'should create object with given document', () => {
 			const deserialized = Position.fromJSON( { root: 'main', path: [ 0, 1, 2 ] }, doc );
 

tests/model/selection.js

@@ -865,6 +865,7 @@ describe( 'Selection', () => {
 
 			doc.schema.registerItem( 'image' );
 			doc.schema.allow( { name: 'image', inside: '$root' } );
+			doc.schema.allow( { name: 'image', inside: '$block' } );
 			doc.schema.allow( { name: '$text', inside: 'image' } );
 
 			// Special block which can contain another blocks.
@@ -884,6 +885,15 @@ describe( 'Selection', () => {
 			expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'b' ] );
 		} );
 
+		it( 'returns block for a collapsed selection (empty block)', () => {
+			setData( doc, '<p>a</p><p>[]</p><p>c</p>' );
+
+			const blocks = Array.from( doc.selection.getSelectedBlocks() );
+
+			expect( blocks ).to.have.length( 1 );
+			expect( blocks[ 0 ].childCount ).to.equal( 0 );
+		} );
+
 		it( 'returns block for a non collapsed selection', () => {
 			setData( doc, '<p>a</p><p>[b]</p><p>c</p>' );
 
@@ -896,11 +906,8 @@ describe( 'Selection', () => {
 			expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'b', 'c' ] );
 		} );
 
-		// This isn't a behaviour that we like (https://github.com/ckeditor/ckeditor5-heading/issues/9) but I don't
-		// want to work on this issue now, when porting this method from the list/heading features.
-		// It has to be covered separately.
-		it( 'returns two blocks for a non collapsed selection if only end of selection is touching a block', () => {
-			setData( doc, '<p>a</p><h>b[</h><p>]c</p><p>d</p>' );
+		it( 'returns two blocks for a non collapsed selection (starts at block end)', () => {
+			setData( doc, '<p>a</p><h>b[</h><p>c]</p><p>d</p>' );
 
 			expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'b', 'c' ] );
 		} );
@@ -980,8 +987,62 @@ describe( 'Selection', () => {
 			expect( toText( doc.selection.getSelectedBlocks() ) ).to.be.empty;
 		} );
 
+		describe( '#984', () => {
+			it( 'does not return the last block if none of its content is selected', () => {
+				setData( doc, '<p>[a</p><p>b</p><p>]c</p>' );
+
+				expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'a', 'b' ] );
+			} );
+
+			it( 'returns only the first block for a non collapsed selection if only end of selection is touching a block', () => {
+				setData( doc, '<p>a</p><h>b[</h><p>]c</p><p>d</p>' );
+
+				expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'b' ] );
+			} );
+
+			it( 'does not return the last block if none of its content is selected (nested case)', () => {
+				setData( doc, '<p>[a</p><nestedBlock><nestedBlock>]b</nestedBlock></nestedBlock>' );
+
+				expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'a' ] );
+			} );
+
+			// Like a super edge case, we can live with this behavior as I don't even know what we could expect here
+			// since only the innermost block is considerd a block to return (so the <nB>b...</nB> needs to be ignored).
+			it( 'does not return the last block if none of its content is selected (nested case, wrapper with a content)', () => {
+				setData( doc, '<p>[a</p><nestedBlock>b<nestedBlock>]c</nestedBlock></nestedBlock>' );
+
+				expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'a' ] );
+			} );
+
+			it( 'returns the last block if at least one of its child nodes is selected', () => {
+				setData( doc, '<p>[a</p><p>b</p><p><image></image>]c</p>' );
+
+				expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'a', 'b', 'c' ] );
+			} );
+
+			// I needed these last 2 cases to justify the use of isTouching() instead of simple `offset == 0` check.
+			it( 'returns the last block if at least one of its child nodes is selected (end in an inline element)', () => {
+				setData( doc, '<p>[a</p><p>b</p><p><image>x]</image>c</p>' );
+
+				expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'a', 'b', 'c' ] );
+			} );
+
+			it(
+				'does not return the last block if at least one of its child nodes is selected ' +
+				'(end in an inline element, no content selected)',
+				() => {
+					setData( doc, '<p>[a</p><p>b</p><p><image>]x</image>c</p>' );
+
+					expect( toText( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'a', 'b' ] );
+				}
+			);
+		} );
+
+		// Map all elements to data of its first child text node.
 		function toText( elements ) {
-			return Array.from( elements ).map( el => el.getChild( 0 ).data );
+			return Array.from( elements ).map( el => {
+				return Array.from( el.getChildren() ).find( child => child.data ).data;
+			} );
 		}
 	} );