Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model#insertContent() should properly auto-paragraph nodes #8958

Merged
merged 3 commits into from
Feb 2, 2021
Merged

Model#insertContent() should properly auto-paragraph nodes #8958

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
74d1d15
Model#insertContent() should properly auto-paragraph nodes.
niegowski Jan 29, 2021
e2cece4
Removed duplicated error doc.
niegowski Jan 31, 2021
21d3463
Merge branch 'master' into i/8953
niegowski Feb 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 88 additions & 5 deletions packages/ckeditor5-engine/src/model/utils/insertcontent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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();
Expand All @@ -226,6 +239,33 @@ 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.
// Note: This error is documented in other place in this file.
throw new CKEditorError( 'insertcontent-invalid-insertion-position', this );
Comment on lines +257 to +261
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clone of the previous check that is in _mergeOnRight() but this is a slightly different scenario.

}

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.
Expand Down Expand Up @@ -284,14 +324,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.
Expand Down Expand Up @@ -657,6 +704,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
Expand Down
43 changes: 22 additions & 21 deletions packages/ckeditor5-engine/tests/model/utils/insertcontent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,7 @@ describe( 'DataController utils', () => {
inheritAllFrom: '$block',
allowAttributes: [ 'listType', 'listIndent' ]
} );
schema.extend( '$text', { allowAttributes: 'foo' } );
} );

it( 'inserts one text node', () => {
Expand Down Expand Up @@ -838,31 +839,14 @@ describe( 'DataController utils', () => {
expect( stringify( root, affectedRange ) ).to.equal( '<paragraph>f[yyy</paragraph><paragraph>xxx]oo</paragraph>' );
} );

// 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, '<paragraph>f[]oo</paragraph>' );
// insertHelper( '<paragraph>yyy</paragraph>xxx<inlineWidget></inlineWidget>zzz' );
// expect( getData( model ) )
// .to.equal( '<paragraph>fyyy</paragraph><paragraph>xxx<inlineWidget></inlineWidget>zzz[]oo</paragraph>' );
// } );

// See the comment above.
it( 'inserts paragraph + text + inlineWidget + text', () => {
setData( model, '<paragraph>f[]oo</paragraph>' );
const affectedRange = insertHelper( '<paragraph>yyy</paragraph>xxx<inlineWidget></inlineWidget>zzz' );

expect( getData( model ) ).to.equal(
'<paragraph>fyyy</paragraph><paragraph>xxx</paragraph>' +
'<paragraph><inlineWidget></inlineWidget></paragraph>' +
'<paragraph>zzz[]oo</paragraph>'
);

expect( stringify( root, affectedRange ) ).to.equal(
'<paragraph>f[yyy</paragraph><paragraph>xxx</paragraph>' +
'<paragraph><inlineWidget></inlineWidget></paragraph>' +
'<paragraph>zzz]oo</paragraph>'
);
expect( getData( model ) )
.to.equal( '<paragraph>fyyy</paragraph><paragraph>xxx<inlineWidget></inlineWidget>zzz[]oo</paragraph>' );
expect( stringify( root, affectedRange ) )
.to.equal( '<paragraph>f[yyy</paragraph><paragraph>xxx<inlineWidget></inlineWidget>zzz]oo</paragraph>' );
} );

it( 'inserts paragraph + text + paragraph', () => {
Expand Down Expand Up @@ -1058,6 +1042,23 @@ describe( 'DataController utils', () => {
'<paragraph>foo</paragraph>[<paragraph><inlineWidget></inlineWidget></paragraph>]<paragraph>bar</paragraph>'
);
} );

it( 'inserts multiple text nodes with different attribute values', () => {
setData( model, '<paragraph>foo</paragraph>[<blockWidget></blockWidget>]<paragraph>bar</paragraph>' );
const affectedRange = insertHelper( '<$text foo="a">yyy</$text><$text foo="b">xxx</$text>' );

expect( getData( model ) ).to.equal(
'<paragraph>foo</paragraph>' +
'<paragraph><$text foo="a">yyy</$text><$text foo="b">xxx[]</$text></paragraph>' +
'<paragraph>bar</paragraph>'
);

expect( stringify( root, affectedRange ) ).to.equal(
'<paragraph>foo</paragraph>' +
'[<paragraph><$text foo="a">yyy</$text><$text foo="b">xxx</$text></paragraph>]' +
'<paragraph>bar</paragraph>'
);
} );
} );

describe( 'content over an inline object', () => {
Expand Down