From edb40fc34682a189322d8398da32718210862fa7 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 15 Jul 2020 15:15:34 +0200 Subject: [PATCH 01/16] WIP --- packages/ckeditor5-engine/src/model/schema.js | 21 +++++++++++++++++++ packages/ckeditor5-table/src/tableediting.js | 4 ++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/schema.js b/packages/ckeditor5-engine/src/model/schema.js index 201ef6377f4..4c4f4db5013 100644 --- a/packages/ckeditor5-engine/src/model/schema.js +++ b/packages/ckeditor5-engine/src/model/schema.js @@ -306,6 +306,27 @@ export default class Schema { return !!( def && def.isInline ); } + /** + * Returns `true` if the given item is defined to be + * a selectable element by the {@link module:engine/model/schema~SchemaItemDefinition}'s `isSelectable` property. + * + * TODO + * + * See the {@glink framework/guides/deep-dive/schema#TODO Selectable elements} section of the Schema deep dive} + * guide for more details. + * + * @param {module:engine/model/item~Item|module:engine/model/schema~SchemaContextItem|String} item + */ + isSelectable( item ) { + const def = this.getDefinition( item ); + + if ( !def ) { + return false; + } + + return !!( def.isSelectable || def.isObject ); + } + /** * Checks whether the given node (`child`) can be a child of the given context. * diff --git a/packages/ckeditor5-table/src/tableediting.js b/packages/ckeditor5-table/src/tableediting.js index f46c8e27d36..be6721fcd6e 100644 --- a/packages/ckeditor5-table/src/tableediting.js +++ b/packages/ckeditor5-table/src/tableediting.js @@ -64,7 +64,6 @@ export default class TableEditing extends Plugin { schema.register( 'table', { allowWhere: '$block', allowAttributes: [ 'headingRows', 'headingColumns' ], - isLimit: true, isObject: true, isBlock: true } ); @@ -77,7 +76,8 @@ export default class TableEditing extends Plugin { schema.register( 'tableCell', { allowIn: 'tableRow', allowAttributes: [ 'colspan', 'rowspan' ], - isObject: true + isLimit: true, + isSelectable: true } ); // Allow all $block content inside table cell. From a8ce29d10a5d9fa7c5d528c78f82673be5acf108 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 30 Jul 2020 16:58:36 +0200 Subject: [PATCH 02/16] Added the Schema#isContent() method. Made $text a content in schema. --- packages/ckeditor5-engine/src/model/model.js | 3 +- packages/ckeditor5-engine/src/model/schema.js | 37 ++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/model.js b/packages/ckeditor5-engine/src/model/model.js index 6967b2d5d1a..feeeac3f90d 100644 --- a/packages/ckeditor5-engine/src/model/model.js +++ b/packages/ckeditor5-engine/src/model/model.js @@ -100,7 +100,8 @@ export default class Model { } ); this.schema.register( '$text', { allowIn: '$block', - isInline: true + isInline: true, + isContent: true } ); this.schema.register( '$clipboardHolder', { allowContentOf: '$root', diff --git a/packages/ckeditor5-engine/src/model/schema.js b/packages/ckeditor5-engine/src/model/schema.js index b834e40c1c7..c6838d826b5 100644 --- a/packages/ckeditor5-engine/src/model/schema.js +++ b/packages/ckeditor5-engine/src/model/schema.js @@ -292,7 +292,7 @@ export default class Schema { * schema.isInline( 'paragraph' ); // -> false * schema.isInline( 'softBreak' ); // -> true * - * const text = writer.createText('foo' ); + * const text = writer.createText( 'foo' ); * schema.isInline( text ); // -> true * * See the {@glink framework/guides/deep-dive/schema#inline-elements Inline elements} section of the Schema deep dive @@ -310,7 +310,13 @@ export default class Schema { * Returns `true` if the given item is defined to be * a selectable element by the {@link module:engine/model/schema~SchemaItemDefinition}'s `isSelectable` property. * - * TODO + * schema.isSelectable( 'paragraph' ); // -> false + * schema.isSelectable( 'heading1' ); // -> false + * schema.isSelectable( 'image' ); // -> true + * schema.isSelectable( 'tableCell' ); // -> true + * + * const text = writer.createText( 'foo' ); + * schema.isSelectable( text ); // -> false * * See the {@glink framework/guides/deep-dive/schema#TODO Selectable elements} section of the Schema deep dive} * guide for more details. @@ -327,6 +333,33 @@ export default class Schema { return !!( def.isSelectable || def.isObject ); } + /** + * Returns `true` if the given item is defined to be + * a content by the {@link module:engine/model/schema~SchemaItemDefinition}'s `isContent` property. + * + * schema.isContent( 'paragraph' ); // -> false + * schema.isContent( 'heading1' ); // -> false + * schema.isContent( 'image' ); // -> true + * schema.isContent( 'horizontalLine' ); // -> true + * + * const text = writer.createText( 'foo' ); + * schema.isContent( text ); // -> true + * + * See the {@glink framework/guides/deep-dive/schema#TODO Content elements} section of the Schema deep dive} + * guide for more details. + * + * @param {module:engine/model/item~Item|module:engine/model/schema~SchemaContextItem|String} item + */ + isContent( item ) { + const def = this.getDefinition( item ); + + if ( !def ) { + return false; + } + + return !!( def.isContent || def.isObject ); + } + /** * Checks whether the given node (`child`) can be a child of the given context. * From dac42e8ea73877382e70bae5b366804e0e1a350d Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Aug 2020 14:22:36 +0200 Subject: [PATCH 03/16] Tests: Added Schema#isContent() and Schema#isSelectable() method tests. --- .../ckeditor5-engine/tests/model/schema.js | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/packages/ckeditor5-engine/tests/model/schema.js b/packages/ckeditor5-engine/tests/model/schema.js index fcbdd34aa01..4a386448510 100644 --- a/packages/ckeditor5-engine/tests/model/schema.js +++ b/packages/ckeditor5-engine/tests/model/schema.js @@ -462,6 +462,76 @@ describe( 'Schema', () => { } ); } ); + describe( 'isSelectable()', () => { + it( 'should return true if an item was registered as a selectable', () => { + schema.register( 'foo', { + isSelectable: true + } ); + + expect( schema.isSelectable( 'foo' ) ).to.be.true; + } ); + + it( 'should return true if an item was registered as an object (because all objects are selectables)', () => { + schema.register( 'foo', { + isObject: true + } ); + + expect( schema.isSelectable( 'foo' ) ).to.be.true; + } ); + + it( 'should return false if an item was not registered as an object or selectable', () => { + schema.register( 'foo' ); + + expect( schema.isSelectable( 'foo' ) ).to.be.false; + } ); + + it( 'should return false if an item was not registered at all', () => { + expect( schema.isSelectable( 'foo' ) ).to.be.false; + } ); + + it( 'uses getDefinition()\'s item to definition normalization', () => { + const stub = sinon.stub( schema, 'getDefinition' ).returns( { isSelectable: true } ); + + expect( schema.isSelectable( 'foo' ) ).to.be.true; + expect( stub.calledOnce ).to.be.true; + } ); + } ); + + describe( 'isContent()', () => { + it( 'should return true if an item was registered as a content', () => { + schema.register( 'foo', { + isContent: true + } ); + + expect( schema.isContent( 'foo' ) ).to.be.true; + } ); + + it( 'should return true if an item was registered as an object (because all objects are content)', () => { + schema.register( 'foo', { + isObject: true + } ); + + expect( schema.isContent( 'foo' ) ).to.be.true; + } ); + + it( 'should return false if an item was not registered as an object or a content', () => { + schema.register( 'foo' ); + + expect( schema.isContent( 'foo' ) ).to.be.false; + } ); + + it( 'should return false if an item was not registered at all', () => { + expect( schema.isContent( 'foo' ) ).to.be.false; + } ); + + it( 'uses getDefinition()\'s item to definition normalization', () => { + const stub = sinon.stub( schema, 'getDefinition' ).returns( { isContent: true } ); + + expect( schema.isContent( 'foo' ) ).to.be.true; + expect( stub.calledOnce ).to.be.true; + } ); + } ); + describe( 'checkChild()', () => { beforeEach( () => { schema.register( '$root' ); From 1453b7fa77db379328562c79bae8cdd49c7f4ddd Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Aug 2020 14:24:47 +0200 Subject: [PATCH 04/16] Tests: Extended a model class test to verify that $text is registered as a content in the schema. --- packages/ckeditor5-engine/tests/model/model.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ckeditor5-engine/tests/model/model.js b/packages/ckeditor5-engine/tests/model/model.js index 22585ab20a2..9195090df58 100644 --- a/packages/ckeditor5-engine/tests/model/model.js +++ b/packages/ckeditor5-engine/tests/model/model.js @@ -43,6 +43,7 @@ describe( 'Model', () => { it( 'registers $text to the schema', () => { expect( schema.isRegistered( '$text' ) ).to.be.true; + expect( schema.isContent( '$text' ) ).to.be.true; expect( schema.checkChild( [ '$block' ], '$text' ) ).to.be.true; } ); From e1f9207c284de98f2df1bf60a452f9e57cdb6ecc Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Aug 2020 14:26:04 +0200 Subject: [PATCH 05/16] Updated the model selection post-fixer to properly handle table cells which are no longer registered as objects but selectables in the schema. --- .../src/model/utils/selection-post-fixer.js | 12 ++++++------ .../tests/model/utils/selection-post-fixer.js | 7 ++++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js b/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js index 9c60f7b0471..1560e7fc00c 100644 --- a/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js +++ b/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js @@ -22,7 +22,7 @@ import Position from '../position'; * allows a `$text`. * * None of the selection's non-collapsed ranges crosses a {@link module:engine/model/schema~Schema#isLimit limit element} * boundary (a range must be rooted within one limit element). - * * Only {@link module:engine/model/schema~Schema#isObject object elements} can be selected from the outside + * * Only {@link module:engine/model/schema~Schema#isSelectable selectable elements} can be selected from the outside * (e.g. `[foo]` is invalid). This rule applies independently to both selection ends, so this * selection is correct: `f[oo]`. * @@ -200,8 +200,8 @@ function tryFixingNonCollapsedRage( range, schema ) { if ( isStartInLimit || isEndInLimit ) { const bothInSameParent = ( start.nodeAfter && end.nodeBefore ) && start.nodeAfter.parent === end.nodeBefore.parent; - const expandStart = isStartInLimit && ( !bothInSameParent || !isInObject( start.nodeAfter, schema ) ); - const expandEnd = isEndInLimit && ( !bothInSameParent || !isInObject( end.nodeBefore, schema ) ); + const expandStart = isStartInLimit && ( !bothInSameParent || !isSelectable( start.nodeAfter, schema ) ); + const expandEnd = isEndInLimit && ( !bothInSameParent || !isSelectable( end.nodeBefore, schema ) ); // Although we've already found limit element on start/end positions we must find the outer-most limit element. // as limit elements might be nested directly inside (ie table > tableRow > tableCell). @@ -285,11 +285,11 @@ function mergeIntersectingRanges( ranges ) { return nonIntersectingRanges; } -// Checks if node exists and if it's an object. +// Checks if node exists and if it's a selectable. // // @param {module:engine/model/node~Node} node // @param {module:engine/model/schema~Schema} schema // @returns {Boolean} -function isInObject( node, schema ) { - return node && schema.isObject( node ); +function isSelectable( node, schema ) { + return node && schema.isSelectable( node ); } diff --git a/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js b/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js index a7d87b40215..8624a54c39c 100644 --- a/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js +++ b/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js @@ -41,7 +41,8 @@ describe( 'Selection post-fixer', () => { model.schema.register( 'tableCell', { allowIn: 'tableRow', allowAttributes: [ 'colspan', 'rowspan' ], - isObject: true + isLimit: true, + isSelectable: true } ); model.schema.extend( '$block', { allowIn: 'tableCell' } ); @@ -1274,7 +1275,7 @@ describe( 'Selection post-fixer', () => { 'foo' + '' + '' + - '[aaa]' + + '[]aaa' + 'bbb' + '' + '
' + @@ -1296,7 +1297,7 @@ describe( 'Selection post-fixer', () => { 'foo' + '' + '' + - '[aaa]' + + '[]aaa' + 'bbb' + '' + '
' + From 0892e081ded5b18071cf3e25ecbf4077cabdb1d8 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Aug 2020 14:28:15 +0200 Subject: [PATCH 06/16] Tests: Updated TableEditing tests to reflect the new table and tableCell definitions in the schema. --- packages/ckeditor5-table/tests/tableediting.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/tests/tableediting.js b/packages/ckeditor5-table/tests/tableediting.js index 955cfb471a4..be540e07993 100644 --- a/packages/ckeditor5-table/tests/tableediting.js +++ b/packages/ckeditor5-table/tests/tableediting.js @@ -52,7 +52,6 @@ describe( 'TableEditing', () => { expect( model.schema.isRegistered( 'table' ) ).to.be.true; expect( model.schema.isObject( 'table' ) ).to.be.true; expect( model.schema.isBlock( 'table' ) ).to.be.true; - expect( model.schema.isLimit( 'table' ) ).to.be.true; expect( model.schema.checkChild( [ '$root' ], 'table' ) ).to.be.true; expect( model.schema.checkAttribute( [ '$root', 'table' ], 'headingRows' ) ).to.be.true; @@ -68,6 +67,8 @@ describe( 'TableEditing', () => { // Table cell: expect( model.schema.isRegistered( 'tableCell' ) ).to.be.true; expect( model.schema.isLimit( 'tableCell' ) ).to.be.true; + expect( model.schema.isObject( 'tableCell' ) ).to.be.false; + expect( model.schema.isSelectable( 'tableCell' ) ).to.be.true; expect( model.schema.checkChild( [ '$root' ], 'tableCell' ) ).to.be.false; expect( model.schema.checkChild( [ 'table' ], 'tableCell' ) ).to.be.false; From b02783e22f0cc4042e603bc2522fcdd89ba55a7b Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Aug 2020 14:44:53 +0200 Subject: [PATCH 07/16] Updated the Model#hasContent() method implementation to use Schema#isContent() instead of Schema#isObject(). --- packages/ckeditor5-engine/src/model/model.js | 16 ++++++----- .../ckeditor5-engine/tests/model/model.js | 27 ++++++++++++++++--- 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/model.js b/packages/ckeditor5-engine/src/model/model.js index feeeac3f90d..c8a0e9850ab 100644 --- a/packages/ckeditor5-engine/src/model/model.js +++ b/packages/ckeditor5-engine/src/model/model.js @@ -541,7 +541,7 @@ export default class Model { * * * any text node (`options.ignoreWhitespaces` allows controlling whether this text node must also contain * any non-whitespace characters), - * * or any {@link module:engine/model/schema~Schema#isObject object element}, + * * or any {@link module:engine/model/schema~Schema#isContent content element}, * * or any {@link module:engine/model/markercollection~Marker marker} which * {@link module:engine/model/markercollection~Marker#_affectsData affects data}. * @@ -574,14 +574,16 @@ export default class Model { } for ( const item of range.getItems() ) { - if ( item.is( '$textProxy' ) ) { - if ( !ignoreWhitespaces ) { - return true; - } else if ( item.data.search( /\S/ ) !== -1 ) { + if ( this.schema.isContent( item ) ) { + if ( item.is( '$textProxy' ) ) { + if ( !ignoreWhitespaces ) { + return true; + } else if ( item.data.search( /\S/ ) !== -1 ) { + return true; + } + } else { return true; } - } else if ( this.schema.isObject( item ) ) { - return true; } } diff --git a/packages/ckeditor5-engine/tests/model/model.js b/packages/ckeditor5-engine/tests/model/model.js index 9195090df58..c663ec2c606 100644 --- a/packages/ckeditor5-engine/tests/model/model.js +++ b/packages/ckeditor5-engine/tests/model/model.js @@ -536,6 +536,10 @@ describe( 'Model', () => { schema.register( 'image', { isObject: true } ); + schema.register( 'content', { + inheritAllFrom: '$block', + isContent: true + } ); schema.extend( 'image', { allowIn: 'div' } ); schema.register( 'listItem', { inheritAllFrom: '$block' @@ -545,15 +549,19 @@ describe( 'Model', () => { model, '
' + - '' + + '' + '
' + 'foo' + '
' + - '' + + '' + '
' + '' + '' + - '' + '' + + 'foo' + + '
' + + '' + + '
' ); root = model.document.getRoot(); @@ -762,6 +770,19 @@ describe( 'Model', () => { expect( model.hasContent( pEmpty, { ignoreMarkers: true } ) ).to.be.true; expect( model.hasContent( pEmpty, { ignoreMarkers: true, ignoreWhitespaces: true } ) ).to.be.false; } ); + + it( 'should return true for an item registered as a content (isContent=true, isObject=false) in the schema', () => { + const contentElement = root.getChild( 6 ); + + expect( model.hasContent( contentElement ) ).to.be.true; + } ); + + it( 'should return true if a range contains an item registered as a content (isContent=true, isObject=false) in the schema', () => { + // [
] + const range = new ModelRange( ModelPosition._createAt( root, 6 ), ModelPosition._createAt( root, 7 ) ); + + expect( model.hasContent( range ) ).to.be.true; + } ); } ); describe( 'createPositionFromPath()', () => { From 72f503678543c42b29c6bb19bf637a56e69eab97 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Aug 2020 14:47:26 +0200 Subject: [PATCH 08/16] Tests: Updated the tableCell element schema definitions since it is not an object anymore (but selectable). --- packages/ckeditor5-engine/tests/model/selection.js | 2 +- .../ckeditor5-horizontal-line/tests/horizontallinecommand.js | 2 +- packages/ckeditor5-image/tests/image/imageinsertcommand.js | 2 +- packages/ckeditor5-image/tests/image/utils.js | 2 +- .../ckeditor5-image/tests/imagecaption/imagecaptionediting.js | 2 +- .../ckeditor5-image/tests/imageupload/imageuploadcommand.js | 2 +- packages/ckeditor5-page-break/tests/pagebreakcommand.js | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-engine/tests/model/selection.js b/packages/ckeditor5-engine/tests/model/selection.js index da5dd9b9875..98c7c488f62 100644 --- a/packages/ckeditor5-engine/tests/model/selection.js +++ b/packages/ckeditor5-engine/tests/model/selection.js @@ -973,7 +973,7 @@ describe( 'Selection', () => { 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', isObject: true } ); + model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true, isSelectable: true } ); model.schema.extend( 'p', { allowIn: 'tableCell' } ); } ); diff --git a/packages/ckeditor5-horizontal-line/tests/horizontallinecommand.js b/packages/ckeditor5-horizontal-line/tests/horizontallinecommand.js index bf9444c18b9..553f4a2af2d 100644 --- a/packages/ckeditor5-horizontal-line/tests/horizontallinecommand.js +++ b/packages/ckeditor5-horizontal-line/tests/horizontallinecommand.js @@ -84,7 +84,7 @@ describe( 'HorizontalLineCommand', () => { it( 'should be true when the selection is inside block element inside isLimit element which allows horizontal line', () => { model.schema.register( 'table', { allowWhere: '$block', isLimit: true, isObject: true, isBlock: true } ); model.schema.register( 'tableRow', { allowIn: 'table', isLimit: true } ); - model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true } ); + model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true, isSelectable: true } ); model.schema.extend( '$block', { allowIn: 'tableCell' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'table', view: 'table' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'tableRow', view: 'tableRow' } ); diff --git a/packages/ckeditor5-image/tests/image/imageinsertcommand.js b/packages/ckeditor5-image/tests/image/imageinsertcommand.js index 4fa00ce1091..d16ff546346 100644 --- a/packages/ckeditor5-image/tests/image/imageinsertcommand.js +++ b/packages/ckeditor5-image/tests/image/imageinsertcommand.js @@ -91,7 +91,7 @@ describe( 'ImageInsertCommand', () => { it( 'should be true when the selection is inside block element inside isLimit element which allows image', () => { model.schema.register( 'table', { allowWhere: '$block', isLimit: true, isObject: true, isBlock: true } ); model.schema.register( 'tableRow', { allowIn: 'table', isLimit: true } ); - model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true } ); + model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true, isSelectable: true } ); model.schema.extend( '$block', { allowIn: 'tableCell' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'table', view: 'table' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'tableRow', view: 'tableRow' } ); diff --git a/packages/ckeditor5-image/tests/image/utils.js b/packages/ckeditor5-image/tests/image/utils.js index 7f8cfb46f81..a0cdb9a357f 100644 --- a/packages/ckeditor5-image/tests/image/utils.js +++ b/packages/ckeditor5-image/tests/image/utils.js @@ -192,7 +192,7 @@ describe( 'image widget utils', () => { it( 'should be true when the selection is inside isLimit element which allows image', () => { model.schema.register( 'table', { allowWhere: '$block', isLimit: true, isObject: true, isBlock: true } ); model.schema.register( 'tableRow', { allowIn: 'table', isLimit: true } ); - model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true } ); + model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true, isSelectable: true } ); model.schema.extend( '$block', { allowIn: 'tableCell' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'table', view: 'table' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'tableRow', view: 'tableRow' } ); diff --git a/packages/ckeditor5-image/tests/imagecaption/imagecaptionediting.js b/packages/ckeditor5-image/tests/imagecaption/imagecaptionediting.js index a54997d3f98..254896089f2 100644 --- a/packages/ckeditor5-image/tests/imagecaption/imagecaptionediting.js +++ b/packages/ckeditor5-image/tests/imagecaption/imagecaptionediting.js @@ -256,7 +256,7 @@ describe( 'ImageCaptionEditing', () => { model.change( writer => { model.schema.register( 'table', { allowWhere: '$block', isLimit: true, isObject: true, isBlock: true } ); model.schema.register( 'tableRow', { allowIn: 'table', isLimit: true } ); - model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true } ); + model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true, isSelectable: true } ); model.schema.extend( '$block', { allowIn: 'tableCell' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'table', view: 'table' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'tableRow', view: 'tr' } ); diff --git a/packages/ckeditor5-image/tests/imageupload/imageuploadcommand.js b/packages/ckeditor5-image/tests/imageupload/imageuploadcommand.js index e977e792b91..16c6aac70a9 100644 --- a/packages/ckeditor5-image/tests/imageupload/imageuploadcommand.js +++ b/packages/ckeditor5-image/tests/imageupload/imageuploadcommand.js @@ -108,7 +108,7 @@ describe( 'ImageUploadCommand', () => { it( 'should be true when the selection is inside block element inside isLimit element which allows image', () => { model.schema.register( 'table', { allowWhere: '$block', isLimit: true, isObject: true, isBlock: true } ); model.schema.register( 'tableRow', { allowIn: 'table', isLimit: true } ); - model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true } ); + model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true, isSelectable: true } ); model.schema.extend( '$block', { allowIn: 'tableCell' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'table', view: 'table' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'tableRow', view: 'tableRow' } ); diff --git a/packages/ckeditor5-page-break/tests/pagebreakcommand.js b/packages/ckeditor5-page-break/tests/pagebreakcommand.js index 7ff33ca67eb..3d169c2ae19 100644 --- a/packages/ckeditor5-page-break/tests/pagebreakcommand.js +++ b/packages/ckeditor5-page-break/tests/pagebreakcommand.js @@ -84,7 +84,7 @@ describe( 'PageBreakCommand', () => { it( 'should be true when the selection is inside block element inside isLimit element which allows page break', () => { model.schema.register( 'table', { allowWhere: '$block', isLimit: true, isObject: true, isBlock: true } ); model.schema.register( 'tableRow', { allowIn: 'table', isLimit: true } ); - model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true } ); + model.schema.register( 'tableCell', { allowIn: 'tableRow', isLimit: true, isSelectable: true } ); model.schema.extend( '$block', { allowIn: 'tableCell' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'table', view: 'table' } ); editor.conversion.for( 'downcast' ).elementToElement( { model: 'tableRow', view: 'tableRow' } ); From b9d751653c96daf8c14e8afb5e1185d9990d50d3 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Aug 2020 16:57:10 +0200 Subject: [PATCH 09/16] Docs: Added sections about selectable elements and content elements to the deep dive into schema guide. --- .../docs/framework/guides/deep-dive/schema.md | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-engine/docs/framework/guides/deep-dive/schema.md b/packages/ckeditor5-engine/docs/framework/guides/deep-dive/schema.md index 1fcf95af564..3eff60ac8c7 100644 --- a/packages/ckeditor5-engine/docs/framework/guides/deep-dive/schema.md +++ b/packages/ckeditor5-engine/docs/framework/guides/deep-dive/schema.md @@ -86,9 +86,11 @@ schema.register( 'myImage', { The {@link module:engine/model/schema~Schema#isObject `Schema#isObject()`} can later be used to check this property. - Every "object" is also a "limit" element. + Every "object" is automatically also: - It means that for every element with `isObject` set to `true`, {@link module:engine/model/schema~Schema#isLimit `Schema#isLimit( element )`} will always return `true`. + * a [limit element](#limit-elements) – for every element with `isObject` set `true`, {@link module:engine/model/schema~Schema#isLimit `Schema#isLimit( element )`} will always return `true`. + * a [selectable element](#selectable-elements) – for every element with `isObject` set `true`, {@link module:engine/model/schema~Schema#isSelectable `Schema#isSelectable( element )`} will always return `true`. + * a [content element](#content-elements) – for every element with `isObject` set `true`, {@link module:engine/model/schema~Schema#isContent `Schema#isContent( element )`} will always return `true`. ### Block elements @@ -109,6 +111,42 @@ Currently, the {@link module:engine/model/schema~SchemaItemDefinition#isInline ` The support for inline elements in CKEditor 5 is so far limited to self-contained elements. Because of this, all elements marked with `isInline` should also be marked with `isObject`. +### Selectable elements + +Elements that users can select as a whole (with all their internals) and then, for instance, copy them or apply formatting, are marked with the {@link module:engine/model/schema~SchemaItemDefinition#isSelectable `isSelectable`} property in the schema: + +```js +schema.register( 'mySelectable', { + isSelectable: true +} ); +``` + +The {@link module:engine/model/schema~Schema#isSelectable `Schema#isSelectable()`} method can later be used to check this property. + + + All [object elements](#object-elements) are selectable by default. There are other selectable elements registered in the editor, though. For instance, there is also the `tableCell` model element (rendered as a `` in the editing view) that is selectable while **not** registered as an object. The {@link features/table#table-selection table selection} plugin takes advantage of this fact and allows users create rectangular selections made of multiple table cells. + + +### Content elements + +You can tell content model elements from other elements by looking at their representation in the editor data (you can use {@link module:editor-classic/classiceditor~ClassicEditor#getData `editor.getData()`} or {@link module:engine/model/model~Model#hasContent Model#hasContent()} to check this out). + +Elements such as images or media will **always** find their way into editor data and this is what makes them content elements. They are marked with the {@link module:engine/model/schema~SchemaItemDefinition#isContent `isContent`} property in the schema: + +```js +schema.register( 'myImage', { + isContent: true +} ); +``` + +The {@link module:engine/model/schema~Schema#isContent `Schema#isContent()`} method can later be used to check this property. + +At the same time, elements like paragraphs, list items, or headings **are not** content elements because they are skipped in the editor output when they are empty. From the data perspective they are transparent unless they contain other content elements (an empty paragraph is as good as no paragraph). + + + [Object elements](#object-elements) and [`$text`](#generic-items) are content by default. + + ## Generic items There are three basic generic items: `$root`, `$block` and `$text`. They are defined as follows: From 986c9699dfba68ef90932ec0e2a9f1d97133809f Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Aug 2020 17:53:45 +0200 Subject: [PATCH 10/16] Docs: Added a table of schema properties to the schema deep dive guide. --- .../docs/framework/guides/deep-dive/schema.md | 265 +++++++++++++++++- 1 file changed, 264 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/docs/framework/guides/deep-dive/schema.md b/packages/ckeditor5-engine/docs/framework/guides/deep-dive/schema.md index 3eff60ac8c7..9854fa07105 100644 --- a/packages/ckeditor5-engine/docs/framework/guides/deep-dive/schema.md +++ b/packages/ckeditor5-engine/docs/framework/guides/deep-dive/schema.md @@ -1,5 +1,6 @@ --- category: framework-deep-dive +classes: schema-deep-dive --- # Schema @@ -48,6 +49,222 @@ While this would be incorrect: In addition to setting allowed structures, the schema can also define additional traits of model elements. By using the `is*` properties, a feature author may declare how a certain element should be treated by other features and the engine. +Here is a table listing various model elements and their properties registered in the schema: + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
Schema entryProperties in the definition
isBlockisLimitisObjectisInlineisSelectableisContent
$blocktruefalsefalsefalsefalsefalse
$clipboardHolderfalsetruefalsefalsefalsefalse
$markerfalsefalsefalsefalsefalsefalse
$rootfalsetruefalsefalsefalsefalse
$textfalsefalsefalsetruefalsetrue
blockQuotefalsefalsefalsefalsefalsefalse
captionfalsetruefalsefalsefalsefalse
codeBlocktruefalsefalsefalsefalsefalse
heading1truefalsefalsefalsefalsefalse
heading2truefalsefalsefalsefalsefalse
heading3truefalsefalsefalsefalsefalse
horizontalLinefalsetrue[1]truefalsetrue[2]true[3]
imagetruetrue[1]truefalsetrue[2]true[3]
listItemtruefalsefalsefalsefalsefalse
mediatruetrue[1]truefalsetrue[2]true[3]
pageBreakfalsetrue[1]truefalsetrue[2]true[3]
paragraphtruefalsefalsefalsefalsefalse
softBreakfalsefalsefalsetruefalsefalse
tabletruetrue[1]truefalsetrue[2]true[3]
tableRowfalsetruefalsefalsefalsefalse
tableCellfalsetruefalsefalsetruefalse
+ + + * [1] The value of `isLimit` is `true` for this element because all [objects](#object-elements) are automatically [limit elements](#limit-elements), + * [2] The value of `isSelectable` is `true` for this element because all [objects](#object-elements) are automatically [selectable elements](#selectable-elements), + * [3] The value of `isContent` is `true` for this element because all [objects](#object-elements) are automatically [content elements](#content-elements). + + ### Limit elements Consider a feature like an image caption. The caption text area should construct a boundary to some internal actions: @@ -305,4 +522,50 @@ Finally, the schema plays a crucial role during the conversion from the view to Some features may miss schema checks. If you happen to find such a scenario, do not hesitate to [report it to us](https://github.com/ckeditor/ckeditor5/issues). - + From e12c28072c7fbd4c1c7b31bbb6d52dacd16b581f Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Aug 2020 18:03:14 +0200 Subject: [PATCH 11/16] Docs: Added #isSelectable and #isContent to SchemaItemDefinition. --- packages/ckeditor5-engine/src/model/schema.js | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/schema.js b/packages/ckeditor5-engine/src/model/schema.js index c6838d826b5..2cba5c86ceb 100644 --- a/packages/ckeditor5-engine/src/model/schema.js +++ b/packages/ckeditor5-engine/src/model/schema.js @@ -318,7 +318,7 @@ export default class Schema { * const text = writer.createText( 'foo' ); * schema.isSelectable( text ); // -> false * - * See the {@glink framework/guides/deep-dive/schema#TODO Selectable elements} section of the Schema deep dive} + * See the {@glink framework/guides/deep-dive/schema#selectable-elements Selectable elements} section of the Schema deep dive} * guide for more details. * * @param {module:engine/model/item~Item|module:engine/model/schema~SchemaContextItem|String} item @@ -345,7 +345,7 @@ export default class Schema { * const text = writer.createText( 'foo' ); * schema.isContent( text ); // -> true * - * See the {@glink framework/guides/deep-dive/schema#TODO Content elements} section of the Schema deep dive} + * See the {@glink framework/guides/deep-dive/schema#content-elements Content elements} section of the Schema deep dive} * guide for more details. * * @param {module:engine/model/item~Item|module:engine/model/schema~SchemaContextItem|String} item @@ -1239,6 +1239,25 @@ mix( Schema, ObservableMixin ); * * Read more about the object elements in the * {@glink framework/guides/deep-dive/schema#object-elements Object elements} section of the Schema deep dive} guide. + * + * @property {Boolean} isSelectable + * `true` when an element should be selectable as a whole by the user. Examples of selectable elements: `image`, `table`, `tableCell`, etc. + * + * **Note:** An object is also a selectable element, so + * {@link module:engine/model/schema~Schema#isSelectable `isSelectable()`} returns `true` for object elements automatically. + * + * Read more about selectable elements in the + * {@glink framework/guides/deep-dive/schema#selectable-elements Selectable elements} section of the Schema deep dive} guide. + * + * @property {Boolean} isContent + * An item is a content when it always finds its way to editor data output regardless of the number and type of its descendants. + * Examples of content elements: `$text`, `image`, `table`, etc. (but not `paragraph`, `heading1` or `tableCell`). + * + * **Note:** An object is also a content element, so + * {@link module:engine/model/schema~Schema#isContent `isContent()`} returns `true` for object elements automatically. + * + * Read more about content elements in the + * {@glink framework/guides/deep-dive/schema#content-elements Content elements} section of the Schema deep dive} guide. */ /** From 9f3783a54d5ab39f5cf6485247381f62e7be4477 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 4 Aug 2020 10:44:08 +0200 Subject: [PATCH 12/16] BalloonToolbar should not show up when multiple table cells are selected. --- packages/ckeditor5-ui/package.json | 1 + .../src/toolbar/balloon/balloontoolbar.js | 10 +++++----- .../tests/toolbar/balloon/balloontoolbar.js | 18 ++++++++++++++++-- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-ui/package.json b/packages/ckeditor5-ui/package.json index c5f94160762..788ae8a6faf 100644 --- a/packages/ckeditor5-ui/package.json +++ b/packages/ckeditor5-ui/package.json @@ -28,6 +28,7 @@ "@ckeditor/ckeditor5-mention": "^21.0.0", "@ckeditor/ckeditor5-paragraph": "^21.0.0", "@ckeditor/ckeditor5-horizontal-line": "^21.0.0", + "@ckeditor/ckeditor5-table": "^21.0.0", "@ckeditor/ckeditor5-typing": "^21.0.0" }, "engines": { diff --git a/packages/ckeditor5-ui/src/toolbar/balloon/balloontoolbar.js b/packages/ckeditor5-ui/src/toolbar/balloon/balloontoolbar.js index f4d86033f0e..131c486791c 100644 --- a/packages/ckeditor5-ui/src/toolbar/balloon/balloontoolbar.js +++ b/packages/ckeditor5-ui/src/toolbar/balloon/balloontoolbar.js @@ -222,9 +222,9 @@ export default class BalloonToolbar extends Plugin { return; } - // Do not show the toolbar when there is more than one range in the selection and they fully contain object elements. + // Do not show the toolbar when there is more than one range in the selection and they fully contain selectable elements. // See https://github.com/ckeditor/ckeditor5/issues/6443. - if ( selectionContainsOnlyMultipleObjects( selection, schema ) ) { + if ( selectionContainsOnlyMultipleSelectables( selection, schema ) ) { return; } @@ -364,14 +364,14 @@ function getBalloonPositions( isBackward ) { ]; } -// Returns "true" when the selection has multiple ranges and each range contains an object +// Returns "true" when the selection has multiple ranges and each range contains a selectable element // and nothing else. // // @private // @param {module:engine/model/selection~Selection} selection // @param {module:engine/model/schema~Schema} schema // @returns {Boolean} -function selectionContainsOnlyMultipleObjects( selection, schema ) { +function selectionContainsOnlyMultipleSelectables( selection, schema ) { // It doesn't contain multiple objects if there is only one range. if ( selection.rangeCount === 1 ) { return false; @@ -380,7 +380,7 @@ function selectionContainsOnlyMultipleObjects( selection, schema ) { return [ ...selection.getRanges() ].every( range => { const element = range.getContainedElement(); - return element && schema.isObject( element ); + return element && schema.isSelectable( element ); } ); } diff --git a/packages/ckeditor5-ui/tests/toolbar/balloon/balloontoolbar.js b/packages/ckeditor5-ui/tests/toolbar/balloon/balloontoolbar.js index 68cf6118f87..7fc472fee74 100644 --- a/packages/ckeditor5-ui/tests/toolbar/balloon/balloontoolbar.js +++ b/packages/ckeditor5-ui/tests/toolbar/balloon/balloontoolbar.js @@ -15,6 +15,7 @@ import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic'; import Underline from '@ckeditor/ckeditor5-basic-styles/src/underline'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import HorizontalLine from '@ckeditor/ckeditor5-horizontal-line/src/horizontalline'; +import TableEditing from '@ckeditor/ckeditor5-table/src/tableediting'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; import ResizeObserver from '@ckeditor/ckeditor5-utils/src/dom/resizeobserver'; @@ -56,7 +57,7 @@ describe( 'BalloonToolbar', () => { return ClassicTestEditor .create( editorElement, { - plugins: [ Paragraph, Bold, Italic, BalloonToolbar, HorizontalLine ], + plugins: [ Paragraph, Bold, Italic, BalloonToolbar, HorizontalLine, TableEditing ], balloonToolbar: [ 'bold', 'italic' ] } ) .then( newEditor => { @@ -377,13 +378,26 @@ describe( 'BalloonToolbar', () => { // https://github.com/ckeditor/ckeditor5/issues/6443 it( 'should not add the #toolbarView to the #_balloon when the selection contains more than one fully contained object', () => { - // This is for multi cell selection in tables. setData( model, '[]foo[]' ); balloonToolbar.show(); sinon.assert.notCalled( balloonAddSpy ); } ); + // https://github.com/ckeditor/ckeditor5/issues/6432 + it( 'should not add the #toolbarView to the #_balloon when the selection contains more than one fully contained selectable', () => { + // This is for multi cell selection in tables. + setData( model, '' + + '' + + '[foo]' + + '[bar]' + + '' + + '
' ); + + balloonToolbar.show(); + sinon.assert.notCalled( balloonAddSpy ); + } ); + it( 'should not add #toolbarView to the #_balloon when all components inside #toolbarView are disabled', () => { Array.from( balloonToolbar.toolbarView.items ).forEach( item => { item.isEnabled = false; From a7d6fe2f8ad791c5a6ac62c4fd962f13ffccb58e Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 4 Aug 2020 11:08:43 +0200 Subject: [PATCH 13/16] Tests: Added a Model#hasContent integration tests for an empty selected cell. --- packages/ckeditor5-table/tests/table-integration.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/ckeditor5-table/tests/table-integration.js b/packages/ckeditor5-table/tests/table-integration.js index d74bd7fd1cb..0dfb157775b 100644 --- a/packages/ckeditor5-table/tests/table-integration.js +++ b/packages/ckeditor5-table/tests/table-integration.js @@ -185,5 +185,15 @@ describe( 'Table feature – integration', () => { [ '
Foo[]Bar
' ] ] ) ); } ); + + it( 'should not make the Model#hasContent() method return "true" when an empty table cell is selected', () => { + setModelData( editor.model, '' + + '' + + '[]' + + '' + + '
' ); + + expect( editor.model.hasContent( editor.model.document.selection.getFirstRange() ) ).to.be.false; + } ); } ); } ); From 4126970dec5c3fc89470be0ad313c53ef2475cd6 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 4 Aug 2020 16:49:49 +0200 Subject: [PATCH 14/16] Updated the modifySelection() helper to consider elements defined as isSelectable in the Schema. --- .../src/model/utils/modifyselection.js | 29 ++++---- .../tests/model/utils/modifyselection.js | 70 +++++++++++++++++++ 2 files changed, 86 insertions(+), 13 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/modifyselection.js b/packages/ckeditor5-engine/src/model/utils/modifyselection.js index cc39c36c8ae..ee639d87f6a 100644 --- a/packages/ckeditor5-engine/src/model/utils/modifyselection.js +++ b/packages/ckeditor5-engine/src/model/utils/modifyselection.js @@ -92,41 +92,44 @@ export default function modifySelection( model, selection, options = {} ) { // @param {{ walker, unit, isForward, schema }} data // @param {module:engine/view/treewalker~TreeWalkerValue} value function tryExtendingTo( data, value ) { + const { isForward, walker, unit, schema } = data; + const { type, item, nextPosition } = value; + // If found text, we can certainly put the focus in it. Let's just find a correct position // based on the unit. - if ( value.type == 'text' ) { + if ( type == 'text' ) { if ( data.unit === 'word' ) { - return getCorrectWordBreakPosition( data.walker, data.isForward ); + return getCorrectWordBreakPosition( walker, isForward ); } - return getCorrectPosition( data.walker, data.unit, data.isForward ); + return getCorrectPosition( walker, unit, isForward ); } // Entering an element. - if ( value.type == ( data.isForward ? 'elementStart' : 'elementEnd' ) ) { - // If it's an object, we can select it now. - if ( data.schema.isObject( value.item ) ) { - return Position._createAt( value.item, data.isForward ? 'after' : 'before' ); + if ( type == ( isForward ? 'elementStart' : 'elementEnd' ) ) { + // If it's a selectable, we can select it now. + if ( schema.isSelectable( item ) ) { + return Position._createAt( item, isForward ? 'after' : 'before' ); } // If text allowed on this position, extend to this place. - if ( data.schema.checkChild( value.nextPosition, '$text' ) ) { - return value.nextPosition; + if ( schema.checkChild( nextPosition, '$text' ) ) { + return nextPosition; } } // Leaving an element. else { // If leaving a limit element, stop. - if ( data.schema.isLimit( value.item ) ) { + if ( schema.isLimit( item ) ) { // NOTE: Fast-forward the walker until the end. - data.walker.skip( () => true ); + walker.skip( () => true ); return; } // If text allowed on this position, extend to this place. - if ( data.schema.checkChild( value.nextPosition, '$text' ) ) { - return value.nextPosition; + if ( schema.checkChild( nextPosition, '$text' ) ) { + return nextPosition; } } } diff --git a/packages/ckeditor5-engine/tests/model/utils/modifyselection.js b/packages/ckeditor5-engine/tests/model/utils/modifyselection.js index 7087fc2b46a..db98191603f 100644 --- a/packages/ckeditor5-engine/tests/model/utils/modifyselection.js +++ b/packages/ckeditor5-engine/tests/model/utils/modifyselection.js @@ -917,6 +917,76 @@ describe( 'DataController utils', () => { ); } ); + describe( 'selectable handling', () => { + beforeEach( () => { + model.schema.register( 'sel', { + isSelectable: true + } ); + model.schema.extend( 'sel', { allowIn: '$root' } ); + model.schema.extend( '$text', { allowIn: 'sel' } ); + + model.schema.register( 'inlineSel', { + allowIn: 'p', + isObject: true + } ); + model.schema.extend( '$text', { allowIn: 'inlineSel' } ); + } ); + + test( + 'extends over next selectable element when at the end of an element', + '

foo[]

bar', + '

foo[

bar]' + ); + + test( + 'extends over previous selectable element when at the beginning of an element ', + 'bar

[]foo

', + '[bar

]foo

', + { direction: 'backward' } + ); + + test( + 'extends over selectable elements - forward', + '[]', + '[]' + ); + + it( 'extends over selectable elements - backward', () => { + setData( model, '[]', { lastRangeBackward: true } ); + + modifySelection( model, doc.selection, { direction: 'backward' } ); + + expect( stringify( doc.getRoot(), doc.selection ) ).to.equal( '[]' ); + expect( doc.selection.isBackward ).to.true; + } ); + + test( + 'extends over inline selectables - forward', + '

foo[]bar

', + '

foo[bar]

' + ); + + test( + 'extends over inline selectables - backward', + '

bar[]foo

', + '

[bar]foo

', + { direction: 'backward' } + ); + + test( + 'extends over empty inline selectables - forward', + '

foo[]

', + '

foo[]

' + ); + + test( + 'extends over empty inline selectables - backward', + '

[]foo

', + '

[]foo

', + { direction: 'backward' } + ); + } ); + describe( 'limits handling', () => { beforeEach( () => { model.schema.register( 'inlineLimit', { From 518f1d3b1c4b260bef182bfaa4935a119cad3de5 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 4 Aug 2020 17:03:32 +0200 Subject: [PATCH 15/16] Updated the model selection post-fixer so it does not fix the selection when its boundaries touch selectable elements. --- .../src/model/utils/selection-post-fixer.js | 13 +++++----- .../tests/model/utils/selection-post-fixer.js | 24 +++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js b/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js index 1560e7fc00c..23456400adb 100644 --- a/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js +++ b/packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js @@ -154,8 +154,7 @@ function tryFixingCollapsedRange( range, schema ) { // @param {module:engine/model/schema~Schema} schema // @returns {module:engine/model/range~Range|null} Returns fixed range or null if range is valid. function tryFixingNonCollapsedRage( range, schema ) { - const start = range.start; - const end = range.end; + const { start, end } = range; const isTextAllowedOnStart = schema.checkChild( start, '$text' ); const isTextAllowedOnEnd = schema.checkChild( end, '$text' ); @@ -176,13 +175,13 @@ function tryFixingNonCollapsedRage( range, schema ) { // - [foo] -> [foo] // - [foo] -> [foo] // - f[oo] -> f[oo] - // - [foo] -> [foo] + // - [foo] -> [foo] if ( checkSelectionOnNonLimitElements( start, end, schema ) ) { - const isStartObject = start.nodeAfter && schema.isObject( start.nodeAfter ); - const fixedStart = isStartObject ? null : schema.getNearestSelectionRange( start, 'forward' ); + const isStartBeforeSelectable = start.nodeAfter && schema.isSelectable( start.nodeAfter ); + const fixedStart = isStartBeforeSelectable ? null : schema.getNearestSelectionRange( start, 'forward' ); - const isEndObject = end.nodeBefore && schema.isObject( end.nodeBefore ); - const fixedEnd = isEndObject ? null : schema.getNearestSelectionRange( end, 'backward' ); + const isEndAfterSelectable = end.nodeBefore && schema.isSelectable( end.nodeBefore ); + const fixedEnd = isEndAfterSelectable ? null : schema.getNearestSelectionRange( end, 'backward' ); // The schema.getNearestSelectionRange might return null - if that happens use original position. const rangeStart = fixedStart ? fixedStart.start : start; diff --git a/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js b/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js index 8624a54c39c..8155bd1b072 100644 --- a/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js +++ b/packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js @@ -1206,6 +1206,30 @@ describe( 'Selection post-fixer', () => { expect( getModelData( model ) ).to.equal( '
[
]
' ); } ); + + it( 'should not fix #5 (selection starts before a selectable, which is not an object)', () => { + model.schema.register( 'div', { allowIn: '$root' } ); + model.schema.register( 'selectable', { isSelectable: true, allowIn: 'div' } ); + model.schema.extend( '$text', { allowIn: 'selectable' } ); + + setModelData( model, + '
[foo]
' + ); + + expect( getModelData( model ) ).to.equal( '
[foo]
' ); + } ); + + it( 'should not fix #6 (selection ends after before a selectable, which is not an object)', () => { + model.schema.register( 'div', { allowIn: '$root' } ); + model.schema.register( 'selectable', { isSelectable: true, allowIn: 'div' } ); + model.schema.extend( '$text', { allowIn: 'selectable' } ); + + setModelData( model, + '
[foo]
' + ); + + expect( getModelData( model ) ).to.equal( '
[foo]
' ); + } ); } ); describe( 'non-collapsed selection - inline widget scenarios', () => { From f6b0b4df32157475797bb7cba578b05b6e7510d3 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 4 Aug 2020 17:16:08 +0200 Subject: [PATCH 16/16] Made it possible to create a Schema object out of three granular properties: #isLimit, #isSelectable, and #isContent. --- packages/ckeditor5-engine/src/model/schema.js | 8 +++- .../ckeditor5-engine/tests/model/schema.js | 43 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/src/model/schema.js b/packages/ckeditor5-engine/src/model/schema.js index 2cba5c86ceb..ff082651f19 100644 --- a/packages/ckeditor5-engine/src/model/schema.js +++ b/packages/ckeditor5-engine/src/model/schema.js @@ -282,7 +282,13 @@ export default class Schema { isObject( item ) { const def = this.getDefinition( item ); - return !!( def && def.isObject ); + if ( !def ) { + return false; + } + + // Note: Check out the implementation of #isLimit(), #isSelectable(), and #isContent() + // to understand why these three constitute an object. + return !!( def.isObject || ( def.isLimit && def.isSelectable && def.isContent ) ); } /** diff --git a/packages/ckeditor5-engine/tests/model/schema.js b/packages/ckeditor5-engine/tests/model/schema.js index 4a386448510..a9b1999ab85 100644 --- a/packages/ckeditor5-engine/tests/model/schema.js +++ b/packages/ckeditor5-engine/tests/model/schema.js @@ -401,6 +401,16 @@ describe( 'Schema', () => { expect( schema.isObject( 'foo' ) ).to.be.true; } ); + it( 'returns true if an item is a limit, selectable, and a content at once (but not explicitely an object)', () => { + schema.register( 'foo', { + isLimit: true, + isSelectable: true, + isContent: true + } ); + + expect( schema.isObject( 'foo' ) ).to.be.true; + } ); + it( 'returns false if an item was registered as a limit (because not all limits are objects)', () => { schema.register( 'foo', { isLimit: true @@ -409,6 +419,39 @@ describe( 'Schema', () => { expect( schema.isObject( 'foo' ) ).to.be.false; } ); + it( 'returns false if an item is a limit and a selectable but not a content ' + + '(because an object must always find its way into data regardless of its children)', + () => { + schema.register( 'foo', { + isLimit: true, + isSelectable: true + } ); + + expect( schema.isObject( 'foo' ) ).to.be.false; + } ); + + it( 'returns false if an item is a limit and content but not a selectable ' + + '(because the user must always be able to select an object)', + () => { + schema.register( 'foo', { + isLimit: true, + isContent: true + } ); + + expect( schema.isObject( 'foo' ) ).to.be.false; + } ); + + it( 'returns false if an item is a selectable and a content but not a limit ' + + '(because an object should never be split or crossed by the selection)', + () => { + schema.register( 'foo', { + isSelectable: true, + isContent: true + } ); + + expect( schema.isObject( 'foo' ) ).to.be.false; + } ); + it( 'returns false if an item was not registered as an object', () => { schema.register( 'foo' );