Skip to content

Commit

Permalink
Merge pull request #7435 from ckeditor/i/7431
Browse files Browse the repository at this point in the history 
Internal (engine): Markers should not be considered a meaningful content in the  `Model#deleteContent()` context. Closes #7431.

Other (engine): Added the `ignoreMarkers` option to the `Model#hasContent()` method.
  • Loading branch information
@oleq
oleq authored Jun 16, 2020
2 parents 599040f + 9a5f66a commit 61a6110
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 8 deletions.
15 changes: 9 additions & 6 deletions packages/ckeditor5-engine/src/model/model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -551,24 +551,27 @@ export default class Model {
* @param {module:engine/model/range~Range|module:engine/model/element~Element} rangeOrElement Range or element to check.
* @param {Object} [options]
* @param {Boolean} [options.ignoreWhitespaces] Whether text node with whitespaces only should be considered empty.
* @param {Boolean} [options.ignoreMarkers] Whether markers should be ignored.
* @returns {Boolean}
*/
hasContent( rangeOrElement, options ) {
hasContent( rangeOrElement, options = {} ) {
const range = rangeOrElement instanceof ModelElement ? ModelRange._createIn( rangeOrElement ) : rangeOrElement;

if ( range.isCollapsed ) {
return false;
}

const { ignoreWhitespaces = false, ignoreMarkers = false } = options;

// Check if there are any markers which affects data in this given range.
for ( const intersectingMarker of this.markers.getMarkersIntersectingRange( range ) ) {
if ( intersectingMarker.affectsData ) {
return true;
if ( !ignoreMarkers ) {
for ( const intersectingMarker of this.markers.getMarkersIntersectingRange( range ) ) {
if ( intersectingMarker.affectsData ) {
return true;
}
}
}

const { ignoreWhitespaces = false } = options || {};

for ( const item of range.getItems() ) {
if ( item.is( 'textProxy' ) ) {
if ( !ignoreWhitespaces ) {
Expand Down
4 changes: 2 additions & 2 deletions packages/ckeditor5-engine/src/model/utils/deletecontent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ function getLivePositionsForSelectedBlocks( range ) {

// If the end of selection is at the start position of last block in the selection, then
// shrink it to not include that trailing block. Note that this should happen only for not empty selection.
if ( model.hasContent( range ) ) {
if ( model.hasContent( range, { ignoreMarkers: true } ) ) {
const endBlock = getParentBlock( endPosition );

if ( endBlock && endPosition.isTouching( model.createPositionAt( endBlock, 0 ) ) ) {
Expand Down Expand Up @@ -213,7 +213,7 @@ function mergeBranches( writer, startPosition, endPosition ) {
// Merging should not go deeper than common ancestor.
const [ startAncestor, endAncestor ] = getAncestorsJustBelowCommonAncestor( startPosition, endPosition );

if ( !model.hasContent( startAncestor ) && model.hasContent( endAncestor ) ) {
if ( !model.hasContent( startAncestor, { ignoreMarkers: true } ) && model.hasContent( endAncestor, { ignoreMarkers: true } ) ) {
mergeBranchesRight( writer, startPosition, endPosition, startAncestor.parent );
} else {
mergeBranchesLeft( writer, startPosition, endPosition, startAncestor.parent );
Expand Down
13 changes: 13 additions & 0 deletions packages/ckeditor5-engine/tests/model/model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,8 @@ describe( 'Model', () => {

expect( model.hasContent( pEmpty ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreWhitespaces: true } ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreMarkers: true } ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreMarkers: true, ignoreWhitespaces: true } ) ).to.be.false;
} );

it( 'should return false for empty element with marker (usingOperation=true, affectsData=false)', () => {
Expand All @@ -672,6 +674,8 @@ describe( 'Model', () => {

expect( model.hasContent( pEmpty ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreWhitespaces: true } ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreMarkers: true } ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreMarkers: true, ignoreWhitespaces: true } ) ).to.be.false;
} );

it( 'should return false (ignoreWhitespaces) for empty text with marker (usingOperation=false, affectsData=false)', () => {
Expand All @@ -688,6 +692,7 @@ describe( 'Model', () => {
} );

expect( model.hasContent( pEmpty, { ignoreWhitespaces: true } ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreWhitespaces: true, ignoreMarkers: true } ) ).to.be.false;
} );

it( 'should return true for empty text with marker (usingOperation=false, affectsData=false)', () => {
Expand All @@ -704,6 +709,8 @@ describe( 'Model', () => {
} );

expect( model.hasContent( pEmpty ) ).to.be.true;
expect( model.hasContent( pEmpty, { ignoreMarkers: true } ) ).to.be.true;
expect( model.hasContent( pEmpty, { ignoreMarkers: true, ignoreWhitespaces: true } ) ).to.be.false;
} );

it( 'should return false for empty element with marker (usingOperation=false, affectsData=true)', () => {
Expand All @@ -717,6 +724,8 @@ describe( 'Model', () => {

expect( model.hasContent( pEmpty ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreWhitespaces: true } ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreMarkers: true } ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreMarkers: true, ignoreWhitespaces: true } ) ).to.be.false;
} );

it( 'should return false for empty element with marker (usingOperation=true, affectsData=true)', () => {
Expand All @@ -730,6 +739,8 @@ describe( 'Model', () => {

expect( model.hasContent( pEmpty ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreWhitespaces: true } ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreMarkers: true } ) ).to.be.false;
expect( model.hasContent( pEmpty, { ignoreMarkers: true, ignoreWhitespaces: true } ) ).to.be.false;
} );

it( 'should return true (ignoreWhitespaces) for empty text with marker (usingOperation=false, affectsData=true)', () => {
Expand All @@ -747,6 +758,8 @@ describe( 'Model', () => {

expect( model.hasContent( pEmpty ) ).to.be.true;
expect( model.hasContent( pEmpty, { ignoreWhitespaces: true } ) ).to.be.true;
expect( model.hasContent( pEmpty, { ignoreMarkers: true } ) ).to.be.true;
expect( model.hasContent( pEmpty, { ignoreMarkers: true, ignoreWhitespaces: true } ) ).to.be.false;
} );
} );

Expand Down
53 changes: 53 additions & 0 deletions packages/ckeditor5-engine/tests/model/utils/deletecontent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,59 @@ describe( 'DataController utils', () => {
);
} );

describe( 'with markers', () => {
it( 'should merge left if the first element is not empty', () => {
setData( model, '<heading1>foo[</heading1><paragraph>]bar</paragraph>' );

model.enqueueChange( 'transparent', writer => {
const root = doc.getRoot( );
const range = writer.createRange(
writer.createPositionFromPath( root, [ 0, 3 ] ),
writer.createPositionFromPath( root, [ 1, 0 ] )
);
writer.addMarker( 'comment1', { range, usingOperation: true, affectsData: true } );
} );

deleteContent( model, doc.selection );

expect( getData( model ) ).to.equal( '<heading1>foo[]bar</heading1>' );
} );

it( 'should merge right if the first element is empty', () => {
setData( model, '<heading1>[</heading1><paragraph>]bar</paragraph>' );

model.enqueueChange( 'transparent', writer => {
const root = doc.getRoot( );
const range = writer.createRange(
writer.createPositionFromPath( root, [ 0, 0 ] ),
writer.createPositionFromPath( root, [ 1, 0 ] )
);
writer.addMarker( 'comment1', { range, usingOperation: true, affectsData: true } );
} );

deleteContent( model, doc.selection );

expect( getData( model ) ).to.equal( '<paragraph>[]bar</paragraph>' );
} );

it( 'should merge left if the last element is empty', () => {
setData( model, '<heading1>foo[</heading1><paragraph>]</paragraph>' );

model.enqueueChange( 'transparent', writer => {
const root = doc.getRoot( );
const range = writer.createRange(
writer.createPositionFromPath( root, [ 0, 3 ] ),
writer.createPositionFromPath( root, [ 1, 0 ] )
);
writer.addMarker( 'comment1', { range, usingOperation: true, affectsData: true } );
} );

deleteContent( model, doc.selection );

expect( getData( model ) ).to.equal( '<heading1>foo[]</heading1>' );
} );
} );

describe( 'filtering out', () => {
beforeEach( () => {
const schema = model.schema;
Expand Down

0 comments on commit 61a6110

Please sign in to comment.