From 7bd2c5a542fcfe819498ccc2500450a228f61c53 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 22 Dec 2020 19:43:25 +0100 Subject: [PATCH 1/4] WIP insertContent batching to reduce number of created operations. --- .../src/model/utils/insertcontent.js | 357 ++++++++++-------- .../ckeditor5-engine/tests/model/model.js | 2 +- .../tests/model/utils/insertcontent.js | 2 +- 3 files changed, 210 insertions(+), 151 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.js b/packages/ckeditor5-engine/src/model/utils/insertcontent.js index 7fb94c67d3e..30f70f35389 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.js @@ -64,12 +64,7 @@ export default function insertContent( model, content, selectable, placeOrOffset nodesToInsert = [ content ]; } - insertion.handleNodes( nodesToInsert, { - // The set of children being inserted is the only set in this context - // so it's the first and last (it's a hack ;)). - isFirst: true, - isLast: true - } ); + insertion.handleNodes( nodesToInsert ); const newRange = insertion.getSelectionRange(); @@ -143,6 +138,34 @@ class Insertion { */ this.schema = model.schema; + /** + * TODO + * @private + */ + this._documentFragment = writer.createDocumentFragment(); + + /** + * TODO + * @private + */ + this._documentFragmentPosition = writer.createPositionAt( this._documentFragment, 0 ); + + /** + * TODO + * @private + */ + this._firstNode = null; + + /** + * TODO + * @private + */ + this._lastNode = null; + + /** + * TODO + * @private + */ this._filterAttributesOf = []; /** @@ -166,20 +189,29 @@ class Insertion { * Handles insertion of a set of nodes. * * @param {Iterable.} nodes Nodes to insert. - * @param {Object} parentContext Context in which parent of these nodes was supposed to be inserted. - * If the parent context is passed it means that the parent element was stripped (was not allowed). */ - handleNodes( nodes, parentContext ) { - nodes = Array.from( nodes ); + handleNodes( nodes ) { + for ( const node of Array.from( nodes ) ) { + this._handleNode( node ); + } - for ( let i = 0; i < nodes.length; i++ ) { - const node = nodes[ i ]; + // Insert nodes collected in temporary DocumentFragment. + this._insertPartialFragment(); - this._handleNode( node, { - isFirst: i === 0 && parentContext.isFirst, - isLast: ( i === ( nodes.length - 1 ) ) && parentContext.isLast - } ); - } + // After the node was inserted we may try to merge it with its siblings. + // This should happen only if it was the first and/or last of the nodes (so only with boundary nodes) + // and only if the selection was in those elements initially. + // + // E.g.: + //

x^

+

y

=>

x

y

=>

xy[]

+ // and: + //

x^y

+

z

=>

x

^

y

+

z

=>

x

z

y

=>

xz[]y

+ // but: + //

x

^

z

+

y

=>

x

y

z

(no merging) + //

x

[]

z

+

y

=>

x

y

z

(no merging, note: after running deleteContents + // it's exactly the same case as above) + this._mergeSiblingsOfFirstNode(); + this._mergeSiblingsOfLastNode(); // TMP this will become a post-fixer. this.schema.removeDisallowedAttributes( this._filterAttributesOf, this.writer ); @@ -232,16 +264,13 @@ class Insertion { * * @private * @param {module:engine/model/node~Node} node - * @param {Object} context - * @param {Boolean} context.isFirst Whether the given node is the first one in the content to be inserted. - * @param {Boolean} context.isLast Whether the given node is the last one in the content to be inserted. */ - _handleNode( node, context ) { + _handleNode( node ) { // Let's handle object in a special way. // * They should never be merged with other elements. // * If they are not allowed in any of the selection ancestors, they could be either autoparagraphed or totally removed. if ( this.schema.isObject( node ) ) { - this._handleObject( node, context ); + this._handleObject( node ); return; } @@ -249,60 +278,71 @@ class Insertion { // Try to find a place for the given node. // Split the position.parent's branch up to a point where the node can be inserted. // If it isn't allowed in the whole branch, then of course don't split anything. - const isAllowed = this._checkAndSplitToAllowedPosition( node, context ); + const isAllowed = this._checkAndSplitToAllowedPosition( node ); if ( !isAllowed ) { - this._handleDisallowedNode( node, context ); + this._handleDisallowedNode( node ); return; } - this._insert( node ); + // Add node to the current temporary DocumentFragment. + this._insertToFragment( node ); - // After the node was inserted we may try to merge it with its siblings. - // This should happen only if it was the first and/or last of the nodes (so only with boundary nodes) - // and only if the selection was in those elements initially. - // - // E.g.: - //

x^

+

y

=>

x

y

=>

xy[]

- // and: - //

x^y

+

z

=>

x

^

y

+

z

=>

x

z

y

=>

xz[]y

- // but: - //

x

^

z

+

y

=>

x

y

z

(no merging) - //

x

[]

z

+

y

=>

x

y

z

(no merging, note: after running deleteContents - // it's exactly the same case as above) - this._mergeSiblingsOf( node, context ); + if ( !this._firstNode ) { + this._firstNode = node; + } + + this._lastNode = node; + } + + /** + * TODO + * @private + */ + _insertPartialFragment() { + if ( this._documentFragment.isEmpty ) { + return; + } + + const livePosition = LivePosition.fromPosition( this.position, 'toNext' ); + + this._setAffectedBoundaries( this.position ); + + this.writer.insert( this._documentFragment, this.position ); + this._documentFragmentPosition = this.writer.createPositionAt( this._documentFragment, 0 ); + + this.position = livePosition.toPosition(); + livePosition.detach(); } /** * @private * @param {module:engine/model/element~Element} node The object element. - * @param {Object} context */ - _handleObject( node, context ) { + _handleObject( node ) { // Try finding it a place in the tree. if ( this._checkAndSplitToAllowedPosition( node ) ) { - this._insert( node ); + this._insertToFragment( node ); } // Try autoparagraphing. else { - this._tryAutoparagraphing( node, context ); + this._tryAutoparagraphing( node ); } } /** * @private * @param {module:engine/model/node~Node} node The disallowed node which needs to be handled. - * @param {Object} context */ - _handleDisallowedNode( node, context ) { + _handleDisallowedNode( node ) { // If the node is an element, try inserting its children (strip the parent). if ( node.is( 'element' ) ) { - this.handleNodes( node.getChildren(), context ); + this.handleNodes( node.getChildren() ); } // If text is not allowed, try autoparagraphing it. else { - this._tryAutoparagraphing( node, context ); + this._tryAutoparagraphing( node ); } } @@ -310,7 +350,7 @@ class Insertion { * @private * @param {module:engine/model/node~Node} node The node to insert. */ - _insert( node ) { + _insertToFragment( node ) { /* istanbul ignore if */ if ( !this.schema.checkChild( this.position, node ) ) { // Algorithm's correctness check. We should never end up here but it's good to know that we did. @@ -330,13 +370,8 @@ class Insertion { ); } - const livePos = LivePosition.fromPosition( this.position, 'toNext' ); - - this._setAffectedBoundaries( this.position ); - this.writer.insert( node, this.position ); - - this.position = livePos.toPosition(); - livePos.detach(); + this.writer.insert( node, this._documentFragmentPosition ); + this._documentFragmentPosition = this._documentFragmentPosition.getShiftedBy( node.offsetSize ); // The last inserted object should be selected because we can't put a collapsed selection after it. if ( this.schema.isObject( node ) && !this.schema.checkChild( this.position, '$text' ) ) { @@ -381,112 +416,136 @@ class Insertion { /** * @private - * @param {module:engine/model/node~Node} node The node which could potentially be merged. - * @param {Object} context */ - _mergeSiblingsOf( node, context ) { + _mergeSiblingsOfFirstNode() { + const node = this._firstNode; + if ( !( node instanceof Element ) ) { return; } - const mergeLeft = this._canMergeLeft( node, context ); - const mergeRight = this._canMergeRight( node, context ); + if ( !this._canMergeLeft( node ) ) { + return; + } + const mergePosLeft = LivePosition._createBefore( node ); mergePosLeft.stickiness = 'toNext'; - const mergePosRight = LivePosition._createAfter( node ); - mergePosRight.stickiness = 'toNext'; - if ( mergeLeft ) { - const livePosition = LivePosition.fromPosition( this.position ); - livePosition.stickiness = 'toNext'; + const livePosition = LivePosition.fromPosition( this.position, 'toNext' ); - // If `_affectedStart` is sames as merge position, it means that the element "marked" by `_affectedStart` is going to be - // removed and its contents will be moved. This won't transform `LivePosition` so `_affectedStart` needs to be moved - // by hand to properly reflect affected range. (Due to `_affectedStart` and `_affectedEnd` stickiness, the "range" is - // shown as `][`). - // - // Example - insert `AbcXyz` at the end of `Foo^`: - // - // FooBar --> - // Foo]AbcXyz[Bar --> - // Foo]AbcXyz[Bar - // - // Note, that if we are here then something must have been inserted, so `_affectedStart` and `_affectedEnd` have to be set. - if ( this._affectedStart.isEqual( mergePosLeft ) ) { - this._affectedStart.detach(); - this._affectedStart = LivePosition._createAt( mergePosLeft.nodeBefore, 'end', 'toPrevious' ); - } + // If `_affectedStart` is sames as merge position, it means that the element "marked" by `_affectedStart` is going to be + // removed and its contents will be moved. This won't transform `LivePosition` so `_affectedStart` needs to be moved + // by hand to properly reflect affected range. (Due to `_affectedStart` and `_affectedEnd` stickiness, the "range" is + // shown as `][`). + // + // Example - insert `AbcXyz` at the end of `Foo^`: + // + // FooBar --> + // Foo]AbcXyz[Bar --> + // Foo]AbcXyz[Bar + // + // Note, that if we are here then something must have been inserted, so `_affectedStart` and `_affectedEnd` have to be set. + if ( this._affectedStart.isEqual( mergePosLeft ) ) { + this._affectedStart.detach(); + this._affectedStart = LivePosition._createAt( mergePosLeft.nodeBefore, 'end', 'toPrevious' ); + } - this.writer.merge( mergePosLeft ); + if ( this._firstNode === this._lastNode ) { + this._firstNode = this._lastNode = mergePosLeft.nodeBefore; + } - // If only one element (the merged one) is in the "affected range", also move the affected range end appropriately. - // - // Example - insert `Abc` at the of `Foo^`: - // - // FooBar --> - // Foo]Abc[Bar --> - // Foo]Abc[Bar --> - // Foo]Abc[Bar - if ( mergePosLeft.isEqual( this._affectedEnd ) && context.isLast ) { - this._affectedEnd.detach(); - this._affectedEnd = LivePosition._createAt( mergePosLeft.nodeBefore, 'end', 'toNext' ); - } + this.writer.merge( mergePosLeft ); - this.position = livePosition.toPosition(); - livePosition.detach(); - } - - if ( mergeRight ) { - /* istanbul ignore if */ - if ( !this.position.isEqual( mergePosRight ) ) { - // Algorithm's correctness check. We should never end up here but it's good to know that we did. - // At this point the insertion position should be after the node we'll merge. If it isn't, - // it should need to be secured as in the left merge case. - /** - * An internal error occurred when merging inserted content with its siblings. - * The insertion position should equal the merge position. - * - * If you encountered this error, report it back to the CKEditor 5 team - * with as many details as possible regarding the content being inserted and the insertion position. - * - * @error insertcontent-invalid-insertion-position - */ - throw new CKEditorError( 'insertcontent-invalid-insertion-position', this ); - } + // If only one element (the merged one) is in the "affected range", also move the affected range end appropriately. + // + // Example - insert `Abc` at the of `Foo^`: + // + // FooBar --> + // Foo]Abc[Bar --> + // Foo]Abc[Bar --> + // Foo]Abc[Bar + if ( mergePosLeft.isEqual( this._affectedEnd ) && this._firstNode === this._lastNode ) { + this._affectedEnd.detach(); + this._affectedEnd = LivePosition._createAt( mergePosLeft.nodeBefore, 'end', 'toNext' ); + } - // Move the position to the previous node, so it isn't moved to the graveyard on merge. - //

x

[]

y

=>

x[]

y

- this.position = Position._createAt( mergePosRight.nodeBefore, 'end' ); + this.position = livePosition.toPosition(); + livePosition.detach(); - // OK:

xx[]

+

yy

=>

xx[]yy

(when sticks to previous) - // NOK:

xx[]

+

yy

=>

xxyy[]

(when sticks to next) - const livePosition = LivePosition.fromPosition( this.position, 'toPrevious' ); + // After merge elements that were marked by _insert() to be filtered might be gone so + // we need to mark the new container. + this._filterAttributesOf.push( this.position.parent ); - // See comment above on moving `_affectedStart`. - if ( this._affectedEnd.isEqual( mergePosRight ) ) { - this._affectedEnd.detach(); - this._affectedEnd = LivePosition._createAt( mergePosRight.nodeBefore, 'end', 'toNext' ); - } + mergePosLeft.detach(); + } + + /** + * @private + */ + _mergeSiblingsOfLastNode() { + const node = this._lastNode; - this.writer.merge( mergePosRight ); + if ( !( node instanceof Element ) ) { + return; + } - // See comment above on moving `_affectedStart`. - if ( mergePosRight.getShiftedBy( -1 ).isEqual( this._affectedStart ) && context.isFirst ) { - this._affectedStart.detach(); - this._affectedStart = LivePosition._createAt( mergePosRight.nodeBefore, 0, 'toPrevious' ); - } + if ( !this._canMergeRight( node ) ) { + return; + } - this.position = livePosition.toPosition(); - livePosition.detach(); + const mergePosRight = LivePosition._createAfter( node ); + mergePosRight.stickiness = 'toNext'; + + /* istanbul ignore if */ + if ( !this.position.isEqual( mergePosRight ) ) { + // Algorithm's correctness check. We should never end up here but it's good to know that we did. + // At this point the insertion position should be after the node we'll merge. If it isn't, + // it should need to be secured as in the left merge case. + /** + * An internal error occurred when merging inserted content with its siblings. + * The insertion position should equal the merge position. + * + * If you encountered this error, report it back to the CKEditor 5 team + * with as many details as possible regarding the content being inserted and the insertion position. + * + * @error insertcontent-invalid-insertion-position + */ + throw new CKEditorError( 'insertcontent-invalid-insertion-position', this ); + } + + // Move the position to the previous node, so it isn't moved to the graveyard on merge. + //

x

[]

y

=>

x[]

y

+ this.position = Position._createAt( mergePosRight.nodeBefore, 'end' ); + + // OK:

xx[]

+

yy

=>

xx[]yy

(when sticks to previous) + // NOK:

xx[]

+

yy

=>

xxyy[]

(when sticks to next) + const livePosition = LivePosition.fromPosition( this.position, 'toPrevious' ); + + // See comment above on moving `_affectedStart`. + if ( this._affectedEnd.isEqual( mergePosRight ) ) { + this._affectedEnd.detach(); + this._affectedEnd = LivePosition._createAt( mergePosRight.nodeBefore, 'end', 'toNext' ); } - if ( mergeLeft || mergeRight ) { - // After merge elements that were marked by _insert() to be filtered might be gone so - // we need to mark the new container. - this._filterAttributesOf.push( this.position.parent ); + if ( this._firstNode === this._lastNode ) { + this._firstNode = this._lastNode = mergePosRight.nodeBefore; } - mergePosLeft.detach(); + this.writer.merge( mergePosRight ); + + // See comment above on moving `_affectedStart`. + if ( mergePosRight.getShiftedBy( -1 ).isEqual( this._affectedStart ) && this._firstNode === this._lastNode ) { + this._affectedStart.detach(); + this._affectedStart = LivePosition._createAt( mergePosRight.nodeBefore, 0, 'toPrevious' ); + } + + this.position = livePosition.toPosition(); + livePosition.detach(); + + // After merge elements that were marked by _insert() to be filtered might be gone so + // we need to mark the new container. + this._filterAttributesOf.push( this.position.parent ); + mergePosRight.detach(); } @@ -495,14 +554,12 @@ class Insertion { * * @private * @param {module:engine/model/node~Node} node The node which could potentially be merged. - * @param {Object} context * @returns {Boolean} */ - _canMergeLeft( node, context ) { + _canMergeLeft( node ) { const previousSibling = node.previousSibling; - return context.isFirst && - ( previousSibling instanceof Element ) && + return ( previousSibling instanceof Element ) && this.canMergeWith.has( previousSibling ) && this.model.schema.checkMerge( previousSibling, node ); } @@ -512,14 +569,12 @@ class Insertion { * * @private * @param {module:engine/model/node~Node} node The node which could potentially be merged. - * @param {Object} context * @returns {Boolean} */ - _canMergeRight( node, context ) { + _canMergeRight( node ) { const nextSibling = node.nextSibling; - return context.isLast && - ( nextSibling instanceof Element ) && + return ( nextSibling instanceof Element ) && this.canMergeWith.has( nextSibling ) && this.model.schema.checkMerge( node, nextSibling ); } @@ -529,9 +584,8 @@ class Insertion { * * @private * @param {module:engine/model/node~Node} node The node which needs to be autoparagraphed. - * @param {Object} context */ - _tryAutoparagraphing( node, context ) { + _tryAutoparagraphing( node ) { const paragraph = this.writer.createElement( 'paragraph' ); // Do not autoparagraph if the paragraph won't be allowed there, @@ -539,7 +593,7 @@ class Insertion { // the next _handleNode() call and we'd be here again. if ( this._getAllowedIn( paragraph, this.position.parent ) && this.schema.checkChild( paragraph, node ) ) { paragraph._appendChild( node ); - this._handleNode( paragraph, context ); + this._handleNode( paragraph ); } } @@ -556,6 +610,11 @@ class Insertion { return false; } + // Insert nodes collected in temporary DocumentFragment if the position parent needs change to process further nodes. + if ( allowedIn != this.position.parent ) { + this._insertPartialFragment(); + } + while ( allowedIn != this.position.parent ) { // If a parent which we'd need to leave is a limit element, break. if ( this.schema.isLimit( this.position.parent ) ) { diff --git a/packages/ckeditor5-engine/tests/model/model.js b/packages/ckeditor5-engine/tests/model/model.js index 15a59a51ae3..394d10fe2c5 100644 --- a/packages/ckeditor5-engine/tests/model/model.js +++ b/packages/ckeditor5-engine/tests/model/model.js @@ -429,7 +429,7 @@ describe( 'Model', () => { model.change( writer => { model.insertContent( new ModelText( 'abc' ) ); - expect( writer.batch.operations ).to.length( 1 ); + expect( writer.batch.operations.filter( operation => operation.isDocumentOperation ) ).to.length( 1 ); } ); } ); } ); diff --git a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js index 0069a5e3307..9ff474206c5 100644 --- a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js @@ -29,7 +29,7 @@ describe( 'DataController utils', () => { model.change( writer => { insertContent( model, writer.createText( 'a' ) ); - expect( writer.batch.operations ).to.length( 1 ); + expect( writer.batch.operations.filter( operation => operation.isDocumentOperation ) ).to.length( 1 ); } ); } ); From 74f19375063412cd25cc2df470efa4180245d004 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 7 Jan 2021 20:25:15 +0100 Subject: [PATCH 2/4] Added workaround for undoing insert + merge when multiple nodes are inserted in one operation. --- .../src/model/utils/insertcontent.js | 53 ++++++++++++++----- .../tests/model/utils/insertcontent.js | 49 +++++++++++++++++ packages/ckeditor5-typing/tests/input.js | 13 +++-- 3 files changed, 95 insertions(+), 20 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.js b/packages/ckeditor5-engine/src/model/utils/insertcontent.js index 30f70f35389..9a4047a7262 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.js @@ -139,32 +139,42 @@ class Insertion { this.schema = model.schema; /** - * TODO + * The temporary DocumentFragment used for grouping multiple nodes for single insert operation. + * * @private + * @type {module:engine/model/documentfragment~DocumentFragment} */ this._documentFragment = writer.createDocumentFragment(); /** - * TODO + * The current position in the temporary DocumentFragment. + * * @private + * @type {module:engine/model/position~Position} */ this._documentFragmentPosition = writer.createPositionAt( this._documentFragment, 0 ); /** - * TODO + * The reference to the first inserted node. + * * @private + * @type {module:engine/model/node~Node} */ this._firstNode = null; /** - * TODO + * The reference to the last inserted node. + * * @private + * @type {module:engine/model/node~Node} */ this._lastNode = null; /** - * TODO + * The array of nodes that should be cleaned of not allowed attributes. + * * @private + * @type {Array.} */ this._filterAttributesOf = []; @@ -198,9 +208,8 @@ class Insertion { // Insert nodes collected in temporary DocumentFragment. this._insertPartialFragment(); - // After the node was inserted we may try to merge it with its siblings. - // This should happen only if it was the first and/or last of the nodes (so only with boundary nodes) - // and only if the selection was in those elements initially. + // After the content was inserted we may try to merge it with its siblings. + // This should happen only if the selection was in those elements initially. // // E.g.: //

x^

+

y

=>

x

y

=>

xy[]

@@ -287,8 +296,9 @@ class Insertion { } // Add node to the current temporary DocumentFragment. - this._insertToFragment( node ); + this._appendToFragment( node ); + // Store the first and last nodes for easy access for merging with sibling nodes. if ( !this._firstNode ) { this._firstNode = node; } @@ -297,7 +307,8 @@ class Insertion { } /** - * TODO + * Inserts the temporary DocumentFragment into the model. + * * @private */ _insertPartialFragment() { @@ -309,7 +320,17 @@ class Insertion { this._setAffectedBoundaries( this.position ); - this.writer.insert( this._documentFragment, this.position ); + // Insert the first node in a separate operation to avoid operation transformation on multiple nodes on undoing (insert + merge). + if ( this._documentFragment.getChild( 0 ) == this._firstNode ) { + this.writer.insert( this._firstNode, this.position ); + this.position = livePosition.toPosition(); + } + + // Insert the remaining nodes from document fragment. + if ( !this._documentFragment.isEmpty ) { + this.writer.insert( this._documentFragment, this.position ); + } + this._documentFragmentPosition = this.writer.createPositionAt( this._documentFragment, 0 ); this.position = livePosition.toPosition(); @@ -323,7 +344,7 @@ class Insertion { _handleObject( node ) { // Try finding it a place in the tree. if ( this._checkAndSplitToAllowedPosition( node ) ) { - this._insertToFragment( node ); + this._appendToFragment( node ); } // Try autoparagraphing. else { @@ -347,10 +368,12 @@ class Insertion { } /** + * Append a node to the temporary DocumentFragment. + * * @private * @param {module:engine/model/node~Node} node The node to insert. */ - _insertToFragment( node ) { + _appendToFragment( node ) { /* istanbul ignore if */ if ( !this.schema.checkChild( this.position, node ) ) { // Algorithm's correctness check. We should never end up here but it's good to know that we did. @@ -415,6 +438,8 @@ class Insertion { } /** + * Merges sibling of the first node if should be merged. + * * @private */ _mergeSiblingsOfFirstNode() { @@ -480,6 +505,8 @@ class Insertion { } /** + * Merges sibling of the last node if should be merged. + * * @private */ _mergeSiblingsOfLastNode() { diff --git a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js index 9ff474206c5..0e218894d54 100644 --- a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js @@ -241,6 +241,55 @@ describe( 'DataController utils', () => { expect( getData( model ) ).to.equal( 'fooxyz[]' ); } ); + it( 'should group multiple node inserts', () => { + model.schema.register( 'paragraph', { inheritAllFrom: '$block' } ); + + setData( model, 'f[]oo' ); + const affectedRange = insertHelper( + 'abc' + + 'def' + + 'ghi' + + 'jkl' + ); + + expect( getData( model ) ).to.equal( + 'fabc' + + 'def' + + 'ghi' + + 'jkl[]oo' + ); + expect( stringify( root, affectedRange ) ).to.equal( + 'f[abc' + + 'def' + + 'ghi' + + 'jkl]oo' + ); + + const batch = model.document.history.getOperation( model.document.version - 1 ).batch; + const operations = batch.operations.filter( operation => operation.isDocumentOperation ); + + expect( operations.length ).to.equal( 5 ); + + expect( operations[ 0 ].type ).to.equal( 'split' ); + expect( operations[ 0 ].splitPosition.path ).to.deep.equal( [ 0, 1 ] ); + + // First node should always be inserted by a separate operation (to avoid operation transformation + // on multiple nodes on undoing (insert + merge). + expect( operations[ 1 ].type ).to.equal( 'insert' ); + expect( operations[ 1 ].position.path ).to.deep.equal( [ 1 ] ); + expect( operations[ 1 ].nodes.length ).to.equal( 1 ); + + expect( operations[ 2 ].type ).to.equal( 'insert' ); + expect( operations[ 2 ].position.path ).to.deep.equal( [ 2 ] ); + expect( operations[ 2 ].nodes.length ).to.equal( 3 ); + + expect( operations[ 3 ].type ).to.equal( 'merge' ); + expect( operations[ 3 ].targetPosition.path ).to.deep.equal( [ 0, 1 ] ); + + expect( operations[ 4 ].type ).to.equal( 'merge' ); + expect( operations[ 4 ].targetPosition.path ).to.deep.equal( [ 3, 3 ] ); + } ); + describe( 'in simple scenarios', () => { beforeEach( () => { model = new Model(); diff --git a/packages/ckeditor5-typing/tests/input.js b/packages/ckeditor5-typing/tests/input.js index 7d92c78e0d5..7496b64c600 100644 --- a/packages/ckeditor5-typing/tests/input.js +++ b/packages/ckeditor5-typing/tests/input.js @@ -14,8 +14,6 @@ import ShiftEnter from '@ckeditor/ckeditor5-enter/src/shiftenter'; import Input from '../src/input'; import TextTransformation from '../src/texttransformation'; -import Writer from '@ckeditor/ckeditor5-engine/src/model/writer'; - import ViewText from '@ckeditor/ckeditor5-engine/src/view/text'; import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element'; import ViewContainerElement from '@ckeditor/ckeditor5-engine/src/view/containerelement'; @@ -323,9 +321,6 @@ describe( 'Input feature', () => { const viewSelection = view.createSelection(); viewSelection.setTo( viewRoot.getChild( 0 ).getChild( 0 ), 6 ); - testUtils.sinon.spy( Writer.prototype, 'insert' ); - testUtils.sinon.spy( Writer.prototype, 'remove' ); - viewDocument.fire( 'mutations', [ { type: 'text', @@ -336,8 +331,12 @@ describe( 'Input feature', () => { viewSelection ); - expect( Writer.prototype.insert.calledOnce ).to.be.true; - expect( Writer.prototype.remove.calledOnce ).to.be.true; + const batch = model.document.history.getOperation( model.document.version - 1 ).batch; + const operations = batch.operations.filter( operation => operation.isDocumentOperation ); + + expect( operations.length ).to.equal( 2 ); + expect( operations[ 0 ].type ).to.equal( 'remove' ); + expect( operations[ 1 ].type ).to.equal( 'insert' ); } ); it( 'should place selection after when correcting to longer word (spellchecker)', () => { From 4451fcdb7214eff99495ba6dd2783f7d6a138b78 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Fri, 15 Jan 2021 19:08:38 +0100 Subject: [PATCH 3/4] Fixed issue with ordering the OT operations. --- .../src/model/utils/insertcontent.js | 80 ++++++++++++++----- .../tests/model/utils/insertcontent.js | 10 +-- 2 files changed, 65 insertions(+), 25 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.js b/packages/ckeditor5-engine/src/model/utils/insertcontent.js index 9a4047a7262..cb2e647a8cc 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.js @@ -208,19 +208,9 @@ class Insertion { // Insert nodes collected in temporary DocumentFragment. this._insertPartialFragment(); - // After the content was inserted we may try to merge it with its siblings. - // This should happen only if the selection was in those elements initially. - // - // E.g.: - //

x^

+

y

=>

x

y

=>

xy[]

- // and: - //

x^y

+

z

=>

x

^

y

+

z

=>

x

z

y

=>

xz[]y

- // but: - //

x

^

z

+

y

=>

x

y

z

(no merging) - //

x

[]

z

+

y

=>

x

y

z

(no merging, note: after running deleteContents - // it's exactly the same case as above) - this._mergeSiblingsOfFirstNode(); - this._mergeSiblingsOfLastNode(); + // After the content was inserted we may try to merge it with its next sibling if the selection was in it initially. + // Merging with the previous sibling was performed just after inserting the first node to the document. + this._mergeOnRight(); // TMP this will become a post-fixer. this.schema.removeDisallowedAttributes( this._filterAttributesOf, this.writer ); @@ -320,9 +310,16 @@ class Insertion { this._setAffectedBoundaries( this.position ); - // Insert the first node in a separate operation to avoid operation transformation on multiple nodes on undoing (insert + merge). + // If the very first node of the whole insertion process is inserted, insert it separately for OT reasons (undo). + // Note: there can be multiple calls to `_insertPartialFragment()` during one insertion process. + // Note: only the very first node can be merged so we have to do separate operation only for it. if ( this._documentFragment.getChild( 0 ) == this._firstNode ) { this.writer.insert( this._firstNode, this.position ); + + // We must merge the first node just after inserting it to avoid problems with OT. + // (See: https://github.com/ckeditor/ckeditor5/pull/8773#issuecomment-760945652). + this._mergeOnLeft(); + this.position = livePosition.toPosition(); } @@ -440,9 +437,20 @@ class Insertion { /** * Merges sibling of the first node if should be merged. * + * After the content was inserted we may try to merge it with its siblings. + * This should happen only if the selection was in those elements initially. + * + * Example: + *

x^

+

y

=>

x

y

=>

xy[]

+ * and: + *

x^y

+

z

=>

x

^

y

+

z

=>

x

z

y

=>

xz[]y

+ * but: + *

x

^

z

+

y

=>

x

y

z

(no merging) + *

x

[]

z

+

y

=>

x

y

z

(no merging, note: after running deleteContents + * it's exactly the same case as above) * @private */ - _mergeSiblingsOfFirstNode() { + _mergeOnLeft() { const node = this._firstNode; if ( !( node instanceof Element ) ) { @@ -475,8 +483,18 @@ class Insertion { this._affectedStart = LivePosition._createAt( mergePosLeft.nodeBefore, 'end', 'toPrevious' ); } + // We need to update the references to the first and last nodes if they will be merged into the previous sibling node + // because the reference would point to the removed node. + // + //

A^A

+

X

+ // + //

A

^

A

+ //

A

X

A

+ //

AX

A

+ //

AXA

if ( this._firstNode === this._lastNode ) { - this._firstNode = this._lastNode = mergePosLeft.nodeBefore; + this._firstNode = mergePosLeft.nodeBefore; + this._lastNode = mergePosLeft.nodeBefore; } this.writer.merge( mergePosLeft ); @@ -507,9 +525,20 @@ class Insertion { /** * Merges sibling of the last node if should be merged. * + * After the content was inserted we may try to merge it with its siblings. + * This should happen only if the selection was in those elements initially. + * + * Example: + *

x^

+

y

=>

x

y

=>

xy[]

+ * and: + *

x^y

+

z

=>

x

^

y

+

z

=>

x

z

y

=>

xz[]y

+ * but: + *

x

^

z

+

y

=>

x

y

z

(no merging) + *

x

[]

z

+

y

=>

x

y

z

(no merging, note: after running deleteContents + * it's exactly the same case as above) * @private */ - _mergeSiblingsOfLastNode() { + _mergeOnRight() { const node = this._lastNode; if ( !( node instanceof Element ) ) { @@ -544,23 +573,34 @@ class Insertion { //

x

[]

y

=>

x[]

y

this.position = Position._createAt( mergePosRight.nodeBefore, 'end' ); + // Explanation of setting position stickiness to `'toPrevious'`: // OK:

xx[]

+

yy

=>

xx[]yy

(when sticks to previous) // NOK:

xx[]

+

yy

=>

xxyy[]

(when sticks to next) const livePosition = LivePosition.fromPosition( this.position, 'toPrevious' ); - // See comment above on moving `_affectedStart`. + // See comment in `_mergeOnLeft()` on moving `_affectedStart`. if ( this._affectedEnd.isEqual( mergePosRight ) ) { this._affectedEnd.detach(); this._affectedEnd = LivePosition._createAt( mergePosRight.nodeBefore, 'end', 'toNext' ); } + // We need to update the references to the first and last nodes if they will be merged into the previous sibling node + // because the reference would point to the removed node. + // + //

A^A

+

X

+ // + //

A

^

A

+ //

A

X

A

+ //

AX

A

+ //

AXA

if ( this._firstNode === this._lastNode ) { - this._firstNode = this._lastNode = mergePosRight.nodeBefore; + this._firstNode = mergePosRight.nodeBefore; + this._lastNode = mergePosRight.nodeBefore; } this.writer.merge( mergePosRight ); - // See comment above on moving `_affectedStart`. + // See comment in `_mergeOnLeft()` on moving `_affectedStart`. if ( mergePosRight.getShiftedBy( -1 ).isEqual( this._affectedStart ) && this._firstNode === this._lastNode ) { this._affectedStart.detach(); this._affectedStart = LivePosition._createAt( mergePosRight.nodeBefore, 0, 'toPrevious' ); diff --git a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js index 0e218894d54..280f2452b22 100644 --- a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js @@ -279,12 +279,12 @@ describe( 'DataController utils', () => { expect( operations[ 1 ].position.path ).to.deep.equal( [ 1 ] ); expect( operations[ 1 ].nodes.length ).to.equal( 1 ); - expect( operations[ 2 ].type ).to.equal( 'insert' ); - expect( operations[ 2 ].position.path ).to.deep.equal( [ 2 ] ); - expect( operations[ 2 ].nodes.length ).to.equal( 3 ); + expect( operations[ 2 ].type ).to.equal( 'merge' ); + expect( operations[ 2 ].targetPosition.path ).to.deep.equal( [ 0, 1 ] ); - expect( operations[ 3 ].type ).to.equal( 'merge' ); - expect( operations[ 3 ].targetPosition.path ).to.deep.equal( [ 0, 1 ] ); + expect( operations[ 3 ].type ).to.equal( 'insert' ); + expect( operations[ 3 ].position.path ).to.deep.equal( [ 1 ] ); + expect( operations[ 3 ].nodes.length ).to.equal( 3 ); expect( operations[ 4 ].type ).to.equal( 'merge' ); expect( operations[ 4 ].targetPosition.path ).to.deep.equal( [ 3, 3 ] ); From 3644952d555dae017acd63d68c476c70bf38b852 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 15 Jan 2021 22:33:32 +0100 Subject: [PATCH 4/4] Docs: Slight changes in API docs. --- .../src/model/utils/insertcontent.js | 33 ++++++++----------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.js b/packages/ckeditor5-engine/src/model/utils/insertcontent.js index cb2e647a8cc..c5e6eaec024 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.js @@ -16,8 +16,17 @@ import Selection from '../selection'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** - * Inserts content into the editor (specified selection) as one would expect the paste - * functionality to work. + * Inserts content into the editor (specified selection) as one would expect the paste functionality to work. + * + * It takes care of removing the selected content, splitting elements (if needed), inserting elements and merging elements appropriately. + * + * Some examples: + * + *

x^

+

y

=>

x

y

=>

xy[]

+ *

x^y

+

z

=>

x

^

y

+

z

=>

x

z

y

=>

xz[]y

+ *

x^y

+ =>

x

^

y

+ =>

x

y

+ *

x

^

z

+

y

=>

x

y[]

z

(no merging) + *

x

[]

z

+

y

=>

x

^

z

+

y

=>

x

y[]

z

* * If an instance of {@link module:engine/model/selection~Selection} is passed as `selectable` it will be modified * to the insertion selection (equal to a range to be selected after insertion). @@ -435,19 +444,11 @@ class Insertion { } /** - * Merges sibling of the first node if should be merged. + * Merges the previous sibling of the first node if it should be merged. * * After the content was inserted we may try to merge it with its siblings. * This should happen only if the selection was in those elements initially. * - * Example: - *

x^

+

y

=>

x

y

=>

xy[]

- * and: - *

x^y

+

z

=>

x

^

y

+

z

=>

x

z

y

=>

xz[]y

- * but: - *

x

^

z

+

y

=>

x

y

z

(no merging) - *

x

[]

z

+

y

=>

x

y

z

(no merging, note: after running deleteContents - * it's exactly the same case as above) * @private */ _mergeOnLeft() { @@ -523,19 +524,11 @@ class Insertion { } /** - * Merges sibling of the last node if should be merged. + * Merges the next sibling of the last node if it should be merged. * * After the content was inserted we may try to merge it with its siblings. * This should happen only if the selection was in those elements initially. * - * Example: - *

x^

+

y

=>

x

y

=>

xy[]

- * and: - *

x^y

+

z

=>

x

^

y

+

z

=>

x

z

y

=>

xz[]y

- * but: - *

x

^

z

+

y

=>

x

y

z

(no merging) - *

x

[]

z

+

y

=>

x

y

z

(no merging, note: after running deleteContents - * it's exactly the same case as above) * @private */ _mergeOnRight() {