Skip to content

Commit

Permalink
Merge pull request #8958 from ckeditor/i/8953
Browse files Browse the repository at this point in the history 
Fix (engine): Pasting formatted single-line text over a widget should not split it into multiple paragraphs. Closes #8953.
  • Loading branch information
@scofalik
scofalik authored Feb 2, 2021
2 parents 8a276bd + 21d3463 commit dfe8035
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 26 deletions.
93 changes: 88 additions & 5 deletions packages/ckeditor5-engine/src/model/utils/insertcontent.js
View file
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 );
}

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
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

0 comments on commit dfe8035

Please sign in to comment.