From a3773f2655b8434aae3a3ee605ed85f6d17d6d9f Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Thu, 13 Jul 2017 11:26:58 +0200 Subject: [PATCH 1/5] Added "$root" element to "Schema.limits" in order to simplify limit checks. --- src/model/schema.js | 2 ++ tests/model/schema/schema.js | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/src/model/schema.js b/src/model/schema.js index 9bd71be99..2a0ec0d2e 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -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 diff --git a/tests/model/schema/schema.js b/tests/model/schema/schema.js index 32daae9e2..912f63bdf 100644 --- a/tests/model/schema/schema.js +++ b/tests/model/schema/schema.js @@ -49,6 +49,10 @@ describe( 'Schema', () => { expect( schema.limits ).to.be.instanceOf( Set ); } ); + it( 'should have defined the limits elements', () => { + expect( schema.limits.has( '$root' ) ).to.be.true; + } ); + describe( '$clipboardHolder', () => { it( 'should allow $block', () => { expect( schema.check( { name: '$block', inside: [ '$clipboardHolder' ] } ) ).to.be.true; From 2360446e4a1ba69e8fefa392a5cc83803fe953d7 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Mon, 17 Jul 2017 09:23:22 +0200 Subject: [PATCH 2/5] `DataController#deleteContent` should leave a paragraph if the entire content was selected. --- src/controller/deletecontent.js | 71 ++++++++++++++++++++++-- tests/controller/deletecontent.js | 92 +++++++++++++++++++++++++++++-- 2 files changed, 152 insertions(+), 11 deletions(-) diff --git a/src/controller/deletecontent.js b/src/controller/deletecontent.js index c137aa4b7..f86ee0429 100644 --- a/src/controller/deletecontent.js +++ b/src/controller/deletecontent.js @@ -32,8 +32,15 @@ export default function deleteContent( selection, batch, options = {} ) { return; } - const selRange = selection.getFirstRange(); + // 0. Replace the entire content with paragraph. + // 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 ); @@ -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(); @@ -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; +} diff --git a/tests/controller/deletecontent.js b/tests/controller/deletecontent.js index 55462ca71..efe5aa7fd 100644 --- a/tests/controller/deletecontent.js +++ b/tests/controller/deletecontent.js @@ -233,12 +233,6 @@ describe( 'DataController', () => { 'f[]x' ); - test( - 'leaves just one element when all selected', - '[xfooy]', - '[]' - ); - it( 'uses remove delta instead of merge delta if merged element is empty', () => { setData( doc, 'ab[cdefgh]' ); @@ -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' ); @@ -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[]z', @@ -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', + '[xx]', + '[]' + ); + + test( + 'when the entire heading and paragraph were selected', + '[xxyy]', + '[]' + ); + + test( + 'inside the limit element when the entire heading and paragraph were inside', + '
[xxyy]
', + '
[]
' + ); + + test( + 'but not if schema does not accept paragraph in limit element', + '
[xxyy]
', + '
[]
' + ); + + test( + 'but not if selection is not containing the whole content', + '[xxyy]', + '[]' + ); + + test( + 'but not if only single element is selected', + '[xx]', + '[]' + ); + + it( 'when root element was not added as Schema.limits works fine as well', () => { + doc.createRoot( 'paragraph', 'paragraphRoot' ); + + setData( + doc, + 'x[]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 ); From 91db6b52e8a5f81632d69bee91c147f36a3df905 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Wed, 19 Jul 2017 08:11:19 +0200 Subject: [PATCH 3/5] Restored removed test, added a new one and fixed test case name. --- tests/controller/deletecontent.js | 12 ++++++++++++ tests/model/schema/schema.js | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/controller/deletecontent.js b/tests/controller/deletecontent.js index efe5aa7fd..2341acbd6 100644 --- a/tests/controller/deletecontent.js +++ b/tests/controller/deletecontent.js @@ -233,6 +233,12 @@ describe( 'DataController', () => { 'f[]x' ); + test( + 'leaves just one element when all selected', + '[xfooy]bar', + '[]bar' + ); + it( 'uses remove delta instead of merge delta if merged element is empty', () => { setData( doc, 'ab[cdefgh]' ); @@ -699,6 +705,12 @@ describe( 'DataController', () => { '[]' ); + test( + 'when the entire content was selected', + '[xfooy]', + '[]' + ); + test( 'inside the limit element when the entire heading and paragraph were inside', '
[xxyy]
', diff --git a/tests/model/schema/schema.js b/tests/model/schema/schema.js index 912f63bdf..3b86e7788 100644 --- a/tests/model/schema/schema.js +++ b/tests/model/schema/schema.js @@ -49,7 +49,7 @@ describe( 'Schema', () => { expect( schema.limits ).to.be.instanceOf( Set ); } ); - it( 'should have defined the limits elements', () => { + it( 'should mark $root as a limit element', () => { expect( schema.limits.has( '$root' ) ).to.be.true; } ); From 9f1c00c8d1cb4886648d086e804db34c1a5d4cb2 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Wed, 19 Jul 2017 08:12:01 +0200 Subject: [PATCH 4/5] Fixed numbers in docs. [skip ci] --- src/controller/deletecontent.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/controller/deletecontent.js b/src/controller/deletecontent.js index f86ee0429..e7615b492 100644 --- a/src/controller/deletecontent.js +++ b/src/controller/deletecontent.js @@ -32,7 +32,7 @@ export default function deleteContent( selection, batch, options = {} ) { return; } - // 0. Replace the entire content with paragraph. + // 1. Replace the entire content with paragraph. // See: https://github.com/ckeditor/ckeditor5-engine/issues/1012#issuecomment-315017594. if ( shouldEntireContentBeReplacedWithParagraph( batch.document.schema, selection ) ) { replaceEntireContentWithParagraph( batch, selection ); @@ -44,12 +44,12 @@ export default function deleteContent( selection, batch, options = {} ) { const startPos = selRange.start; const endPos = LivePosition.createFromPosition( selRange.end ); - // 1. Remove the content if there is any. + // 2. Remove the content if there is any. if ( !selRange.start.isTouching( selRange.end ) ) { batch.remove( selRange ); } - // 2. Merge elements in the right branch to the elements in the left branch. + // 3. Merge elements in the right branch to the elements in the left branch. // The only reasonable (in terms of data and selection correctness) case in which we need to do that is: // // Fo[]ar => Fo^ar @@ -63,7 +63,7 @@ export default function deleteContent( selection, batch, options = {} ) { selection.collapse( startPos ); - // 3. Autoparagraphing. + // 4. 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 ) ) { insertParagraph( batch, startPos, selection ); From 757912c3baa40c9a19bd76d0fac397f2ca883ec6 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Wed, 19 Jul 2017 09:18:47 +0200 Subject: [PATCH 5/5] Fixed code style. --- src/controller/deletecontent.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/controller/deletecontent.js b/src/controller/deletecontent.js index e7615b492..17a298275 100644 --- a/src/controller/deletecontent.js +++ b/src/controller/deletecontent.js @@ -206,7 +206,8 @@ function shouldEntireContentBeReplacedWithParagraph( schema, selection ) { const limitStartPosition = Position.createAt( limitElement ); const limitEndPosition = Position.createAt( limitElement, 'end' ); - if ( !limitStartPosition.isTouching( selection.getFirstPosition() ) || + if ( + !limitStartPosition.isTouching( selection.getFirstPosition() ) || !limitEndPosition.isTouching( selection.getLastPosition() ) ) { return false;