Skip to content

Commit

Permalink
Merge pull request #13822 from ckeditor/ck/11585-fix-range-selection
Browse files Browse the repository at this point in the history
Fix (engine): `Selection#getSelectedBlocks()` should ignore trailing blocks where no content is selected. The selection of such blocks is not visible to the content author and is usually there unintentionally. Closes #11585.
  • Loading branch information
arkflpc authored Apr 11, 2023
2 parents 221ba92 + 2b7bc95 commit 9d9604b
Show file tree
Hide file tree
Showing 9 changed files with 328 additions and 86 deletions.
85 changes: 79 additions & 6 deletions packages/ckeditor5-engine/src/model/selection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -637,13 +637,23 @@ export default class Selection extends EmitterMixin( TypeCheckable ) {
* </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.
* **Special case**: Selection ignores first and/or last blocks if nothing (from user perspective) is selected in them.
*
* ```xml
* // Selection ends and the beginning of the last block.
* <paragraph>[a</paragraph>
* <paragraph>b</paragraph>
* <paragraph>]c</paragraph> // this block will not be returned
* <paragraph>]c</paragraph> // This block will not be returned
*
* // Selection begins at the end of the first block.
* <paragraph>a[</paragraph> // This block will not be returned
* <paragraph>b</paragraph>
* <paragraph>c]</paragraph>
*
* // Selection begings at the end of the first block and ends at the beginning of the last block.
* <paragraph>a[</paragraph> // This block will not be returned
* <paragraph>b</paragraph>
* <paragraph>]c</paragraph> // This block will not be returned
* ```
*/
public* getSelectedBlocks(): IterableIterator<Element> {
Expand All @@ -653,7 +663,7 @@ export default class Selection extends EmitterMixin( TypeCheckable ) {
// Get start block of range in case of a collapsed range.
const startBlock = getParentBlock( range.start, visited );

if ( startBlock && isTopBlockInRange( startBlock, range ) ) {
if ( isStartBlockSelected( startBlock, range ) ) {
yield startBlock as any;
}

Expand All @@ -667,8 +677,7 @@ export default class Selection extends EmitterMixin( TypeCheckable ) {

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 ) ) && isTopBlockInRange( endBlock, range ) ) {
if ( isEndBlockSelected( endBlock, range ) ) {
yield endBlock as any;
}
}
Expand Down Expand Up @@ -876,6 +885,70 @@ function isTopBlockInRange( block: Node, range: Range ) {
return !isParentInRange;
}

/**
* If a selection starts at the end of a block, that block is not returned as from user perspective this block wasn't selected.
* See [#11585](https://github.com/ckeditor/ckeditor5/issues/11585) for more details.
*
* ```xml
* <paragraph>a[</paragraph> // This block will not be returned
* <paragraph>b</paragraph>
* <paragraph>c]</paragraph>
* ```
*
* Collapsed selection is not affected by it:
*
* ```xml
* <paragraph>a[]</paragraph> // This block will be returned
* ```
*/
function isStartBlockSelected( startBlock: Element | undefined, range: Range ): boolean {
if ( !startBlock ) {
return false;
}

if ( range.isCollapsed || startBlock.isEmpty ) {
return true;
}

if ( range.start.isTouching( Position._createAt( startBlock, startBlock.maxOffset ) ) ) {
return false;
}

return isTopBlockInRange( startBlock, range );
}

/**
* 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.
*
* ```xml
* <paragraph>[a</paragraph>
* <paragraph>b</paragraph>
* <paragraph>]c</paragraph> // this block will not be returned
* ```
*
* Collapsed selection is not affected by it:
*
* ```xml
* <paragraph>[]a</paragraph> // this block will be returned
* ```
*/
function isEndBlockSelected( endBlock: Element | undefined, range: Range ): boolean {
if ( !endBlock ) {
return false;
}

if ( range.isCollapsed || endBlock.isEmpty ) {
return true;
}

if ( range.end.isTouching( Position._createAt( endBlock, 0 ) ) ) {
return false;
}

return isTopBlockInRange( endBlock, range );
}

/**
* Returns first ancestor block of a node.
*/
Expand Down
22 changes: 22 additions & 0 deletions packages/ckeditor5-engine/tests/model/_utils/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,28 @@ export function getText( element ) {
return text;
}

/**
* Maps all elements to names. If element contains child text node it will be appended to name with '#'.
*
* @param {Array.<engine.model.Element>} element Array of Element from which text will be returned.
* @returns {String} Text contents of the element.
*/
export function stringifyBlocks( elements ) {
return Array.from( elements ).map( el => {
const name = el.name;

let innerText = '';

for ( const child of el.getChildren() ) {
if ( child.is( '$text' ) ) {
innerText += child.data;
}
}

return innerText.length ? `${ name }#${ innerText }` : name;
} );
}

/**
* Creates a range on given {@link engine.model.Element element} only. The range starts directly before that element
* and ends before the first child of that element.
Expand Down
74 changes: 4 additions & 70 deletions packages/ckeditor5-engine/tests/model/selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { parse, setData } from '../../src/dev-utils/model';
import Schema from '../../src/model/schema';
import { expectToThrowCKEditorError } from '@ckeditor/ckeditor5-utils/tests/_utils/utils';

import { stringifyBlocks } from './_utils/utils';

describe( 'Selection', () => {
let model, doc, root, selection, liveRange, range, range1, range2, range3;

Expand Down Expand Up @@ -1001,10 +1003,10 @@ describe( 'Selection', () => {
expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'h#b', 'p#c' ] );
} );

it( 'returns two blocks for a non collapsed selection (starts at block end)', () => {
it( 'returns one block for a non collapsed selection (starts at block end)', () => {
setData( model, '<p>a</p><h>b[</h><p>c]</p><p>d</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'h#b', 'p#c' ] );
expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#c' ] );
} );

it( 'returns proper block for a multi-range selection', () => {
Expand Down Expand Up @@ -1131,57 +1133,6 @@ describe( 'Selection', () => {

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'blk', 'p#bar' ] );
} );

describe( '#984', () => {
it( 'does not return the last block if none of its content is selected', () => {
setData( model, '<p>[a</p><p>b</p><p>]c</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#a', 'p#b' ] );
} );

it( 'returns only the first block for a non collapsed selection if only end of selection is touching a block', () => {
setData( model, '<p>a</p><h>b[</h><p>]c</p><p>d</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'h#b' ] );
} );

it( 'does not return the last block if none of its content is selected (nested case)', () => {
setData( model, '<p>[a</p><nestedBlock><nestedBlock>]b</nestedBlock></nestedBlock>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#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( model, '<p>[a</p><nestedBlock>b<nestedBlock>]c</nestedBlock></nestedBlock>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#a' ] );
} );

it( 'returns the last block if at least one of its child nodes is selected', () => {
setData( model, '<p>[a</p><p>b</p><p><imageBlock></imageBlock>]c</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#a', 'p#b', 'p#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( model, '<p>[a</p><p>b</p><p><imageBlock>x]</imageBlock>c</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#a', 'p#b', 'p#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( model, '<p>[a</p><p>b</p><p><imageBlock>]x</imageBlock>c</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#a', 'p#b' ] );
}
);
} );
} );

describe( 'attributes interface', () => {
Expand Down Expand Up @@ -1357,21 +1308,4 @@ 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;

let innerText = '';

for ( const child of el.getChildren() ) {
if ( child.is( '$text' ) ) {
innerText += child.data;
}
}

return innerText.length ? `${ name }#${ innerText }` : name;
} );
}
} );
100 changes: 100 additions & 0 deletions packages/ckeditor5-engine/tests/tickets/11585.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/**
* @license Copyright (c) 2003-2023, CKSource Holding sp. z o.o. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

import Model from '../../src/model/model';
import Element from '../../src/model/element';
import Text from '../../src/model/text';
import Position from '../../src/model/position';
import LiveRange from '../../src/model/liverange';
import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
import { setData } from '../../src/dev-utils/model';

import { stringifyBlocks } from '../model/_utils/utils';

describe( '#11585', () => {
let model, doc, root, liveRange;

testUtils.createSinonSandbox();

beforeEach( () => {
model = new Model();
doc = model.document;
root = doc.createRoot();
root._appendChild( [
new Element( 'p' ),
new Element( 'p' ),
new Element( 'p', [], new Text( 'foobar' ) ),
new Element( 'p' ),
new Element( 'p' ),
new Element( 'p' ),
new Element( 'p', [], new Text( 'foobar' ) )
] );

liveRange = new LiveRange( new Position( root, [ 0 ] ), new Position( root, [ 1 ] ) );

model.schema.register( 'p', { inheritAllFrom: '$block' } );
model.schema.register( 'h', { inheritAllFrom: '$block' } );

model.schema.register( 'blockquote' );
model.schema.extend( 'blockquote', { allowIn: '$root' } );
model.schema.extend( '$block', { allowIn: 'blockquote' } );

model.schema.register( 'imageBlock', {
allowIn: [ '$root', '$block' ],
allowChildren: '$text'
} );

// Special block which can contain another blocks.
model.schema.register( 'nestedBlock', { inheritAllFrom: '$block' } );
model.schema.extend( 'nestedBlock', { allowIn: '$block' } );

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', isLimit: true, isSelectable: true } );

model.schema.extend( 'p', { allowIn: 'tableCell' } );
} );

afterEach( () => {
model.destroy();
liveRange.detach();
} );

it( 'does not return the first block if none of its contents is selected', () => {
setData( model, '<p>a[</p><p>b</p><p>c]</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#b', 'p#c' ] );
} );

it( 'returns the first block if at least one of its child nodes is selected', () => {
setData( model, '<p>a[<imageBlock></imageBlock></p><p>b</p><p>c]</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#a', 'p#b', 'p#c' ] );
} );

it( 'returns the block if it has a collapsed selection at the beginning', () => {
setData( model, '<p>[]a</p><p>b</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#a' ] );
} );

it( 'returns the block if it has a collapsed selection at the end', () => {
setData( model, '<p>a[]</p><p>b</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p#a' ] );
} );

it( 'does not return first and last blocks if no content is selected', () => {
setData( model, '<p>a[</p><p>]b</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [] );
} );

it( 'returns the first and last blocks if no content is selected but both blocks are empty', () => {
setData( model, '<p>[</p><p>]</p>' );

expect( stringifyBlocks( doc.selection.getSelectedBlocks() ) ).to.deep.equal( [ 'p', 'p' ] );
} );
} );
Loading

0 comments on commit 9d9604b

Please sign in to comment.