Merge pull request #1770 from ckeditor/t/ckeditor5-widget/95

Other: Remove the `Selection#getTopMostBlocks()` method. Closes ckeditor/ckeditor5-widget#95. Closes ckeditor/ckeditor5-table#199. BREAKING CHANGE: The `Selection#getTopMostBlocks()` was removed from the public API. Use `Selection#getSelectedBlocks()` instead. BREAKING CHANGE: The `Selection#getSelectedBlocks()` does not return blocks nested in other blocks now.
ckeditor · Sep 3, 2019 · 7970f17 · 7970f17
2 parents 2210696 + e1f7e8d
commit 7970f17
Show file tree

Hide file tree

Showing 4 changed files with 184 additions and 198 deletions.
diff --git a/src/model/documentselection.js b/src/model/documentselection.js
@@ -233,41 +233,26 @@ export default class DocumentSelection {
 	}
 
 	/**
-	 * Gets elements of type "block" touched by the selection.
+	 * Gets elements of type {@link module:engine/model/schema~Schema#isBlock "block"} touched by the 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()` returns blocks that are nested in other non-block elements
+	 * but will not return blocks nested in other blocks.
 	 *
-	 * In this case the function will return exactly all 3 paragraphs:
+	 * In this case the function will return exactly all 3 paragraphs (note: `<blockQuote>` is not a block itself):
 	 *
 	 *		<paragraph>[a</paragraph>
-	 *		<quote>
+	 *		<blockQuote>
 	 *			<paragraph>b</paragraph>
-	 *		</quote>
+	 *		</blockQuote>
 	 *		<paragraph>c]d</paragraph>
 	 *
 	 * In this case the paragraph will also be returned, despite the collapsed 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 {Iterable.<module:engine/model/element~Element>}
-	 */
-	getSelectedBlocks() {
-		return this._selection.getSelectedBlocks();
-	}
-
-	/**
-	 * Returns blocks that aren't nested in other selected blocks.
-	 *
-	 * In this case the method will return blocks A, B and E because C & D are children of block B:
+	 * In such a scenario, however, only blocks A, B & E will be returned as blocks C & D are nested in block B:
 	 *
 	 *		[<blockA></blockA>
 	 *		<blockB>
@@ -276,12 +261,24 @@ export default class DocumentSelection {
 	 *		</blockB>
 	 *		<blockE></blockE>]
 	 *
-	 * **Note:** To get all selected blocks use {@link #getSelectedBlocks `getSelectedBlocks()`}.
+	 * If the selection is inside a block all the inner blocks (A & B) are returned:
+	 *
+	 * 		<block>
+	 *			<blockA>[a</blockA>
+	 * 			<blockB>b]</blockB>
+	 * 		</block>
+	 *
+	 * **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 {Iterable.<module:engine/model/element~Element>}
 	 */
-	getTopMostBlocks() {
-		return this._selection.getTopMostBlocks();
+	getSelectedBlocks() {
+		return this._selection.getSelectedBlocks();
 	}
 
 	/**

diff --git a/src/model/selection.js b/src/model/selection.js
@@ -641,24 +641,41 @@ export default class Selection {
 	}
 
 	/**
-	 * Gets elements of type "block" touched by the selection.
+	 * Gets elements of type {@link module:engine/model/schema~Schema#isBlock "block"} touched by the 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()` returns blocks that are nested in other non-block elements
+	 * but will not return blocks nested in other blocks.
 	 *
-	 * In this case the function will return exactly all 3 paragraphs:
+	 * In this case the function will return exactly all 3 paragraphs (note: `<blockQuote>` is not a block itself):
 	 *
 	 *		<paragraph>[a</paragraph>
-	 *		<quote>
+	 *		<blockQuote>
 	 *			<paragraph>b</paragraph>
-	 *		</quote>
+	 *		</blockQuote>
 	 *		<paragraph>c]d</paragraph>
 	 *
 	 * In this case the paragraph will also be returned, despite the collapsed selection:
 	 *
 	 *		<paragraph>[]a</paragraph>
 	 *
+	 * In such a scenario, however, only blocks A, B & E will be returned as blocks C & D are nested in block B:
+	 *
+	 *		[<blockA></blockA>
+	 *		<blockB>
+	 *			<blockC></blockC>
+	 *			<blockD></blockD>
+	 *		</blockB>
+	 *		<blockE></blockE>]
+	 *
+	 * If the selection is inside a block all the inner blocks (A & B) are returned:
+	 *
+	 * 		<block>
+	 *			<blockA>[a</blockA>
+	 * 			<blockB>b]</blockB>
+	 * 		</block>
+	 *
 	 * **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.
 	 *
@@ -672,56 +689,30 @@ export default class Selection {
 		const visited = new WeakSet();
 
 		for ( const range of this.getRanges() ) {
+			// Get start block of range in case of a collapsed range.
 			const startBlock = getParentBlock( range.start, visited );
 
-			if ( startBlock ) {
+			if ( startBlock && isTopBlockInRange( startBlock, range ) ) {
 				yield startBlock;
 			}
 
 			for ( const value of range.getWalker() ) {
-				if ( value.type == 'elementEnd' && isUnvisitedBlockContainer( value.item, visited ) ) {
-					yield value.item;
+				const block = value.item;
+
+				if ( value.type == 'elementEnd' && isUnvisitedTopBlock( block, visited, range ) ) {
+					yield block;
 				}
 			}
 
 			const endBlock = getParentBlock( range.end, visited );
 
 			// #984. Don't return the end block if the range ends right at its beginning.
-			if ( endBlock && !range.end.isTouching( Position._createAt( endBlock, 0 ) ) ) {
+			if ( endBlock && !range.end.isTouching( Position._createAt( endBlock, 0 ) ) && isTopBlockInRange( endBlock, range ) ) {
 				yield endBlock;
 			}
 		}
 	}
 
-	/**
-	 * Returns blocks that aren't nested in other selected blocks.
-	 *
-	 * In this case the method will return blocks A, B and E because C & D are children of block B:
-	 *
-	 *		[<blockA></blockA>
-	 *		<blockB>
-	 *			<blockC></blockC>
-	 *			<blockD></blockD>
-	 *		</blockB>
-	 *		<blockE></blockE>]
-	 *
-	 * **Note:** To get all selected blocks use {@link #getSelectedBlocks `getSelectedBlocks()`}.
-	 *
-	 * @returns {Iterable.<module:engine/model/element~Element>}
-	 */
-	* getTopMostBlocks() {
-		const selected = Array.from( this.getSelectedBlocks() );
-
-		for ( const block of selected ) {
-			const parentBlock = findAncestorBlock( block );
-
-			// Filter out blocks that are nested in other selected blocks (like paragraphs in tables).
-			if ( !parentBlock || !selected.includes( parentBlock ) ) {
-				yield block;
-			}
-		}
-	}
-
 	/**
 	 * Checks whether the selection contains the entire content of the given element. This means that selection must start
 	 * at a position {@link module:engine/model/position~Position#isTouching touching} the element's start and ends at position
@@ -831,7 +822,7 @@ mix( Selection, EmitterMixin );
 
 // Checks whether the given element extends $block in the schema and has a parent (is not a root).
 // Marks it as already visited.
-function isUnvisitedBlockContainer( element, visited ) {
+function isUnvisitedBlock( element, visited ) {
 	if ( visited.has( element ) ) {
 		return false;
 	}
@@ -841,6 +832,11 @@ function isUnvisitedBlockContainer( element, visited ) {
 	return element.document.model.schema.isBlock( element ) && element.parent;
 }
 
+// Checks if the given element is a $block was not previously visited and is a top block in a range.
+function isUnvisitedTopBlock( element, visited, range ) {
+	return isUnvisitedBlock( element, visited ) && isTopBlockInRange( element, range );
+}
+
 // 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.
@@ -859,7 +855,7 @@ function getParentBlock( position, visited ) {
 
 		hasParentLimit = schema.isLimit( element );
 
-		return !hasParentLimit && isUnvisitedBlockContainer( element, visited );
+		return !hasParentLimit && isUnvisitedBlock( element, visited );
 	} );
 
 	// Mark all ancestors of this position's parent, because find() might've stopped early and
@@ -869,6 +865,23 @@ function getParentBlock( position, visited ) {
 	return block;
 }
 
+// Checks if the blocks is not nested in other block inside a range.
+//
+// @param {module:engine/model/elmenent~Element} block Block to check.
+// @param {module:engine/model/range~Range} range Range to check.
+function isTopBlockInRange( block, range ) {
+	const parentBlock = findAncestorBlock( block );
+
+	if ( !parentBlock ) {
+		return true;
+	}
+
+	// Add loose flag to check as parentRange can be equal to range.
+	const isParentInRange = range.containsRange( Range._createOn( parentBlock ), true );
+
+	return !isParentInRange;
+}
+
 // Returns first ancestor block of a node.
 //
 // @param {module:engine/model/node~Node} node

diff --git a/src/view/downcastwriter.js b/src/view/downcastwriter.js
@@ -788,26 +788,26 @@ export default class DowncastWriter {
 	}
 
 	/**
-     * Wraps elements within range with provided {@link module:engine/view/attributeelement~AttributeElement AttributeElement}.
-     * If a collapsed range is provided, it will be wrapped only if it is equal to view selection.
-     *
-     * If a collapsed range was passed and is same as selection, the selection
-     * will be moved to the inside of the wrapped attribute element.
-     *
-     * Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-invalid-range-container`
-     * when {@link module:engine/view/range~Range#start}
-     * and {@link module:engine/view/range~Range#end} positions are not placed inside same parent container.
-     *
-     * Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-wrap-invalid-attribute` when passed attribute element is not
-     * an instance of {@link module:engine/view/attributeelement~AttributeElement AttributeElement}.
-     *
-     * Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-wrap-nonselection-collapsed-range` when passed range
-     * is collapsed and different than view selection.
-     *
-     * @param {module:engine/view/range~Range} range Range to wrap.
-     * @param {module:engine/view/attributeelement~AttributeElement} attribute Attribute element to use as wrapper.
-     * @returns {module:engine/view/range~Range} range Range after wrapping, spanning over wrapping attribute element.
-    */
+	 * Wraps elements within range with provided {@link module:engine/view/attributeelement~AttributeElement AttributeElement}.
+	 * If a collapsed range is provided, it will be wrapped only if it is equal to view selection.
+	 *
+	 * If a collapsed range was passed and is same as selection, the selection
+	 * will be moved to the inside of the wrapped attribute element.
+	 *
+	 * Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-invalid-range-container`
+	 * when {@link module:engine/view/range~Range#start}
+	 * and {@link module:engine/view/range~Range#end} positions are not placed inside same parent container.
+	 *
+	 * Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-wrap-invalid-attribute` when passed attribute element is not
+	 * an instance of {@link module:engine/view/attributeelement~AttributeElement AttributeElement}.
+	 *
+	 * Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-wrap-nonselection-collapsed-range` when passed range
+	 * is collapsed and different than view selection.
+	 *
+	 * @param {module:engine/view/range~Range} range Range to wrap.
+	 * @param {module:engine/view/attributeelement~AttributeElement} attribute Attribute element to use as wrapper.
+	 * @returns {module:engine/view/range~Range} range Range after wrapping, spanning over wrapping attribute element.
+	*/
 	wrap( range, attribute ) {
 		if ( !( attribute instanceof AttributeElement ) ) {
 			throw new CKEditorError( 'view-writer-wrap-invalid-attribute', this.document );