Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/1012: Handles with a paragraph when the entire content was removed #1022

Merged
merged 5 commits into from
Jul 19, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 66 additions & 5 deletions src/controller/deletecontent.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,15 @@ export default function deleteContent( selection, batch, options = {} ) {
return;
}

const selRange = selection.getFirstRange();
// 0. Replace the entire content with paragraph.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to fix the numbering.

// See: https://github.com/ckeditor/ckeditor5-engine/issues/1012#issuecomment-315017594.
if ( shouldEntireContentBeReplacedWithParagraph( batch.document.schema, selection ) ) {
replaceEntireContentWithParagraph( batch, selection );

return;
}

const selRange = selection.getFirstRange();
const startPos = selRange.start;
const endPos = LivePosition.createFromPosition( selRange.end );

Expand All @@ -59,10 +66,7 @@ export default function deleteContent( selection, batch, options = {} ) {
// 3. Autoparagraphing.
// Check if a text is allowed in the new container. If not, try to create a new paragraph (if it's allowed here).
if ( shouldAutoparagraph( batch.document, startPos ) ) {
const paragraph = new Element( 'paragraph' );
batch.insert( startPos, paragraph );

selection.collapse( paragraph );
insertParagraph( batch, startPos, selection );
}

endPos.detach();
Expand Down Expand Up @@ -163,3 +167,60 @@ function checkCanBeMerged( leftPos, rightPos ) {

return true;
}

// Returns the lowest limit element defined in `Schema.limits` for passed selection.
function getLimitElement( schema, selection ) {
let element = selection.getFirstRange().getCommonAncestor();

while ( !schema.limits.has( element.name ) ) {
if ( element.parent ) {
element = element.parent;
} else {
break;
}
}

return element;
}

function insertParagraph( batch, position, selection ) {
const paragraph = new Element( 'paragraph' );
batch.insert( position, paragraph );

selection.collapse( paragraph );
}

function replaceEntireContentWithParagraph( batch, selection ) {
const limitElement = getLimitElement( batch.document.schema, selection );

batch.remove( Range.createIn( limitElement ) );
insertParagraph( batch, Position.createAt( limitElement ), selection );
}

// We want to replace the entire content with a paragraph when:
// * the entire content is selected,
// * selection contains at least two elements,
// * whether the paragraph is allowed in schema in the common ancestor.
function shouldEntireContentBeReplacedWithParagraph( schema, selection ) {
const limitElement = getLimitElement( schema, selection );
const limitStartPosition = Position.createAt( limitElement );
const limitEndPosition = Position.createAt( limitElement, 'end' );

if ( !limitStartPosition.isTouching( selection.getFirstPosition() ) ||
!limitEndPosition.isTouching( selection.getLastPosition() )
) {
return false;
}

const range = selection.getFirstRange();

if ( range.start.parent == range.end.parent ) {
return false;
}

if ( !schema.check( { name: 'paragraph', inside: limitElement.name } ) ) {
return false;
}

return true;
}
2 changes: 2 additions & 0 deletions src/model/schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ export default class Schema {
this.allow( { name: '$block', inside: '$root' } );
this.allow( { name: '$inline', inside: '$block' } );

this.limits.add( '$root' );

// TMP!
// Create an "all allowed" context in the schema for processing the pasted content.
// Read: https://github.com/ckeditor/ckeditor5-engine/issues/638#issuecomment-255086588
Expand Down
92 changes: 86 additions & 6 deletions tests/controller/deletecontent.js
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,6 @@ describe( 'DataController', () => {
'<heading1>f[]</heading1><paragraph>x</paragraph>'
);

test(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to avoid deleting tests completely. This may be the only test which checks if the middle paragraph (foo) is deleted. So, either just adjust its result or add some content outside to not make it a full content selection (and adjust the title).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS. Note that you haven't added this test in the "should leave a paragraph if the entire content was selected" block.

'leaves just one element when all selected',
'<heading1>[x</heading1><paragraph>foo</paragraph><paragraph>y]</paragraph>',
'<heading1>[]</heading1>'
);

it( 'uses remove delta instead of merge delta if merged element is empty', () => {
setData( doc, '<paragraph>ab[cd</paragraph><paragraph>efgh]</paragraph>' );

Expand Down Expand Up @@ -450,6 +444,8 @@ describe( 'DataController', () => {

const schema = doc.schema;

schema.limits.add( 'restrictedRoot' );

schema.registerItem( 'image', '$inline' );
schema.registerItem( 'paragraph', '$block' );
schema.registerItem( 'heading1', '$block' );
Expand All @@ -465,6 +461,8 @@ describe( 'DataController', () => {
// See also "in simple scenarios => deletes an element".

it( 'deletes two inline elements', () => {
doc.schema.limits.add( 'paragraph' );

setData(
doc,
'x[<image></image><image></image>]z',
Expand Down Expand Up @@ -659,6 +657,88 @@ describe( 'DataController', () => {
);
} );

describe( 'should leave a paragraph if the entire content was selected', () => {
beforeEach( () => {
doc = new Document();
doc.createRoot();

const schema = doc.schema;

schema.registerItem( 'div', '$block' );
schema.limits.add( 'div' );

schema.registerItem( 'article', '$block' );
schema.limits.add( 'article' );

schema.registerItem( 'image', '$inline' );
schema.objects.add( 'image' );

schema.registerItem( 'paragraph', '$block' );
schema.registerItem( 'heading1', '$block' );
schema.registerItem( 'heading2', '$block' );

schema.allow( { name: '$text', inside: '$root' } );

schema.allow( { name: 'image', inside: '$root' } );
schema.allow( { name: 'image', inside: 'heading1' } );
schema.allow( { name: 'heading1', inside: 'div' } );
schema.allow( { name: 'paragraph', inside: 'div' } );
schema.allow( { name: 'heading1', inside: 'article' } );
schema.allow( { name: 'heading2', inside: 'article' } );
} );

test(
'but not if only one block was selected',
'<heading1>[xx]</heading1>',
'<heading1>[]</heading1>'
);

test(
'when the entire heading and paragraph were selected',
'<heading1>[xx</heading1><paragraph>yy]</paragraph>',
'<paragraph>[]</paragraph>'
);

test(
'inside the limit element when the entire heading and paragraph were inside',
'<div><heading1>[xx</heading1><paragraph>yy]</paragraph></div>',
'<div><paragraph>[]</paragraph></div>'
);

test(
'but not if schema does not accept paragraph in limit element',
'<article><heading1>[xx</heading1><heading2>yy]</heading2></article>',
'<article><heading1>[]</heading1></article>'
);

test(
'but not if selection is not containing the whole content',
'<image></image><heading1>[xx</heading1><paragraph>yy]</paragraph>',
'<image></image><heading1>[]</heading1>'
);

test(
'but not if only single element is selected',
'<heading1>[<image></image>xx]</heading1>',
'<heading1>[]</heading1>'
);

it( 'when root element was not added as Schema.limits works fine as well', () => {
doc.createRoot( 'paragraph', 'paragraphRoot' );

setData(
doc,
'x[<image></image><image></image>]z',
{ rootName: 'paragraphRoot' }
);

deleteContent( doc.selection, doc.batch() );

expect( getData( doc, { rootName: 'paragraphRoot' } ) )
.to.equal( 'x[]z' );
} );
} );

function test( title, input, output, options ) {
it( title, () => {
setData( doc, input );
Expand Down
4 changes: 4 additions & 0 deletions tests/model/schema/schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ describe( 'Schema', () => {
expect( schema.limits ).to.be.instanceOf( Set );
} );

it( 'should have defined the limits elements', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"should mark $root as a limit element"

expect( schema.limits.has( '$root' ) ).to.be.true;
} );

describe( '$clipboardHolder', () => {
it( 'should allow $block', () => {
expect( schema.check( { name: '$block', inside: [ '$clipboardHolder' ] } ) ).to.be.true;
Expand Down