Merge pull request #8518 from ckeditor/i/7978

Fix (engine): The select all command should include all selectable elements in the content. Closes #7978.
ckeditor · Nov 24, 2020 · e9d7d17 · e9d7d17
2 parents 446f026 + b6b9458
commit e9d7d17
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 1 deletion.
diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js
@@ -213,6 +213,18 @@ function mergeBranches( writer, startPosition, endPosition ) {
 	// Merging should not go deeper than common ancestor.
 	const [ startAncestor, endAncestor ] = getAncestorsJustBelowCommonAncestor( startPosition, endPosition );
 
+	// Branches can't be merged if one of the positions is directly inside a common ancestor.
+	//
+	// Example:
+	//     <blockQuote>
+	//         <paragraph>[foo</paragraph>]
+	//         <table> ... </table>
+	//     <blockQuote>
+	//
+	if ( !startAncestor || !endAncestor ) {
+		return;
+	}
+
 	if ( !model.hasContent( startAncestor, { ignoreMarkers: true } ) && model.hasContent( endAncestor, { ignoreMarkers: true } ) ) {
 		mergeBranchesRight( writer, startPosition, endPosition, startAncestor.parent );
 	} else {

diff --git a/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js b/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js
@@ -185,7 +185,7 @@ function tryFixingNonCollapsedRage( range, schema ) {
 
 			// The schema.getNearestSelectionRange might return null - if that happens use original position.
 			const rangeStart = fixedStart ? fixedStart.start : start;
-			const rangeEnd = fixedEnd ? fixedEnd.start : end;
+			const rangeEnd = fixedEnd ? fixedEnd.end : end;
 
 			return new Range( rangeStart, rangeEnd );
 		}

diff --git a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js
@@ -293,6 +293,18 @@ describe( 'DataController utils', () => {
 				'<paragraph>x</paragraph><paragraph>[]ar</paragraph><paragraph>y</paragraph>'
 			);
 
+			test(
+				'should not merge if the end position of the selection is directly in a common ancestor',
+				'<paragraph><pchild>[foo</pchild>]<widget></widget></paragraph>',
+				'<paragraph><pchild>[]</pchild><widget></widget></paragraph>'
+			);
+
+			test(
+				'should not merge if the start position of the selection is directly in a common ancestor',
+				'<paragraph><widget></widget>[<pchild>foo]</pchild></paragraph>',
+				'<paragraph><widget></widget>[]<pchild></pchild></paragraph>'
+			);
+
 			// Note: in all these cases we ignore the direction of merge.
 			// If https://github.com/ckeditor/ckeditor5-engine/issues/470 was fixed we could differently treat
 			// forward and backward delete.

diff --git a/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js b/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js
@@ -866,6 +866,35 @@ describe( 'Selection post-fixer', () => {
 
 				expect( model.document.selection.hasAttribute( 'foo' ) ).to.be.true;
 			} );
+
+			it( 'should include a selectable object at the end of the selection', () => {
+				model.schema.register( 'blockQuote', {
+					allowWhere: '$block',
+					allowContentOf: '$root'
+				} );
+
+				setModelData( model,
+					'[<blockQuote>' +
+						'<paragraph>foo</paragraph>' +
+						'<table>' +
+							'<tableRow>' +
+								'<tableCell><paragraph>bar</paragraph></tableCell>' +
+							'</tableRow>' +
+						'</table>' +
+					'</blockQuote>]'
+				);
+
+				assertEqualMarkup( getModelData( model ),
+					'<blockQuote>' +
+						'<paragraph>[foo</paragraph>' +
+						'<table>' +
+							'<tableRow>' +
+								'<tableCell><paragraph>bar</paragraph></tableCell>' +
+							'</tableRow>' +
+						'</table>]' +
+					'</blockQuote>'
+				);
+			} );
 		} );
 
 		describe( 'non-collapsed selection - image scenarios', () => {