From 74d1d15e819524914cedf9d6d9a799368d137437 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Fri, 29 Jan 2021 21:44:32 +0100 Subject: [PATCH 1/2] Model#insertContent() should properly auto-paragraph nodes. --- .../src/model/utils/insertcontent.js | 101 +++++++++++++++++- .../tests/model/utils/insertcontent.js | 43 ++++---- 2 files changed, 118 insertions(+), 26 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.js b/packages/ckeditor5-engine/src/model/utils/insertcontent.js index 9bbe45055e4..76b7290cdad 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.js @@ -179,6 +179,14 @@ class Insertion { */ this._lastNode = null; + /** + * The reference to the last auto paragraph node. + * + * @private + * @type {module:engine/model/node~Node} + */ + this._lastAutoParagraph = null; + /** * The array of nodes that should be cleaned of not allowed attributes. * @@ -217,6 +225,11 @@ class Insertion { // Insert nodes collected in temporary DocumentFragment. this._insertPartialFragment(); + // If there was an auto paragraph then we might need to adjust the end of insertion. + if ( this._lastAutoParagraph ) { + this._updateLastNodeFromAutoParagraph( this._lastAutoParagraph ); + } + // 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(); @@ -226,6 +239,41 @@ class Insertion { this._filterAttributesOf = []; } + /** + * Updates the last node after the auto paragraphing. + * + * @private + * @param {module:engine/model/node~Node} node The last auto paragraphing node. + */ + _updateLastNodeFromAutoParagraph( node ) { + const positionAfterLastNode = this.writer.createPositionAfter( this._lastNode ); + const positionAfterNode = this.writer.createPositionAfter( node ); + + // If the real end was after the last auto paragraph then update relevant properties. + if ( positionAfterNode.isAfter( positionAfterLastNode ) ) { + this._lastNode = node; + + /* istanbul ignore if */ + if ( this.position.parent != node || !this.position.isAtEnd ) { + // 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 at the end of the last auto paragraph. + /** + * 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 ); + } + + this.position = positionAfterNode; + this._setAffectedBoundaries( this.position ); + } + } + /** * Returns range to be selected after insertion. * Returns `null` if there is no valid range to select after insertion. @@ -284,14 +332,21 @@ 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 ); + + // Check if a node can be inserted in the given position or it would be accepted if a paragraph would be inserted. + // Inserts the auto paragraph if it would allow for insertion. + let isAllowed = this._checkAndAutoParagraphToAllowedPosition( node ); if ( !isAllowed ) { - this._handleDisallowedNode( 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. + isAllowed = this._checkAndSplitToAllowedPosition( node ); - return; + if ( !isAllowed ) { + this._handleDisallowedNode( node ); + + return; + } } // Add node to the current temporary DocumentFragment. @@ -657,6 +712,42 @@ class Insertion { } } + /** + * Checks if a node can be inserted in the given position or it would be accepted if a paragraph would be inserted. + * It also handles inserting the paragraph. + * + * @private + * @param {module:engine/model/node~Node} node The node. + * @returns {Boolean} Whether an allowed position was found. + * `false` is returned if the node isn't allowed at the current position or in auto paragraph, `true` if was. + */ + _checkAndAutoParagraphToAllowedPosition( node ) { + if ( this.schema.checkChild( this.position.parent, node ) ) { + return true; + } + + // Do not auto paragraph if the paragraph won't be allowed there, + // cause that would lead to an infinite loop. The paragraph would be rejected in + // the next _handleNode() call and we'd be here again. + if ( !this.schema.checkChild( this.position.parent, 'paragraph' ) || !this.schema.checkChild( 'paragraph', node ) ) { + return false; + } + + // Insert nodes collected in temporary DocumentFragment if the position parent needs change to process further nodes. + this._insertPartialFragment(); + + // Insert a paragraph and move insertion position to it. + const paragraph = this.writer.createElement( 'paragraph' ); + + this.writer.insert( paragraph, this.position ); + this._setAffectedBoundaries( this.position ); + + this._lastAutoParagraph = paragraph; + this.position = this.writer.createPositionAt( paragraph, 0 ); + + return true; + } + /** * @private * @param {module:engine/model/node~Node} node diff --git a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js index 517794ff08f..b57dd665a1a 100644 --- a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js @@ -465,6 +465,7 @@ describe( 'DataController utils', () => { inheritAllFrom: '$block', allowAttributes: [ 'listType', 'listIndent' ] } ); + schema.extend( '$text', { allowAttributes: 'foo' } ); } ); it( 'inserts one text node', () => { @@ -838,31 +839,14 @@ describe( 'DataController utils', () => { expect( stringify( root, affectedRange ) ).to.equal( 'f[yyyxxx]oo' ); } ); - // This is the expected result, but it was so hard to achieve at this stage that I - // decided to go with the what the next test represents. - // it( 'inserts paragraph + text + inlineWidget + text', () => { - // setData( model, 'f[]oo' ); - // insertHelper( 'yyyxxxzzz' ); - // expect( getData( model ) ) - // .to.equal( 'fyyyxxxzzz[]oo' ); - // } ); - - // See the comment above. it( 'inserts paragraph + text + inlineWidget + text', () => { setData( model, 'f[]oo' ); const affectedRange = insertHelper( 'yyyxxxzzz' ); - expect( getData( model ) ).to.equal( - 'fyyyxxx' + - '' + - 'zzz[]oo' - ); - - expect( stringify( root, affectedRange ) ).to.equal( - 'f[yyyxxx' + - '' + - 'zzz]oo' - ); + expect( getData( model ) ) + .to.equal( 'fyyyxxxzzz[]oo' ); + expect( stringify( root, affectedRange ) ) + .to.equal( 'f[yyyxxxzzz]oo' ); } ); it( 'inserts paragraph + text + paragraph', () => { @@ -1058,6 +1042,23 @@ describe( 'DataController utils', () => { 'foo[]bar' ); } ); + + it( 'inserts multiple text nodes with different attribute values', () => { + setData( model, 'foo[]bar' ); + const affectedRange = insertHelper( '<$text foo="a">yyy<$text foo="b">xxx' ); + + expect( getData( model ) ).to.equal( + 'foo' + + '<$text foo="a">yyy<$text foo="b">xxx[]' + + 'bar' + ); + + expect( stringify( root, affectedRange ) ).to.equal( + 'foo' + + '[<$text foo="a">yyy<$text foo="b">xxx]' + + 'bar' + ); + } ); } ); describe( 'content over an inline object', () => { From e2cece415a715b9918da88d8f7651f0b11de5617 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sun, 31 Jan 2021 23:59:02 +0100 Subject: [PATCH 2/2] Removed duplicated error doc. --- .../ckeditor5-engine/src/model/utils/insertcontent.js | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.js b/packages/ckeditor5-engine/src/model/utils/insertcontent.js index 76b7290cdad..aa4f7da2e95 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.js @@ -257,15 +257,7 @@ class Insertion { if ( this.position.parent != node || !this.position.isAtEnd ) { // 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 at the end of the last auto paragraph. - /** - * 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 - */ + // Note: This error is documented in other place in this file. throw new CKEditorError( 'insertcontent-invalid-insertion-position', this ); }