From ff541a4b5bb3fcb7db7017d6cb5230dcd5640e5c Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 21 May 2020 22:41:46 +0200 Subject: [PATCH 01/11] Adjusting #deleteContent() not to merge headings with paragraphs. --- .../src/model/utils/deletecontent.js | 51 +++++++++++++++---- .../tests/model/utils/deletecontent.js | 6 +-- 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js index a74a30fa311..0bfeb4c5de8 100644 --- a/packages/ckeditor5-engine/src/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/src/model/utils/deletecontent.js @@ -82,6 +82,7 @@ export default function deleteContent( model, selection, options = {} ) { const startPos = selRange.start; const endPos = LivePosition.fromPosition( selRange.end, 'toNext' ); + let finalPos = startPos; // 2. Remove the content if there is any. if ( !selRange.start.isTouching( selRange.end ) ) { @@ -96,8 +97,8 @@ export default function deleteContent( model, selection, options = {} ) { // However, the algorithm supports also merging deeper structures (up to the depth of the shallower branch), // as it's hard to imagine what should actually be the default behavior. Usually, specific features will // want to override that behavior anyway. - if ( !options.leaveUnmerged ) { - mergeBranches( writer, startPos, endPos ); + if ( !options.leaveUnmerged && mergeBranches( writer, startPos, endPos ) ) { + finalPos = endPos; // TMP this will be replaced with a postfixer. // We need to check and strip disallowed attributes in all nested nodes because after merge @@ -105,16 +106,16 @@ export default function deleteContent( model, selection, options = {} ) { // // e.g. bold is disallowed for

//

Fo{o

b}ar

->

Fo{}ar

->

Fo{}ar

. - schema.removeDisallowedAttributes( startPos.parent.getChildren(), writer ); + schema.removeDisallowedAttributes( finalPos.parent.getChildren(), writer ); } - collapseSelectionAt( writer, selection, startPos ); + collapseSelectionAt( writer, selection, finalPos ); // 4. Add a paragraph to set selection in it. // Check if a text is allowed in the new container. If not, try to create a new paragraph (if it's allowed here). // If autoparagraphing is off, we assume that you know what you do so we leave the selection wherever it was. - if ( !options.doNotAutoparagraph && shouldAutoparagraph( schema, startPos ) ) { - insertParagraph( writer, startPos, selection ); + if ( !options.doNotAutoparagraph && shouldAutoparagraph( schema, finalPos ) ) { + insertParagraph( writer, finalPos, selection ); } endPos.detach(); @@ -130,19 +131,31 @@ function mergeBranches( writer, startPos, endPos ) { // If both positions ended up in the same parent, then there's nothing more to merge: // <$root>

x[]

{}y

=> <$root>

xy

[]{} if ( startParent == endParent ) { - return; + return false; } // If one of the positions is a limit element, then there's nothing to merge because we don't want to cross the limit boundaries. if ( writer.model.schema.isLimit( startParent ) || writer.model.schema.isLimit( endParent ) ) { - return; + return false; } // Check if operations we'll need to do won't need to cross object or limit boundaries. // E.g., we can't merge endParent into startParent in this case: // x[]{} if ( !checkCanBeMerged( startPos, endPos, writer.model.schema ) ) { - return; + return false; + } + + // Check start and end blocks if we should merge or just remove start block. + // Example: + // []foo -> <[]foo + const startBlock = getParentBlock( startPos ); + const endBlock = getParentBlock( endPos ); + + if ( startBlock && endBlock && startBlock.isEmpty && !endBlock.isEmpty && startBlock.parent == endBlock.parent ) { + writer.remove( startBlock ); + + return true; } // Remember next positions to merge. For example: @@ -180,6 +193,26 @@ function mergeBranches( writer, startPos, endPos ) { // Continue merging next level. mergeBranches( writer, startPos, endPos ); + + return true; +} + +// Finds the lowest element in position's ancestors which is a block. +// It will search until first ancestor that is a limit element. +function getParentBlock( position ) { + const element = position.parent; + const schema = element.root.document.model.schema; + const ancestors = element.getAncestors( { parentFirst: true, includeSelf: true } ); + + for ( const element of ancestors ) { + if ( schema.isLimit( element ) ) { + return null; + } + + if ( schema.isBlock( element ) ) { + return element; + } + } } function shouldAutoparagraph( schema, position ) { diff --git a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js index b9a5b8fe79c..45a43aeda4a 100644 --- a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js @@ -270,9 +270,9 @@ describe( 'DataController utils', () => { ); test( - 'merges second element to an empty first element', + 'removes empty first element', 'x[fo]oy', - 'x[]oy' + 'x[]oy' ); test( @@ -284,7 +284,7 @@ describe( 'DataController utils', () => { test( 'leaves just one element when all selected', '[xfooy]bar', - '[]bar' + '[]bar' ); it( 'uses merge operation even if merged element is empty', () => { From 53ef9a22c2bd61841117fec6bda16f428d614d0c Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Fri, 22 May 2020 10:29:00 +0200 Subject: [PATCH 02/11] Added renaming of first empty block in #deleteContent() to preserve desired element name. --- .../src/model/utils/deletecontent.js | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js index 0bfeb4c5de8..327f270c8c5 100644 --- a/packages/ckeditor5-engine/src/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/src/model/utils/deletecontent.js @@ -82,7 +82,6 @@ export default function deleteContent( model, selection, options = {} ) { const startPos = selRange.start; const endPos = LivePosition.fromPosition( selRange.end, 'toNext' ); - let finalPos = startPos; // 2. Remove the content if there is any. if ( !selRange.start.isTouching( selRange.end ) ) { @@ -97,8 +96,8 @@ export default function deleteContent( model, selection, options = {} ) { // However, the algorithm supports also merging deeper structures (up to the depth of the shallower branch), // as it's hard to imagine what should actually be the default behavior. Usually, specific features will // want to override that behavior anyway. - if ( !options.leaveUnmerged && mergeBranches( writer, startPos, endPos ) ) { - finalPos = endPos; + if ( !options.leaveUnmerged ) { + mergeBranches( writer, startPos, endPos ); // TMP this will be replaced with a postfixer. // We need to check and strip disallowed attributes in all nested nodes because after merge @@ -106,16 +105,16 @@ export default function deleteContent( model, selection, options = {} ) { // // e.g. bold is disallowed for

//

Fo{o

b}ar

->

Fo{}ar

->

Fo{}ar

. - schema.removeDisallowedAttributes( finalPos.parent.getChildren(), writer ); + schema.removeDisallowedAttributes( startPos.parent.getChildren(), writer ); } - collapseSelectionAt( writer, selection, finalPos ); + collapseSelectionAt( writer, selection, startPos ); // 4. Add a paragraph to set selection in it. // Check if a text is allowed in the new container. If not, try to create a new paragraph (if it's allowed here). // If autoparagraphing is off, we assume that you know what you do so we leave the selection wherever it was. - if ( !options.doNotAutoparagraph && shouldAutoparagraph( schema, finalPos ) ) { - insertParagraph( writer, finalPos, selection ); + if ( !options.doNotAutoparagraph && shouldAutoparagraph( schema, startPos ) ) { + insertParagraph( writer, startPos, selection ); } endPos.detach(); @@ -131,31 +130,29 @@ function mergeBranches( writer, startPos, endPos ) { // If both positions ended up in the same parent, then there's nothing more to merge: // <$root>

x[]

{}y

=> <$root>

xy

[]{} if ( startParent == endParent ) { - return false; + return; } // If one of the positions is a limit element, then there's nothing to merge because we don't want to cross the limit boundaries. if ( writer.model.schema.isLimit( startParent ) || writer.model.schema.isLimit( endParent ) ) { - return false; + return; } // Check if operations we'll need to do won't need to cross object or limit boundaries. // E.g., we can't merge endParent into startParent in this case: // x[]{} if ( !checkCanBeMerged( startPos, endPos, writer.model.schema ) ) { - return false; + return; } - // Check start and end blocks if we should merge or just remove start block. + // If first block element is empty then override it's name to make merging more user friendly. // Example: - // []foo -> <[]foo + // []foo -> []foo const startBlock = getParentBlock( startPos ); const endBlock = getParentBlock( endPos ); - if ( startBlock && endBlock && startBlock.isEmpty && !endBlock.isEmpty && startBlock.parent == endBlock.parent ) { - writer.remove( startBlock ); - - return true; + if ( startBlock && endBlock && startBlock.isEmpty && !endBlock.isEmpty && startBlock.name != endBlock.name ) { + writer.rename( startBlock, endBlock.name ); } // Remember next positions to merge. For example: @@ -193,8 +190,6 @@ function mergeBranches( writer, startPos, endPos ) { // Continue merging next level. mergeBranches( writer, startPos, endPos ); - - return true; } // Finds the lowest element in position's ancestors which is a block. From 4b30f8be65cedf4f666bbce19dead223ae5154ad Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Fri, 22 May 2020 21:11:03 +0200 Subject: [PATCH 03/11] Modified #deleteContent() to check the elements next to common ancestor element to decide if it should merge or remove them. --- .../src/model/utils/deletecontent.js | 50 ++++--- .../tests/model/utils/deletecontent.js | 136 ++++-------------- 2 files changed, 59 insertions(+), 127 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js index 327f270c8c5..fb2a95b1176 100644 --- a/packages/ckeditor5-engine/src/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/src/model/utils/deletecontent.js @@ -80,7 +80,7 @@ export default function deleteContent( model, selection, options = {} ) { return; } - const startPos = selRange.start; + let startPos = selRange.start; const endPos = LivePosition.fromPosition( selRange.end, 'toNext' ); // 2. Remove the content if there is any. @@ -97,7 +97,7 @@ export default function deleteContent( model, selection, options = {} ) { // as it's hard to imagine what should actually be the default behavior. Usually, specific features will // want to override that behavior anyway. if ( !options.leaveUnmerged ) { - mergeBranches( writer, startPos, endPos ); + startPos = mergeBranches( writer, startPos, endPos ) || startPos; // TMP this will be replaced with a postfixer. // We need to check and strip disallowed attributes in all nested nodes because after merge @@ -145,14 +145,16 @@ function mergeBranches( writer, startPos, endPos ) { return; } - // If first block element is empty then override it's name to make merging more user friendly. - // Example: + // If the start element on the common ancestor level is empty, and the end element on the same level is not empty + // then remove former one and merging is done. // []foo -> []foo - const startBlock = getParentBlock( startPos ); - const endBlock = getParentBlock( endPos ); + //
[]foo ->
[]foo
+ const [ startAncestor, endAncestor ] = getElementsNextToCommonAncestor( startPos, endPos ); - if ( startBlock && endBlock && startBlock.isEmpty && !endBlock.isEmpty && startBlock.name != endBlock.name ) { - writer.rename( startBlock, endBlock.name ); + if ( !hasContent( startAncestor ) && hasContent( endAncestor ) ) { + writer.remove( startAncestor ); + + return endPos; } // Remember next positions to merge. For example: @@ -192,22 +194,30 @@ function mergeBranches( writer, startPos, endPos ) { mergeBranches( writer, startPos, endPos ); } -// Finds the lowest element in position's ancestors which is a block. -// It will search until first ancestor that is a limit element. -function getParentBlock( position ) { - const element = position.parent; - const schema = element.root.document.model.schema; - const ancestors = element.getAncestors( { parentFirst: true, includeSelf: true } ); +function getElementsNextToCommonAncestor( positionA, positionB ) { + const ancestorsA = positionA.getAncestors(); + const ancestorsB = positionB.getAncestors(); - for ( const element of ancestors ) { - if ( schema.isLimit( element ) ) { - return null; - } + let i = 0; + + while ( ancestorsA[ i ] && ancestorsA[ i ] == ancestorsB[ i ] ) { + i++; + } - if ( schema.isBlock( element ) ) { - return element; + return [ ancestorsA[ i ], ancestorsB[ i ] ]; +} + +function hasContent( element ) { + const schema = element.root.document.model.schema; + const range = Range._createIn( element ); + + for ( const item of range.getItems() ) { + if ( item.is( 'textProxy' ) || schema.isObject( item ) ) { + return true; } } + + return false; } function shouldAutoparagraph( schema, position ) { diff --git a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js index 45a43aeda4a..54cf7b8822e 100644 --- a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js @@ -220,7 +220,8 @@ describe( 'DataController utils', () => { schema.register( 'image', { inheritAllFrom: '$text' } ); schema.register( 'pchild', { allowIn: 'paragraph' } ); schema.register( 'pparent', { allowIn: '$root' } ); - schema.extend( '$text', { allowIn: [ 'pchild', 'pparent' ] } ); + schema.register( 'hchild', { allowIn: 'heading1' } ); + schema.extend( '$text', { allowIn: [ 'pchild', 'pparent', 'hchild' ] } ); } ); test( @@ -344,47 +345,24 @@ describe( 'DataController utils', () => { 'xfo[]ary' ); - it( 'merges elements when deep nested (3rd level)', () => { - const root = doc.getRoot(); - - // We need to use the raw API due to https://github.com/ckeditor/ckeditor5-engine/issues/905. - // xxfo[o - // b]aryy - - root._appendChild( - new Element( 'pparent', null, [ - 'x', - new Element( 'paragraph', null, [ - 'x', - new Element( 'pchild', null, 'foo' ) - ] ) - ] ) - ); - - root._appendChild( - new Element( 'pparent', null, [ - new Element( 'paragraph', null, [ - new Element( 'pchild', null, 'bar' ), - 'y' - ] ), - 'y' - ] ) - ); - - const range = new Range( - new Position( doc.getRoot(), [ 0, 1, 1, 2 ] ), // fo[o - new Position( doc.getRoot(), [ 1, 0, 0, 1 ] ) // b]ar - ); - - model.change( writer => { - writer.setSelection( range ); - } ); + test( + 'removes block element with nested element', + '[foob]ar', + '[]ar' + ); - deleteContent( model, doc.selection ); + test( + 'removes heading element', + '[foob]ar', + '[]ar' + ); - expect( getData( model ) ) - .to.equal( 'xxfo[]aryy' ); - } ); + test( + 'merges elements when deep nested (3rd level)', + 'xxfo[o' + + 'b]aryy', + 'xxfo[]aryy' + ); test( 'merges elements when left end deep nested', @@ -398,40 +376,11 @@ describe( 'DataController utils', () => { 'xfo[]arx' ); - it( 'merges elements when left end deep nested (3rd level)', () => { - const root = doc.getRoot(); - - // We need to use the raw API due to https://github.com/ckeditor/ckeditor5-engine/issues/905. - // xfooba[rb]om - - root._appendChild( - new Element( 'pparent', null, [ - 'x', - new Element( 'paragraph', null, [ - 'foo', - new Element( 'pchild', null, 'bar' ) - ] ) - ] ) - ); - - root._appendChild( - new Element( 'paragraph', null, 'bom' ) - ); - - const range = new Range( - new Position( doc.getRoot(), [ 0, 1, 3, 2 ] ), // ba[r - new Position( doc.getRoot(), [ 1, 1 ] ) // b]om - ); - - model.change( writer => { - writer.setSelection( range ); - } ); - - deleteContent( model, doc.selection ); - - expect( getData( model ) ) - .to.equal( 'xfooba[]om' ); - } ); + test( + 'merges elements when left end deep nested (3rd level)', + 'xfooba[rb]om', + 'xfooba[]om' + ); test( 'merges elements when right end deep nested (in an empty container)', @@ -442,41 +391,14 @@ describe( 'DataController utils', () => { test( 'merges elements when left end deep nested (in an empty container)', '[foob]arx', - '[]arx' + '[]arx' ); - it( 'merges elements when right end deep nested (3rd level)', () => { - const root = doc.getRoot(); - - // We need to use the raw API due to https://github.com/ckeditor/ckeditor5-engine/issues/905. - // fo[obar] - - root._appendChild( - new Element( 'paragraph', null, 'foo' ) - ); - - root._appendChild( - new Element( 'pparent', null, [ - new Element( 'paragraph', null, [ - new Element( 'pchild', null, 'bar' ) - ] ) - ] ) - ); - - const range = new Range( - new Position( doc.getRoot(), [ 0, 2 ] ), // f[oo - new Position( doc.getRoot(), [ 1, 0, 0, 3 ] ) // bar] - ); - - model.change( writer => { - writer.setSelection( range ); - } ); - - deleteContent( model, doc.selection ); - - expect( getData( model ) ) - .to.equal( 'fo[]' ); - } ); + test( + 'merges elements when right end deep nested (3rd level)', + 'fo[obar]', + 'fo[]' + ); } ); describe( 'with object elements', () => { From 33dacbe6549b0d670ae5e3bb8d8241be9fa3217b Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sun, 24 May 2020 16:50:14 +0200 Subject: [PATCH 04/11] Added tests for #deleteContent(). --- .../tests/model/operation/transform.js | 17 ++ .../tests/model/utils/deletecontent.js | 231 ++++++++++++++++-- 2 files changed, 234 insertions(+), 14 deletions(-) diff --git a/packages/ckeditor5-engine/tests/model/operation/transform.js b/packages/ckeditor5-engine/tests/model/operation/transform.js index 83210304bb1..aea215598b2 100644 --- a/packages/ckeditor5-engine/tests/model/operation/transform.js +++ b/packages/ckeditor5-engine/tests/model/operation/transform.js @@ -20,6 +20,7 @@ import MarkerOperation from '../../../src/model/operation/markeroperation'; import MoveOperation from '../../../src/model/operation/moveoperation'; import RenameOperation from '../../../src/model/operation/renameoperation'; import NoOperation from '../../../src/model/operation/nooperation'; +import SplitOperation from '../../../src/model/operation/splitoperation'; describe( 'transform', () => { let model, doc, root, op, nodeA, nodeB, expected; @@ -722,6 +723,22 @@ describe( 'transform', () => { expectOperation( transOp[ 2 ], expected ); } ); } ); + + describe( 'by SplitOperation', () => { + it( 'stays the same if split of end node and has graveyardPosition', () => { + const transformBy = new SplitOperation( + new Position( root, [ 0, 2, 3, 1 ] ), + 1, + new Position( doc.graveyard, [ 0 ] ), + 0 + ); + + const transOp = transform( op, transformBy ); + + expect( transOp.length ).to.equal( 1 ); + expectOperation( transOp[ 0 ], expected ); + } ); + } ); } ); describe( 'RootAttributeOperation', () => { diff --git a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js index 54cf7b8822e..f9233df86bf 100644 --- a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js @@ -216,11 +216,12 @@ describe( 'DataController utils', () => { allowIn: 'pparent', allowAttributes: 'align' } ); - schema.register( 'heading1', { inheritAllFrom: '$block' } ); + schema.register( 'heading1', { inheritAllFrom: '$block', allowIn: 'pparent' } ); schema.register( 'image', { inheritAllFrom: '$text' } ); schema.register( 'pchild', { allowIn: 'paragraph' } ); schema.register( 'pparent', { allowIn: '$root' } ); schema.register( 'hchild', { allowIn: 'heading1' } ); + schema.register( 'widget', { isObject: true, allowWhere: '$text', isInline: true } ); schema.extend( '$text', { allowIn: [ 'pchild', 'pparent', 'hchild' ] } ); } ); @@ -236,6 +237,18 @@ describe( 'DataController utils', () => { 'xfo[]ary' ); + test( + 'removes first element when it\'s empty but second element is not empty (same name)', + 'x[foob]ary', + 'x[]ary' + ); + + test( + 'does not remove first element when first element contains object (same name)', + 'x[foob]ary', + 'x[]ary' + ); + test( 'does not merge second element into the first one (same name, !option.merge)', 'xfo[ob]ary', @@ -243,12 +256,25 @@ describe( 'DataController utils', () => { { leaveUnmerged: true } ); + test( + 'does not remove first empty element when it\'s empty but second element is not empty (same name, !option.merge)', + 'x[foob]ary', + 'x[]ary', + { leaveUnmerged: true } + ); + test( 'merges second element into the first one (different name)', 'xfo[ob]ary', 'xfo[]ary' ); + test( + 'removes first element when it\'s empty but second element is not empty (different name)', + 'x[foob]ary', + 'x[]ary' + ); + // Note: in all these cases we ignore the direction of merge. // If https://github.com/ckeditor/ckeditor5-engine/issues/470 was fixed we could differently treat // forward and backward delete. @@ -284,10 +310,22 @@ describe( 'DataController utils', () => { test( 'leaves just one element when all selected', + '[xfooy]bar', + '[]bar' + ); + + test( + 'leaves just one element when all selected (different name)', '[xfooy]bar', '[]bar' ); + test( + 'leaves just one (first) element when all selected (different name)', + 'foo[xbary]', + 'foo[]' + ); + it( 'uses merge operation even if merged element is empty', () => { let mergeSpy; @@ -318,6 +356,42 @@ describe( 'DataController utils', () => { expect( mergeSpy.called ).to.be.true; } ); + it( 'uses remove operation if first element is empty (because of content delete) and last is not', () => { + let mergeSpy; + let removeSpy; + + setData( model, '[abcdef]gh' ); + + model.change( writer => { + mergeSpy = sinon.spy( writer, 'merge' ); + removeSpy = sinon.spy( writer, 'remove' ); + deleteContent( model, doc.selection ); + } ); + + expect( getData( model ) ).to.equal( '[]gh' ); + + expect( mergeSpy.called ).to.be.false; + expect( removeSpy.called ).to.be.true; + } ); + + it( 'uses remove operation if first element is empty and last is not', () => { + let mergeSpy; + let removeSpy; + + setData( model, '[ef]gh' ); + + model.change( writer => { + mergeSpy = sinon.spy( writer, 'merge' ); + removeSpy = sinon.spy( writer, 'remove' ); + deleteContent( model, doc.selection ); + } ); + + expect( getData( model ) ).to.equal( '[]gh' ); + + expect( mergeSpy.called ).to.be.false; + expect( removeSpy.called ).to.be.true; + } ); + it( 'does not try to move the second block if not needed', () => { let mergeSpy, moveSpy; @@ -351,6 +425,18 @@ describe( 'DataController utils', () => { '[]ar' ); + test( + 'does not remove block element with nested element and object', + '[foob]ar', + '[]ar' + ); + + test( + 'merges heading element', + 'x[foob]ar', + 'x[]ar' + ); + test( 'removes heading element', '[foob]ar', @@ -358,10 +444,9 @@ describe( 'DataController utils', () => { ); test( - 'merges elements when deep nested (3rd level)', - 'xxfo[o' + - 'b]aryy', - 'xxfo[]aryy' + 'does not remove heading element if contains object', + '[foob]ar', + '[]ar' ); test( @@ -370,6 +455,12 @@ describe( 'DataController utils', () => { 'xfo[]aryx' ); + test( + 'removes elements when left end deep nested and will be empty', + '[foob]arx', + '[]arx' + ); + test( 'merges elements when right end deep nested', 'xfo[ob]arx', @@ -377,9 +468,9 @@ describe( 'DataController utils', () => { ); test( - 'merges elements when left end deep nested (3rd level)', - 'xfooba[rb]om', - 'xfooba[]om' + 'removes element when right end deep nested but left end would be empty', + 'x[foob]ar', + 'x[]ar' ); test( @@ -389,16 +480,128 @@ describe( 'DataController utils', () => { ); test( - 'merges elements when left end deep nested (in an empty container)', + 'removes elements when left end deep nested (in an empty container)', '[foob]arx', '[]arx' ); - test( - 'merges elements when right end deep nested (3rd level)', - 'fo[obar]', - 'fo[]' - ); + describe( 'with 3rd level of nesting', () => { + test( + 'merges elements when deep nested (same name)', + 'xxfo[o' + + 'b]aryy', + 'xxfo[]aryy' + ); + + test( + 'removes elements when deep nested (same name)', + '[foo' + + 'b]aryy', + '[]aryy' + ); + + test( + 'removes elements up to common ancestor when deep nested (same name)', + '' + + '[foo' + + 'b]aryy' + + '', + '[]aryy' + ); + + test( + 'merges elements when deep nested (different name)', + 'xxfo[o' + + 'b]aryy', + 'xxfo[]aryy' + ); + + test( + 'removes elements when deep nested (different name)', + '[foo' + + 'b]aryy', + '[]aryy' + ); + + test( + 'removes elements up to common ancestor when deep nested (different names)', + '' + + '[foo' + + 'b]aryy' + + '', + '[]aryy' + ); + } ); + + describe( 'with 3rd level of nesting o the left end', () => { + test( + 'merges elements', + 'xfooba[r' + + 'b]om', + 'xfooba[]om' + ); + + test( + 'merges elements (different names)', + 'xfooba[r' + + 'b]om', + 'xfooba[]om' + ); + + test( + 'removes elements', + '[bar' + + 'b]om', + '[]om' + ); + + test( + 'removes elements up to common ancestor (different names)', + '' + + '[foo' + + 'b]ary' + + '', + '[]ary' + ); + } ); + + describe( 'with 3rd level of nesting o the right end', () => { + test( + 'merges elements', + 'b[om' + + 'ba]r', + 'b[]r' + ); + + test( + 'merges elements (different names)', + 'bo[m' + + 'b]ar', + 'bo[]ar' + ); + test( + 'merges elements (different names, reversed)', + 'bo[m' + + 'b]ar', + 'bo[]ar' + ); + + test( + 'removes elements', + '[bom' + + 'b]ar', + '[]ar' + ); + + test( + 'removes elements up to common ancestor (different names)', + '' + + '[bary' + + 'f]oo' + + '', + '[]oo' + ); + } ); } ); describe( 'with object elements', () => { From cfe4f2ca2e70ffdb5e3982ca0bff39b81cdb7fdb Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 25 May 2020 17:51:32 +0200 Subject: [PATCH 05/11] Refactored Model#deleteContent() to use left or right merge. --- .../src/model/utils/deletecontent.js | 168 ++++++++++++++---- .../tests/model/operation/transform.js | 17 -- .../tests/model/utils/deletecontent.js | 23 +-- 3 files changed, 144 insertions(+), 64 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js index fb2a95b1176..d22bd656cdd 100644 --- a/packages/ckeditor5-engine/src/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/src/model/utils/deletecontent.js @@ -123,25 +123,9 @@ export default function deleteContent( model, selection, options = {} ) { // This function is a result of reaching the Ballmer's peak for just the right amount of time. // Even I had troubles documenting it after a while and after reading it again I couldn't believe that it really works. -function mergeBranches( writer, startPos, endPos ) { - const startParent = startPos.parent; - const endParent = endPos.parent; - - // If both positions ended up in the same parent, then there's nothing more to merge: - // <$root>

x[]

{}y

=> <$root>

xy

[]{} - if ( startParent == endParent ) { - return; - } - - // If one of the positions is a limit element, then there's nothing to merge because we don't want to cross the limit boundaries. - if ( writer.model.schema.isLimit( startParent ) || writer.model.schema.isLimit( endParent ) ) { - return; - } - - // Check if operations we'll need to do won't need to cross object or limit boundaries. - // E.g., we can't merge endParent into startParent in this case: - // x[]{} - if ( !checkCanBeMerged( startPos, endPos, writer.model.schema ) ) { +function mergeBranches( writer, startPosition, endPosition ) { + // Verify if there is a need and possibility to merge. + if ( !checkShouldMerge( writer.model.schema, startPosition, endPosition ) ) { return; } @@ -149,51 +133,160 @@ function mergeBranches( writer, startPos, endPos ) { // then remove former one and merging is done. // []foo -> []foo //
[]foo ->
[]foo
- const [ startAncestor, endAncestor ] = getElementsNextToCommonAncestor( startPos, endPos ); + const [ startAncestor, endAncestor ] = getElementsNextToCommonAncestor( startPosition, endPosition ); if ( !hasContent( startAncestor ) && hasContent( endAncestor ) ) { - writer.remove( startAncestor ); + mergeBranchesRight( writer, startPosition, endPosition, startAncestor.parent ); + } else { + mergeBranchesLeft( writer, startPosition, endPosition, startAncestor.parent ); + } - return endPos; + // The new start position will be equal to end position (this is a LivePosition so it's up to date). + return endPosition; +} + +function mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ) { + const startElement = startPosition.parent; + const endElement = endPosition.parent; + + // Merging reached the common ancestor element, stop here. + if ( startElement == commonAncestor || endElement == commonAncestor ) { + return; } // Remember next positions to merge. For example: - // x[]{}y + // x[]y // will become: - // xy[]{} - startPos = writer.createPositionAfter( startParent ); - endPos = writer.createPositionBefore( endParent ); + // xy[] + startPosition = writer.createPositionAfter( startElement ); + endPosition = writer.createPositionBefore( endElement ); - if ( !endPos.isEqual( startPos ) ) { - // In this case, before we merge, we need to move `endParent` to the `startPos`: - // x[]{}y + if ( !endPosition.isEqual( startPosition ) ) { + // In this case, before we merge, we need to move `endElement` to the `startPosition`: + // x[]y // becomes: - // x[]y{} - writer.insert( endParent, startPos ); + // x[y] + writer.insert( endElement, startPosition ); } // Merge two siblings: // x[]y -> xy (the usual case) // x[]y -> xy[] (this is the "move parent" case shown above) - writer.merge( startPos ); + writer.merge( startPosition ); // Remove empty end ancestors: // fo[obar] // becomes: - // fo[]{} + // fo[] // So we can remove and . - while ( endPos.parent.isEmpty ) { - const parentToRemove = endPos.parent; + while ( endPosition.parent.isEmpty ) { + const parentToRemove = endPosition.parent; + + endPosition = writer.createPositionBefore( parentToRemove ); + + writer.remove( parentToRemove ); + } + + // Verify if there is a need and possibility to merge next level. + if ( !checkShouldMerge( writer.model.schema, startPosition, endPosition ) ) { + return; + } + + // Continue merging next level. + mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ); +} + +function mergeBranchesRight( writer, startPosition, endPosition, commonAncestor ) { + const startElement = startPosition.parent; + const endElement = endPosition.parent; + + // Merging reached the common ancestor element, stop here. + if ( startElement == commonAncestor || endElement == commonAncestor ) { + return; + } + + // Remember next positions to merge. For example: + // x[]y + // will become: + // []xy + startPosition = writer.createPositionAfter( startElement ); + endPosition = writer.createPositionBefore( endElement ); + + if ( !endPosition.isEqual( startPosition ) ) { + // In this case, before we merge, we need to move `startElement` to the `endPosition`: + // x[]y + // becomes: + // [x]y + writer.insert( startElement, endPosition ); + } + + // Remove empty start ancestors: + // x[]y + // becomes: + // [x]y + // So we can remove and . + while ( startPosition.parent.isEmpty ) { + const parentToRemove = startPosition.parent; - endPos = writer.createPositionBefore( parentToRemove ); + startPosition = writer.createPositionBefore( parentToRemove ); writer.remove( parentToRemove ); } + // Update endPosition after inserting and removing elements. + endPosition = writer.createPositionBefore( endElement ); + + // Merge two siblings: + // x[]y -> xy + mergeRight( writer, endPosition ); + + // Verify if there is a need and possibility to merge next level. + if ( !checkShouldMerge( writer.model.schema, startPosition, endPosition ) ) { + return; + } + // Continue merging next level. - mergeBranches( writer, startPos, endPos ); + mergeBranchesRight( writer, startPosition, endPosition, commonAncestor ); +} + +// There is no right merge operation so we need to simulate it. +function mergeRight( writer, position ) { + const startElement = position.nodeBefore; + const endElement = position.nodeAfter; + + if ( startElement.name != endElement.name ) { + writer.rename( startElement, endElement.name ); + } + + writer.clearAttributes( startElement ); + writer.setAttributes( Object.fromEntries( endElement.getAttributes() ), startElement ); + + writer.merge( position ); +} + +// Verifies if merging is needed and possible. +function checkShouldMerge( schema, startPosition, endPosition ) { + const startElement = startPosition.parent; + const endElement = endPosition.parent; + + // If both positions ended up in the same parent, then there's nothing more to merge: + // <$root>

x[

]y

=> <$root>

xy

[] + if ( startElement == endElement ) { + return false; + } + + // If one of the positions is a limit element, then there's nothing to merge because we don't want to cross the limit boundaries. + if ( schema.isLimit( startElement ) || schema.isLimit( endElement ) ) { + return false; + } + + // Check if operations we'll need to do won't need to cross object or limit boundaries. + // E.g., we can't merge endElement into startElement in this case: + // x[] + return checkCanBeMerged( startPosition, endPosition, schema ); } +// Returns the elements that are the ancestors of the provided positions that are direct children of the common ancestor. function getElementsNextToCommonAncestor( positionA, positionB ) { const ancestorsA = positionA.getAncestors(); const ancestorsB = positionB.getAncestors(); @@ -207,6 +300,7 @@ function getElementsNextToCommonAncestor( positionA, positionB ) { return [ ancestorsA[ i ], ancestorsB[ i ] ]; } +// Returns true if element contains any text. function hasContent( element ) { const schema = element.root.document.model.schema; const range = Range._createIn( element ); diff --git a/packages/ckeditor5-engine/tests/model/operation/transform.js b/packages/ckeditor5-engine/tests/model/operation/transform.js index aea215598b2..83210304bb1 100644 --- a/packages/ckeditor5-engine/tests/model/operation/transform.js +++ b/packages/ckeditor5-engine/tests/model/operation/transform.js @@ -20,7 +20,6 @@ import MarkerOperation from '../../../src/model/operation/markeroperation'; import MoveOperation from '../../../src/model/operation/moveoperation'; import RenameOperation from '../../../src/model/operation/renameoperation'; import NoOperation from '../../../src/model/operation/nooperation'; -import SplitOperation from '../../../src/model/operation/splitoperation'; describe( 'transform', () => { let model, doc, root, op, nodeA, nodeB, expected; @@ -723,22 +722,6 @@ describe( 'transform', () => { expectOperation( transOp[ 2 ], expected ); } ); } ); - - describe( 'by SplitOperation', () => { - it( 'stays the same if split of end node and has graveyardPosition', () => { - const transformBy = new SplitOperation( - new Position( root, [ 0, 2, 3, 1 ] ), - 1, - new Position( doc.graveyard, [ 0 ] ), - 0 - ); - - const transOp = transform( op, transformBy ); - - expect( transOp.length ).to.equal( 1 ); - expectOperation( transOp[ 0 ], expected ); - } ); - } ); } ); describe( 'RootAttributeOperation', () => { diff --git a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js index f9233df86bf..49162aa7bd3 100644 --- a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js @@ -356,40 +356,34 @@ describe( 'DataController utils', () => { expect( mergeSpy.called ).to.be.true; } ); - it( 'uses remove operation if first element is empty (because of content delete) and last is not', () => { + it( 'uses merge operation if first element is empty (because of content delete) and last is not', () => { let mergeSpy; - let removeSpy; setData( model, '[abcdef]gh' ); model.change( writer => { mergeSpy = sinon.spy( writer, 'merge' ); - removeSpy = sinon.spy( writer, 'remove' ); deleteContent( model, doc.selection ); } ); expect( getData( model ) ).to.equal( '[]gh' ); - expect( mergeSpy.called ).to.be.false; - expect( removeSpy.called ).to.be.true; + expect( mergeSpy.called ).to.be.true; } ); - it( 'uses remove operation if first element is empty and last is not', () => { + it( 'uses merge operation if first element is empty and last is not', () => { let mergeSpy; - let removeSpy; setData( model, '[ef]gh' ); model.change( writer => { mergeSpy = sinon.spy( writer, 'merge' ); - removeSpy = sinon.spy( writer, 'remove' ); deleteContent( model, doc.selection ); } ); expect( getData( model ) ).to.equal( '[]gh' ); - expect( mergeSpy.called ).to.be.false; - expect( removeSpy.called ).to.be.true; + expect( mergeSpy.called ).to.be.true; } ); it( 'does not try to move the second block if not needed', () => { @@ -523,6 +517,15 @@ describe( 'DataController utils', () => { '[]aryy' ); + test( + 'merges elements up to common ancestor when deep nested (different names)', + '' + + 'fo[o' + + 'b]ar' + + '', + 'fo[]ar' + ); + test( 'removes elements up to common ancestor when deep nested (different names)', '' + From acd65f68d7993c3295df39228a2effd29e10e99d Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 27 May 2020 13:21:13 +0200 Subject: [PATCH 06/11] Model#deleteContent no longer including start of trailing block. --- .../src/model/utils/deletecontent.js | 63 +++++++++++++++---- 1 file changed, 50 insertions(+), 13 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js index d22bd656cdd..a0632e25bc9 100644 --- a/packages/ckeditor5-engine/src/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/src/model/utils/deletecontent.js @@ -71,6 +71,27 @@ export default function deleteContent( model, selection, options = {} ) { const schema = model.schema; + let startPosition = selRange.start; + let endPosition = selRange.end; + + // 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 ( hasContent( selRange ) ) { + const endBlock = getParentBlock( endPosition ); + + if ( endBlock && endPosition.isTouching( model.createPositionAt( endBlock, 0 ) ) ) { + // Create forward selection from range. + const selection = model.createSelection( selRange ); + + // Modify the selection in backward direction to shrink it and remove first position of following block from it. + model.modifySelection( selection, { direction: 'backward' } ); + + endPosition = selection.getLastPosition(); + } + } + + endPosition = LivePosition.fromPosition( endPosition, 'toNext' ); + model.change( writer => { // 1. Replace the entire content with paragraph. // See: https://github.com/ckeditor/ckeditor5-engine/issues/1012#issuecomment-315017594. @@ -80,9 +101,6 @@ export default function deleteContent( model, selection, options = {} ) { return; } - let startPos = selRange.start; - const endPos = LivePosition.fromPosition( selRange.end, 'toNext' ); - // 2. Remove the content if there is any. if ( !selRange.start.isTouching( selRange.end ) ) { writer.remove( selRange ); @@ -97,7 +115,7 @@ export default function deleteContent( model, selection, options = {} ) { // as it's hard to imagine what should actually be the default behavior. Usually, specific features will // want to override that behavior anyway. if ( !options.leaveUnmerged ) { - startPos = mergeBranches( writer, startPos, endPos ) || startPos; + startPosition = mergeBranches( writer, startPosition, endPosition ) || startPosition; // TMP this will be replaced with a postfixer. // We need to check and strip disallowed attributes in all nested nodes because after merge @@ -105,22 +123,40 @@ export default function deleteContent( model, selection, options = {} ) { // // e.g. bold is disallowed for

//

Fo{o

b}ar

->

Fo{}ar

->

Fo{}ar

. - schema.removeDisallowedAttributes( startPos.parent.getChildren(), writer ); + schema.removeDisallowedAttributes( startPosition.parent.getChildren(), writer ); } - collapseSelectionAt( writer, selection, startPos ); + collapseSelectionAt( writer, selection, startPosition ); // 4. Add a paragraph to set selection in it. // Check if a text is allowed in the new container. If not, try to create a new paragraph (if it's allowed here). // If autoparagraphing is off, we assume that you know what you do so we leave the selection wherever it was. - if ( !options.doNotAutoparagraph && shouldAutoparagraph( schema, startPos ) ) { - insertParagraph( writer, startPos, selection ); + if ( !options.doNotAutoparagraph && shouldAutoparagraph( schema, startPosition ) ) { + insertParagraph( writer, startPosition, selection ); } - endPos.detach(); + endPosition.detach(); } ); } +// Finds the lowest element in position's ancestors which is a block. +// It will search until first ancestor that is a limit element. +function getParentBlock( position ) { + const element = position.parent; + const schema = element.root.document.model.schema; + const ancestors = element.getAncestors( { parentFirst: true, includeSelf: true } ); + + for ( const element of ancestors ) { + if ( schema.isLimit( element ) ) { + return null; + } + + if ( schema.isBlock( element ) ) { + return element; + } + } +} + // This function is a result of reaching the Ballmer's peak for just the right amount of time. // Even I had troubles documenting it after a while and after reading it again I couldn't believe that it really works. function mergeBranches( writer, startPosition, endPosition ) { @@ -300,10 +336,11 @@ function getElementsNextToCommonAncestor( positionA, positionB ) { return [ ancestorsA[ i ], ancestorsB[ i ] ]; } -// Returns true if element contains any text. -function hasContent( element ) { - const schema = element.root.document.model.schema; - const range = Range._createIn( element ); +// Returns true if element or range contains any text. +function hasContent( elementOrRange ) { + const model = elementOrRange.root.document.model; + const schema = model.schema; + const range = elementOrRange.is( 'range' ) ? elementOrRange : model.createRangeIn( elementOrRange ); for ( const item of range.getItems() ) { if ( item.is( 'textProxy' ) || schema.isObject( item ) ) { From f0a88136bfebc1fe09e8452cb68ac83bc238c318 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 27 May 2020 19:09:31 +0200 Subject: [PATCH 07/11] Adjusting deleteContent's range to span only blocks selected from user perspective. --- .../src/model/utils/deletecontent.js | 59 +++++++++++-------- .../tests/model/utils/deletecontent.js | 12 ++++ 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js index a0632e25bc9..b09fa7c9518 100644 --- a/packages/ckeditor5-engine/src/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/src/model/utils/deletecontent.js @@ -71,27 +71,6 @@ export default function deleteContent( model, selection, options = {} ) { const schema = model.schema; - let startPosition = selRange.start; - let endPosition = selRange.end; - - // 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 ( hasContent( selRange ) ) { - const endBlock = getParentBlock( endPosition ); - - if ( endBlock && endPosition.isTouching( model.createPositionAt( endBlock, 0 ) ) ) { - // Create forward selection from range. - const selection = model.createSelection( selRange ); - - // Modify the selection in backward direction to shrink it and remove first position of following block from it. - model.modifySelection( selection, { direction: 'backward' } ); - - endPosition = selection.getLastPosition(); - } - } - - endPosition = LivePosition.fromPosition( endPosition, 'toNext' ); - model.change( writer => { // 1. Replace the entire content with paragraph. // See: https://github.com/ckeditor/ckeditor5-engine/issues/1012#issuecomment-315017594. @@ -101,6 +80,9 @@ export default function deleteContent( model, selection, options = {} ) { return; } + // Get the live positions for the range adjusted to span only blocks selected from the user perspective. + const [ startPosition, endPosition ] = getLivePositionsForSelectedBlocks( selRange ); + // 2. Remove the content if there is any. if ( !selRange.start.isTouching( selRange.end ) ) { writer.remove( selRange ); @@ -115,7 +97,7 @@ export default function deleteContent( model, selection, options = {} ) { // as it's hard to imagine what should actually be the default behavior. Usually, specific features will // want to override that behavior anyway. if ( !options.leaveUnmerged ) { - startPosition = mergeBranches( writer, startPosition, endPosition ) || startPosition; + mergeBranches( writer, startPosition, endPosition ); // TMP this will be replaced with a postfixer. // We need to check and strip disallowed attributes in all nested nodes because after merge @@ -135,10 +117,40 @@ export default function deleteContent( model, selection, options = {} ) { insertParagraph( writer, startPosition, selection ); } + startPosition.detach(); endPosition.detach(); } ); } +// Returns the live positions for the range adjusted to span only blocks selected from the user perspective. +function getLivePositionsForSelectedBlocks( range ) { + const model = range.root.document.model; + + const startPosition = range.start; + let endPosition = range.end; + + // 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 ( hasContent( range ) ) { + const endBlock = getParentBlock( endPosition ); + + if ( endBlock && endPosition.isTouching( model.createPositionAt( endBlock, 0 ) ) ) { + // Create forward selection from range. + const selection = model.createSelection( range ); + + // Modify the selection in backward direction to shrink it and remove first position of following block from it. + model.modifySelection( selection, { direction: 'backward' } ); + + endPosition = selection.getLastPosition(); + } + } + + return [ + LivePosition.fromPosition( startPosition, 'toPrevious' ), + LivePosition.fromPosition( endPosition, 'toNext' ) + ]; +} + // Finds the lowest element in position's ancestors which is a block. // It will search until first ancestor that is a limit element. function getParentBlock( position ) { @@ -176,9 +188,6 @@ function mergeBranches( writer, startPosition, endPosition ) { } else { mergeBranchesLeft( writer, startPosition, endPosition, startAncestor.parent ); } - - // The new start position will be equal to end position (this is a LivePosition so it's up to date). - return endPosition; } function mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ) { diff --git a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js index 49162aa7bd3..122c567961e 100644 --- a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js @@ -243,6 +243,18 @@ describe( 'DataController utils', () => { 'x[]ary' ); + test( + 'excludes end block from deletion if selection ends at start position of it', + 'x[foo]bary', + 'x[]bary' + ); + + test( + 'removes empty element', + 'x[]bary', + 'x[]bary' + ); + test( 'does not remove first element when first element contains object (same name)', 'x[foob]ary', From 07776bc0ab59018e22548dfc9690aebf52ad8bcd Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 28 May 2020 10:13:34 +0200 Subject: [PATCH 08/11] Using Model#hasContent. --- .../src/model/utils/deletecontent.js | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js index b09fa7c9518..4febae67f7f 100644 --- a/packages/ckeditor5-engine/src/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/src/model/utils/deletecontent.js @@ -131,7 +131,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 ( hasContent( range ) ) { + if ( model.hasContent( range ) ) { const endBlock = getParentBlock( endPosition ); if ( endBlock && endPosition.isTouching( model.createPositionAt( endBlock, 0 ) ) ) { @@ -172,6 +172,8 @@ function getParentBlock( position ) { // This function is a result of reaching the Ballmer's peak for just the right amount of time. // Even I had troubles documenting it after a while and after reading it again I couldn't believe that it really works. function mergeBranches( writer, startPosition, endPosition ) { + const model = writer.model; + // Verify if there is a need and possibility to merge. if ( !checkShouldMerge( writer.model.schema, startPosition, endPosition ) ) { return; @@ -183,7 +185,7 @@ function mergeBranches( writer, startPosition, endPosition ) { //
[]foo ->
[]foo
const [ startAncestor, endAncestor ] = getElementsNextToCommonAncestor( startPosition, endPosition ); - if ( !hasContent( startAncestor ) && hasContent( endAncestor ) ) { + if ( !model.hasContent( startAncestor ) && model.hasContent( endAncestor ) ) { mergeBranchesRight( writer, startPosition, endPosition, startAncestor.parent ); } else { mergeBranchesLeft( writer, startPosition, endPosition, startAncestor.parent ); @@ -345,21 +347,6 @@ function getElementsNextToCommonAncestor( positionA, positionB ) { return [ ancestorsA[ i ], ancestorsB[ i ] ]; } -// Returns true if element or range contains any text. -function hasContent( elementOrRange ) { - const model = elementOrRange.root.document.model; - const schema = model.schema; - const range = elementOrRange.is( 'range' ) ? elementOrRange : model.createRangeIn( elementOrRange ); - - for ( const item of range.getItems() ) { - if ( item.is( 'textProxy' ) || schema.isObject( item ) ) { - return true; - } - } - - return false; -} - function shouldAutoparagraph( schema, position ) { const isTextAllowed = schema.checkChild( position, '$text' ); const isParagraphAllowed = schema.checkChild( position, 'paragraph' ); From 873ec6cb9090a854af2f934c66b1003f3e394209 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 1 Jun 2020 18:52:25 +0200 Subject: [PATCH 09/11] Updated code comments. --- .../src/model/utils/deletecontent.js | 161 +++++++++++++----- 1 file changed, 118 insertions(+), 43 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js index 4febae67f7f..49782b057f0 100644 --- a/packages/ckeditor5-engine/src/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/src/model/utils/deletecontent.js @@ -123,6 +123,13 @@ export default function deleteContent( model, selection, options = {} ) { } // Returns the live positions for the range adjusted to span only blocks selected from the user perspective. +// +// Examples: +// [foo +// bar +// ]abc <-- this block is not considered as selected +// +// This is the same behavior as in Selection#getSelectedBlocks() "special case". function getLivePositionsForSelectedBlocks( range ) { const model = range.root.document.model; @@ -135,10 +142,11 @@ function getLivePositionsForSelectedBlocks( range ) { const endBlock = getParentBlock( endPosition ); if ( endBlock && endPosition.isTouching( model.createPositionAt( endBlock, 0 ) ) ) { - // Create forward selection from range. + // Create forward selection as a probe to find a valid position after excluding last block from the range. const selection = model.createSelection( range ); - // Modify the selection in backward direction to shrink it and remove first position of following block from it. + // Modify the forward selection in backward direction to shrink it and remove first position of following block from it. + // This is how modifySelection works and here we are making use of it. model.modifySelection( selection, { direction: 'backward' } ); endPosition = selection.getLastPosition(); @@ -152,7 +160,7 @@ function getLivePositionsForSelectedBlocks( range ) { } // Finds the lowest element in position's ancestors which is a block. -// It will search until first ancestor that is a limit element. +// Returns null if a limit element is encountered before reaching a block element. function getParentBlock( position ) { const element = position.parent; const schema = element.root.document.model.schema; @@ -180,10 +188,31 @@ function mergeBranches( writer, startPosition, endPosition ) { } // If the start element on the common ancestor level is empty, and the end element on the same level is not empty - // then remove former one and merging is done. - // []foo -> []foo - //
[]foo ->
[]foo
- const [ startAncestor, endAncestor ] = getElementsNextToCommonAncestor( startPosition, endPosition ); + // then merge those to the right element so that it's properties are preserved (name, attributes). + // Because of OT merging is used instead of removing elements. + // + // Merge left: + // foo[ -> foo[]bar + // ]bar -> --^ + // + // Merge right: + // [ -> + // ]bar -> []bar + // + // Merge left: + //
->
+ // foo[ -> foo[]bar + // ]bar -> --^ + //
->
+ // + // Merge right: + //
->
+ // [ -> + // ]bar -> []bar + //
->
+ + // Merging should not go deeper than common ancestor. + const [ startAncestor, endAncestor ] = getAncestorsJustBelowCommonAncestor( startPosition, endPosition ); if ( !model.hasContent( startAncestor ) && model.hasContent( endAncestor ) ) { mergeBranchesRight( writer, startPosition, endPosition, startAncestor.parent ); @@ -192,6 +221,18 @@ function mergeBranches( writer, startPosition, endPosition ) { } } +// Merging blocks to the left (properties of the left block are preserved). +// Simple example: +// foo[ -> foo[bar] +// ]bar -> --^ +// +// Nested example: +//
->
+// foo[ -> foo[bar +//
->
] ^ +// -> | +// ]bar -> --- +// -> function mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ) { const startElement = startPosition.parent; const endElement = endPosition.parent; @@ -201,31 +242,41 @@ function mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ) return; } - // Remember next positions to merge. For example: - // x[]y - // will become: - // xy[] + // Remember next positions to merge in next recursive step (also used as modification points pointers). startPosition = writer.createPositionAfter( startElement ); endPosition = writer.createPositionBefore( endElement ); + // Move endElement just after startElement if they aren't siblings. if ( !endPosition.isEqual( startPosition ) ) { - // In this case, before we merge, we need to move `endElement` to the `startPosition`: - // x[]y - // becomes: - // x[y] + //
->
+ // foo[ -> foo[bar + //
->
^ + // -> | + // ]bar -> ] --- + // -> writer.insert( endElement, startPosition ); } - // Merge two siblings: - // x[]y -> xy (the usual case) - // x[]y -> xy[] (this is the "move parent" case shown above) + // Merge two siblings (nodes on sides of startPosition): + //
->
+ // foo[bar -> foo[bar + //
->
+ // -> + // ] -> ] + // -> + // + // Or in simple case (without moving elements in above if): + // foo[bar] -> foo[bar] + // writer.merge( startPosition ); // Remove empty end ancestors: - // fo[obar] - // becomes: - // fo[] - // So we can remove and . + //
->
+ // foo[bar -> foo[bar + //
->
+ // -> + // ] -> ] + // -> while ( endPosition.parent.isEmpty ) { const parentToRemove = endPosition.parent; @@ -239,10 +290,22 @@ function mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ) return; } - // Continue merging next level. + // Continue merging next level (blockQuote with blockBlock in the examples above if it would not be empty and got removed). mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ); } +// Merging blocks to the right (properties of the right block are preserved). +// Simple example: +// foo[ -> --v +// ]bar -> [foo]bar +// +// Nested example: +//
-> +// foo[ -> --- +//
-> | +// -> [ v +// ]bar -> foo]bar +// -> function mergeBranchesRight( writer, startPosition, endPosition, commonAncestor ) { const startElement = startPosition.parent; const endElement = endPosition.parent; @@ -252,26 +315,28 @@ function mergeBranchesRight( writer, startPosition, endPosition, commonAncestor return; } - // Remember next positions to merge. For example: - //
x[]y - // will become: - // []xy + // Remember next positions to merge in next recursive step (also used as modification points pointers). startPosition = writer.createPositionAfter( startElement ); endPosition = writer.createPositionBefore( endElement ); + // Move startElement just before endElement if they aren't siblings. if ( !endPosition.isEqual( startPosition ) ) { - // In this case, before we merge, we need to move `startElement` to the `endPosition`: - // x[]y - // becomes: - // [x]y + //
->
+ // foo[ -> [ --- + //
->
| + // -> v + // ]bar -> foo]bar + // -> writer.insert( startElement, endPosition ); } - // Remove empty start ancestors: - // x[]y - // becomes: - // [x]y - // So we can remove and . + // Remove empty end ancestors: + //
-> + // [ -> [ + //
-> + // -> + // foo]bar -> foo]bar + // -> while ( startPosition.parent.isEmpty ) { const parentToRemove = startPosition.parent; @@ -283,8 +348,17 @@ function mergeBranchesRight( writer, startPosition, endPosition, commonAncestor // Update endPosition after inserting and removing elements. endPosition = writer.createPositionBefore( endElement ); - // Merge two siblings: - //
x[]y -> xy + // Merge right two siblings (nodes on sides of endPosition): + // -> + // [ -> [ + // -> + // -> + // foo]bar -> foo]bar + // -> + // + // Or in simple case (without moving elements in above if): + // [foo]bar -> [foo]bar + // mergeRight( writer, endPosition ); // Verify if there is a need and possibility to merge next level. @@ -292,7 +366,7 @@ function mergeBranchesRight( writer, startPosition, endPosition, commonAncestor return; } - // Continue merging next level. + // Continue merging next level (blockQuote with blockBlock in the examples above if it would not be empty and got removed). mergeBranchesRight( writer, startPosition, endPosition, commonAncestor ); } @@ -311,7 +385,8 @@ function mergeRight( writer, position ) { writer.merge( position ); } -// Verifies if merging is needed and possible. +// Verifies if merging is needed and possible. It's not needed if both positions are in the same element +// and it's not possible if some element is a limit or the range crosses a limit element. function checkShouldMerge( schema, startPosition, endPosition ) { const startElement = startPosition.parent; const endElement = endPosition.parent; @@ -330,11 +405,11 @@ function checkShouldMerge( schema, startPosition, endPosition ) { // Check if operations we'll need to do won't need to cross object or limit boundaries. // E.g., we can't merge endElement into startElement in this case: // x[] - return checkCanBeMerged( startPosition, endPosition, schema ); + return isCrossingLimitElement( startPosition, endPosition, schema ); } // Returns the elements that are the ancestors of the provided positions that are direct children of the common ancestor. -function getElementsNextToCommonAncestor( positionA, positionB ) { +function getAncestorsJustBelowCommonAncestor( positionA, positionB ) { const ancestorsA = positionA.getAncestors(); const ancestorsB = positionB.getAncestors(); @@ -360,7 +435,7 @@ function shouldAutoparagraph( schema, position ) { // E.g. in

x[]

{} // we'll check

, , and . // Usually, widget and caption are marked as objects/limits in the schema, so in this case merging will be blocked. -function checkCanBeMerged( leftPos, rightPos, schema ) { +function isCrossingLimitElement( leftPos, rightPos, schema ) { const rangeToCheck = new Range( leftPos, rightPos ); for ( const value of rangeToCheck.getWalker() ) { From 41bdc634536edf4491906b492c83079cda15bbbd Mon Sep 17 00:00:00 2001 From: Kuba Niegowski <1232187+niegowski@users.noreply.github.com> Date: Tue, 2 Jun 2020 13:46:15 +0200 Subject: [PATCH 10/11] Apply review comment. Co-authored-by: Maciej --- packages/ckeditor5-engine/tests/model/utils/deletecontent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js index 122c567961e..69d1ec8489f 100644 --- a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js @@ -244,7 +244,7 @@ describe( 'DataController utils', () => { ); test( - 'excludes end block from deletion if selection ends at start position of it', + 'do not remove end block if selection ends at start position of it', 'x[foo]bary', 'x[]bary' ); From 16722aed094ca7b5381019b9d1ef298dc23b435b Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 2 Jun 2020 15:55:05 +0200 Subject: [PATCH 11/11] updated tests' descriptions for deleteContent. --- .../src/model/utils/deletecontent.js | 14 ++++++-- .../tests/model/utils/deletecontent.js | 34 +++++++++++++------ 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/deletecontent.js b/packages/ckeditor5-engine/src/model/utils/deletecontent.js index 49782b057f0..2643ad4a63c 100644 --- a/packages/ckeditor5-engine/src/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/src/model/utils/deletecontent.js @@ -122,9 +122,8 @@ export default function deleteContent( model, selection, options = {} ) { } ); } -// Returns the live positions for the range adjusted to span only blocks selected from the user perspective. +// Returns the live positions for the range adjusted to span only blocks selected from the user perspective. Example: // -// Examples: // [foo // bar // ]abc <-- this block is not considered as selected @@ -233,6 +232,7 @@ function mergeBranches( writer, startPosition, endPosition ) { // -> | // ]bar -> --- // -> +// function mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ) { const startElement = startPosition.parent; const endElement = endPosition.parent; @@ -248,16 +248,19 @@ function mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ) // Move endElement just after startElement if they aren't siblings. if ( !endPosition.isEqual( startPosition ) ) { + // //

->
// foo[ -> foo[bar //
->
^ // -> | // ]bar -> ] --- // -> + // writer.insert( endElement, startPosition ); } // Merge two siblings (nodes on sides of startPosition): + // //
->
// foo[bar -> foo[bar //
->
@@ -271,12 +274,14 @@ function mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ) writer.merge( startPosition ); // Remove empty end ancestors: + // //
->
// foo[bar -> foo[bar //
->
// -> // ] -> ] // -> + // while ( endPosition.parent.isEmpty ) { const parentToRemove = endPosition.parent; @@ -306,6 +311,7 @@ function mergeBranchesLeft( writer, startPosition, endPosition, commonAncestor ) // -> [ v // ]bar -> foo]bar // -> +// function mergeBranchesRight( writer, startPosition, endPosition, commonAncestor ) { const startElement = startPosition.parent; const endElement = endPosition.parent; @@ -321,22 +327,26 @@ function mergeBranchesRight( writer, startPosition, endPosition, commonAncestor // Move startElement just before endElement if they aren't siblings. if ( !endPosition.isEqual( startPosition ) ) { + // //
->
// foo[ -> [ --- //
->
| // -> v // ]bar -> foo]bar // -> + // writer.insert( startElement, endPosition ); } // Remove empty end ancestors: + // //
-> // [ -> [ //
-> // -> // foo]bar -> foo]bar // -> + // while ( startPosition.parent.isEmpty ) { const parentToRemove = startPosition.parent; diff --git a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js index 69d1ec8489f..89a883d8a9b 100644 --- a/packages/ckeditor5-engine/tests/model/utils/deletecontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/deletecontent.js @@ -238,7 +238,7 @@ describe( 'DataController utils', () => { ); test( - 'removes first element when it\'s empty but second element is not empty (same name)', + 'merges first element into the second element (it would become empty but second element would not) (same name)', 'x[foob]ary', 'x[]ary' ); @@ -250,13 +250,13 @@ describe( 'DataController utils', () => { ); test( - 'removes empty element', + 'removes empty element (merges it into second element)', 'x[]bary', 'x[]bary' ); test( - 'does not remove first element when first element contains object (same name)', + 'treats inline widget elements as content so parent element is not considered as empty after merging (same name)', 'x[foob]ary', 'x[]ary' ); @@ -327,13 +327,13 @@ describe( 'DataController utils', () => { ); test( - 'leaves just one element when all selected (different name)', + 'leaves just one (last) element when all selected (first block would become empty) (different name)', '[xfooy]bar', '[]bar' ); test( - 'leaves just one (first) element when all selected (different name)', + 'leaves just one (first) element when all selected (first block would not become empty) (different name)', 'foo[xbary]', 'foo[]' ); @@ -368,7 +368,7 @@ describe( 'DataController utils', () => { expect( mergeSpy.called ).to.be.true; } ); - it( 'uses merge operation if first element is empty (because of content delete) and last is not', () => { + it( 'uses "merge" operation (from OT) if first element is empty (because of content delete) and last is not', () => { let mergeSpy; setData( model, '[abcdef]gh' ); @@ -426,7 +426,7 @@ describe( 'DataController utils', () => { ); test( - 'removes block element with nested element', + 'merges block element to the right (with nested element)', '[foob]ar', '[]ar' ); @@ -438,19 +438,25 @@ describe( 'DataController utils', () => { ); test( - 'merges heading element', + 'merges nested elements', 'x[foob]ar', 'x[]ar' ); test( - 'removes heading element', + 'merges nested elements on multiple levels', + 'x[foob]arabc', + 'x[]arabc' + ); + + test( + 'merges nested elements to the right if left side element would become empty', '[foob]ar', '[]ar' ); test( - 'does not remove heading element if contains object', + 'merges to the left if first element contains object (considers it as a content of that element)', '[foob]ar', '[]ar' ); @@ -462,7 +468,13 @@ describe( 'DataController utils', () => { ); test( - 'removes elements when left end deep nested and will be empty', + 'merges nested elements to the right (on multiple levels) if left side element would become empty', + '[foob]arabc', + '[]arabc' + ); + + test( + 'merges to the right element when left end deep nested and will be empty', '[foob]arx', '[]arx' );