Skip to content

Commit

Permalink
Merge pull request #16704 from ckeditor/ck/16321
Browse files Browse the repository at this point in the history 
Fix (engine): `insertcontent-invalid-insertion-position` exception is no longer thrown after pasting content containing `block element` + `non-paragraph` + `block element` elements. Closes #16321.

Other (engine): The `Model#insertContent()` method should keep inline objects in the same auto-paragraph as text nodes and other inline objects. See #16321.

Other (engine): The `Schema#checkMerge()` method should return `false` if one of the elements is a limit element. See #16321.
  • Loading branch information
@niegowski
niegowski authored Jul 17, 2024
2 parents 448f596 + c08d7d8 commit 34cf600
Show file tree
Hide file tree
Showing 5 changed files with 439 additions and 96 deletions.
14 changes: 11 additions & 3 deletions packages/ckeditor5-engine/src/model/schema.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -418,10 +418,14 @@ export default class Schema extends /* #__PURE__ */ ObservableMixin() {
return def.allowAttributes.includes( attributeName );
}

public checkMerge( position: Position ): boolean;
public checkMerge( baseElement: Element, elementToMerge: Element ): boolean;

/**
* Checks whether the given element (`elementToMerge`) can be merged with the specified base element (`positionOrBaseElement`).
*
* In other words – whether `elementToMerge`'s children {@link #checkChild are allowed} in the `positionOrBaseElement`.
* In other words – both elements are not a limit elements and whether `elementToMerge`'s children
* {@link #checkChild are allowed} in the `positionOrBaseElement`.
*
* This check ensures that elements merged with {@link module:engine/model/writer~Writer#merge `Writer#merge()`}
* will be valid.
Expand All @@ -432,7 +436,7 @@ export default class Schema extends /* #__PURE__ */ ObservableMixin() {
* @param positionOrBaseElement The position or base element to which the `elementToMerge` will be merged.
* @param elementToMerge The element to merge. Required if `positionOrBaseElement` is an element.
*/
public checkMerge( positionOrBaseElement: Position | Element, elementToMerge: Element ): boolean {
public checkMerge( positionOrBaseElement: Position | Element, elementToMerge?: Element ): boolean {
if ( positionOrBaseElement instanceof Position ) {
const nodeBefore = positionOrBaseElement.nodeBefore;
const nodeAfter = positionOrBaseElement.nodeAfter;
Expand Down Expand Up @@ -464,7 +468,11 @@ export default class Schema extends /* #__PURE__ */ ObservableMixin() {
return this.checkMerge( nodeBefore, nodeAfter );
}

for ( const child of elementToMerge.getChildren() ) {
if ( this.isLimit( positionOrBaseElement ) || this.isLimit( elementToMerge! ) ) {
return false;
}

for ( const child of elementToMerge!.getChildren() ) {
if ( !this.checkChild( positionOrBaseElement, child ) ) {
return false;
}
Expand Down
101 changes: 24 additions & 77 deletions packages/ckeditor5-engine/src/model/utils/insertcontent.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,31 +408,15 @@ class Insertion {
* Handles insertion of a single node.
*/
private _handleNode( node: Node ): void {
// 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 as Element );

return;
}

// Try to find a place for the given 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 ) {
// 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 );

if ( !isAllowed ) {
// 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.
if ( !this._checkAndSplitToAllowedPosition( node ) ) {
// Handle element children if it's not an object (strip container).
if ( !this.schema.isObject( node ) ) {
this._handleDisallowedNode( node );

return;
}

return;
}

// Add node to the current temporary DocumentFragment.
Expand Down Expand Up @@ -482,20 +466,6 @@ class Insertion {
livePosition.detach();
}

/**
* @param node The object element.
*/
private _handleObject( node: Element ): void {
// Try finding it a place in the tree.
if ( this._checkAndSplitToAllowedPosition( node ) ) {
this._appendToFragment( node );
}
// Try autoparagraphing.
else {
this._tryAutoparagraphing( node );
}
}

/**
* @param node The disallowed node which needs to be handled.
*/
Expand All @@ -504,10 +474,6 @@ class Insertion {
if ( node.is( 'element' ) ) {
this.handleNodes( node.getChildren() );
}
// If text is not allowed, try autoparagraphing it.
else {
this._tryAutoparagraphing( node );
}
}

/**
Expand Down Expand Up @@ -765,41 +731,9 @@ class Insertion {
}

/**
* Tries wrapping the node in a new paragraph and inserting it this way.
*
* @param node The node which needs to be autoparagraphed.
*/
private _tryAutoparagraphing( node: Node ): void {
const paragraph = this.writer.createElement( 'paragraph' );

// Do not autoparagraph 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._getAllowedIn( this.position.parent as any, paragraph ) && this.schema.checkChild( paragraph, node ) ) {
paragraph._appendChild( node );
this._handleNode( paragraph );
}
}

/**
* 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.
*
* @returns 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.
* Inserts a paragraph and moves the insertion position into it.
*/
private _checkAndAutoParagraphToAllowedPosition( node: Node ): boolean {
if ( this.schema.checkChild( this.position.parent as any, 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 as any, 'paragraph' ) || !this.schema.checkChild( 'paragraph', node ) ) {
return false;
}

private _insertAutoParagraph(): void {
// Insert nodes collected in temporary DocumentFragment if the position parent needs change to process further nodes.
this._insertPartialFragment();

Expand All @@ -811,8 +745,6 @@ class Insertion {

this._lastAutoParagraph = paragraph;
this.position = this.writer.createPositionAt( paragraph, 0 );

return true;
}

/**
Expand Down Expand Up @@ -867,20 +799,35 @@ class Insertion {
}
}

// At this point, we split elements up to the parent in which `node` is allowed.
// Note that `_getAllowedIn()` checks if the `node` is allowed either directly, or when auto-paragraphed.
// So, let's check if the `node` is allowed directly. If not, we need to auto-paragraph it.
if ( !this.schema.checkChild( this.position.parent, node ) ) {
this._insertAutoParagraph();
}

return true;
}

/**
* Gets the element in which the given node is allowed. It checks the passed element and all its ancestors.
*
* It also verifies if auto-paragraphing could help.
*
* @param contextElement The element in which context the node should be checked.
* @param childNode The node to check.
*/
private _getAllowedIn( contextElement: Element, childNode: Node ): Element | null {
// Check if a node can be inserted in the given context...
if ( this.schema.checkChild( contextElement, childNode ) ) {
return contextElement;
}

// ...or it would be accepted if a paragraph would be inserted.
if ( this.schema.checkChild( contextElement, 'paragraph' ) && this.schema.checkChild( 'paragraph', childNode ) ) {
return contextElement;
}

// If the child wasn't allowed in the context element and the element is a limit there's no point in
// checking any further towards the root. This is it: the limit is unsplittable and there's nothing
// we can do about it. Without this check, the algorithm will analyze parent of the limit and may create
Expand Down
39 changes: 39 additions & 0 deletions packages/ckeditor5-engine/tests/model/schema.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,17 @@ describe( 'Schema', () => {
allowWhere: '$block',
allowContentOf: '$root'
} );
schema.register( 'blockObject', {
allowWhere: '$block',
isBlock: true,
isObject: true
} );
schema.register( 'inlineObject', {
allowWhere: '$text',
allowAttributesOf: '$text',
isInline: true,
isObject: true
} );
} );

it( 'returns false if a block cannot be merged with other block (disallowed element is the first child)', () => {
Expand Down Expand Up @@ -980,6 +991,34 @@ describe( 'Schema', () => {
expect( schema.checkMerge( position ) ).to.be.true;
} );

it( 'return false if elements on the left is a block object', () => {
const left = new Element( 'blockObject' );
const right = new Element( 'paragraph' );

expect( schema.checkMerge( left, right ) ).to.be.false;
} );

it( 'return false if elements on the right is a block object', () => {
const left = new Element( 'paragraph' );
const right = new Element( 'blockObject' );

expect( schema.checkMerge( left, right ) ).to.be.false;
} );

it( 'return false if both elements are block objects', () => {
const left = new Element( 'blockObject' );
const right = new Element( 'blockObject' );

expect( schema.checkMerge( left, right ) ).to.be.false;
} );

it( 'return false if both elements are inline objects', () => {
const left = new Element( 'inlineObject' );
const right = new Element( 'inlineObject' );

expect( schema.checkMerge( left, right ) ).to.be.false;
} );

it( 'throws an error if there is no element before the position', () => {
const listItem = new Element( 'listItem', null, [
new Text( 'foo' )
Expand Down
Loading

0 comments on commit 34cf600

Please sign in to comment.