From 28fb67187f83f820cf895c39c98c0773eb260ce1 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Wed, 10 Jul 2024 12:29:27 +0200 Subject: [PATCH 1/6] Drop `insertcontent-invalid-insertion-position` check --- .../src/model/utils/insertcontent.ts | 9 --------- .../tests/model/utils/insertcontent.js | 14 ++++++++++++++ .../ckeditor5-image/tests/manual/imageblock.html | 4 +++- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts index c4750516338..9b5c77f830d 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts @@ -353,15 +353,6 @@ class Insertion { // If the real end was after the last auto paragraph then update relevant properties. if ( positionAfterNode.isAfter( positionAfterLastNode ) ) { this._lastNode = node; - - /* istanbul ignore if -- @preserve */ - 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 ); } diff --git a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js index 1715a2f2680..52c5d09f39a 100644 --- a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js @@ -882,6 +882,20 @@ describe( 'DataController utils', () => { ); } ); + it( 'inserts object + text + object', () => { + setData( model, 'foo[]' ); + + const affectedRange = insertHelper( 'foo' ); + + expect( getData( model ) ).to.equal( + 'foofoo[]' + ); + + expect( stringify( root, affectedRange ) ).to.equal( + 'foo[foo]' + ); + } ); + it( 'inserts text + object (at the end)', () => { setData( model, 'foo[]' ); const affectedRange = insertHelper( 'xxx' ); diff --git a/packages/ckeditor5-image/tests/manual/imageblock.html b/packages/ckeditor5-image/tests/manual/imageblock.html index 5b0c45ec78e..bea8c9382c5 100644 --- a/packages/ckeditor5-image/tests/manual/imageblock.html +++ b/packages/ckeditor5-image/tests/manual/imageblock.html @@ -3,7 +3,9 @@ - + +
+

Inline image

Lorem ipsum dolor sit amet, consectetur adipiscing elit.

From a1aab67ed0c511ffd55d02b58bf9ae51e09fa84b Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 15 Jul 2024 18:23:50 +0200 Subject: [PATCH 2/6] Schema should not allow merging limit element to any other element. InsertContent helper should not merge inner inserted paragraph with an outer one. --- packages/ckeditor5-engine/src/model/schema.ts | 15 ++++- .../src/model/utils/insertcontent.ts | 43 ++++++++------ .../tests/model/utils/insertcontent.js | 56 ++++++++++++++++--- .../tests/manual/imageblock.html | 4 +- 4 files changed, 86 insertions(+), 32 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/schema.ts b/packages/ckeditor5-engine/src/model/schema.ts index e045a09d843..cd30e48f252 100644 --- a/packages/ckeditor5-engine/src/model/schema.ts +++ b/packages/ckeditor5-engine/src/model/schema.ts @@ -418,6 +418,9 @@ 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`). * @@ -432,7 +435,11 @@ 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 ( elementToMerge && this.isLimit( elementToMerge ) ) { + return false; + } + if ( positionOrBaseElement instanceof Position ) { const nodeBefore = positionOrBaseElement.nodeBefore; const nodeAfter = positionOrBaseElement.nodeAfter; @@ -464,7 +471,11 @@ export default class Schema extends /* #__PURE__ */ ObservableMixin() { return this.checkMerge( nodeBefore, nodeAfter ); } - for ( const child of elementToMerge.getChildren() ) { + if ( this.isLimit( positionOrBaseElement ) ) { + return false; + } + + for ( const child of elementToMerge!.getChildren() ) { if ( !this.checkChild( positionOrBaseElement, child ) ) { return false; } diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts index 9b5c77f830d..2e4a9de15aa 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts @@ -353,6 +353,15 @@ class Insertion { // If the real end was after the last auto paragraph then update relevant properties. if ( positionAfterNode.isAfter( positionAfterLastNode ) ) { this._lastNode = node; + + /* istanbul ignore if -- @preserve */ + 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 ); } @@ -404,30 +413,28 @@ class Insertion { // * 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 ); + } else { + // Try to find a place for the given node. - 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 ); + // 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. - this._appendToFragment( node ); + // Add node to the current temporary DocumentFragment. + this._appendToFragment( node ); + } // Store the first and last nodes for easy access for merging with sibling nodes. if ( !this._firstNode ) { diff --git a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js index 52c5d09f39a..98d47c10a37 100644 --- a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js @@ -1754,11 +1754,49 @@ describe( 'DataController utils', () => { expect( expectedMarker.getRange().end.path ).to.deep.equal( [ 0, 1 ] ); } ); + it( 'should create marker after inserting paragraph with imageInline and text inside existing paragraph', () => { + // F[]oo + // {ba}r + // + // F{ba}roo + setData( model, 'F[]oo' ); + + insertHelper( 'bar', { + 'marker-a': { start: [ 0 ], end: [ 0, 3 ] } + } ); + + const expectedMarker = model.markers.get( 'marker-a' ); + + expect( getData( model ) ).to.equal( 'Fbar[]oo' ); + + expect( expectedMarker.getRange().start.path ).to.deep.equal( [ 0, 1 ] ); + expect( expectedMarker.getRange().end.path ).to.deep.equal( [ 0, 4 ] ); + } ); + + it( 'should create marker after inserting paragraph with imageInline and another paragraph inside existing paragraph', () => { + // F[]oo + // {ba}r + // + // F{ba}roo + setData( model, 'F[]oo' ); + + insertHelper( 'bar', { + 'marker-a': { start: [ 0 ], end: [ 1, 2 ] } + } ); + + const expectedMarker = model.markers.get( 'marker-a' ); + + expect( getData( model ) ).to.equal( 'Fbar[]oo' ); + + expect( expectedMarker.getRange().start.path ).to.deep.equal( [ 0, 1 ] ); + expect( expectedMarker.getRange().end.path ).to.deep.equal( [ 1, 2 ] ); + } ); + it( 'should create marker after inserting imageInline and paragraph inside existing paragraph', () => { // F[]oo // {ba}r // - // F{ba}roo + // F{ba}roo setData( model, 'F[]oo' ); insertHelper( 'bar', { @@ -1767,10 +1805,10 @@ describe( 'DataController utils', () => { const expectedMarker = model.markers.get( 'marker-a' ); - expect( getData( model ) ).to.equal( 'Fbar[]oo' ); + expect( getData( model ) ).to.equal( 'Fbar[]oo' ); expect( expectedMarker.getRange().start.path ).to.deep.equal( [ 0, 1 ] ); - expect( expectedMarker.getRange().end.path ).to.deep.equal( [ 0, 4 ] ); + expect( expectedMarker.getRange().end.path ).to.deep.equal( [ 1, 2 ] ); } ); it( 'should create marker which starts in paragraph and ends inside the beginning of next paragraph', () => { @@ -1796,7 +1834,7 @@ describe( 'DataController utils', () => { // Foo[] // {ba}r // - // Foo{ba}r + // Foo{ba}r setData( model, 'Foo[]' ); insertHelper( 'bar', { @@ -1805,10 +1843,10 @@ describe( 'DataController utils', () => { const expectedMarker = model.markers.get( 'marker-a' ); - expect( getData( model ) ).to.equal( 'Foobar[]' ); + expect( getData( model ) ).to.equal( 'Foobar[]' ); expect( expectedMarker.getRange().start.path ).to.deep.equal( [ 0, 3 ] ); - expect( expectedMarker.getRange().end.path ).to.deep.equal( [ 0, 6 ] ); + expect( expectedMarker.getRange().end.path ).to.deep.equal( [ 1, 2 ] ); } ); it( 'should create marker from imageInline and part of text from the next paragraph after inserting at he beginning', @@ -1816,7 +1854,7 @@ describe( 'DataController utils', () => { // []Foo // {ba}r // - // {ba}rFoo + // {ba}rFoo setData( model, '[]Foo' ); insertHelper( 'bar', { @@ -1826,10 +1864,10 @@ describe( 'DataController utils', () => { const expectedMarker = model.markers.get( 'marker-a' ); expect( getData( model ) ) - .to.equal( 'bar[]Foo' ); + .to.equal( 'bar[]Foo' ); expect( expectedMarker.getRange().start.path ).to.deep.equal( [ 0, 0 ] ); - expect( expectedMarker.getRange().end.path ).to.deep.equal( [ 0, 3 ] ); + expect( expectedMarker.getRange().end.path ).to.deep.equal( [ 1, 2 ] ); } ); it.skip( 'should create marker on imageInline and part of paragraph text after inserting next to paragraph', () => { diff --git a/packages/ckeditor5-image/tests/manual/imageblock.html b/packages/ckeditor5-image/tests/manual/imageblock.html index bea8c9382c5..5b0c45ec78e 100644 --- a/packages/ckeditor5-image/tests/manual/imageblock.html +++ b/packages/ckeditor5-image/tests/manual/imageblock.html @@ -3,9 +3,7 @@ - -
- +

Inline image

Lorem ipsum dolor sit amet, consectetur adipiscing elit.

From 662906f8aa325f3b579390412a746a011bcbcecc Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 16 Jul 2024 11:34:53 +0200 Subject: [PATCH 3/6] Added more tests for insert-content to make sure inline object behaves the same as text while auto-paragraphing. --- .../tests/model/utils/insertcontent.js | 206 ++++++++++++++++++ 1 file changed, 206 insertions(+) diff --git a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js index 98d47c10a37..63793ac2101 100644 --- a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js @@ -1060,6 +1060,212 @@ describe( 'DataController utils', () => { ); } ); } ); + + describe( 'merging edge auto-paragraphs', () => { + describe( 'with text auto-paragraphing', () => { + it( 'inserts text + paragraph + text in the middle of a paragraph text', () => { + setData( model, '12[]34' ); + const affectedRange = insertHelper( 'aaabbbccc' ); + + expect( getData( model ) ).to.equal( + '12aaabbbccc[]34' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '12[aaabbbccc]34' + ); + } ); + + it( 'inserts text + paragraph + text in the end of a paragraph text', () => { + setData( model, '1234[]' ); + const affectedRange = insertHelper( 'aaabbbccc' ); + + expect( getData( model ) ).to.equal( + '1234aaabbbccc[]' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '1234[aaabbbccc]' + ); + } ); + + it( 'inserts text + paragraph + text in the start of a paragraph text', () => { + setData( model, '[]1234' ); + const affectedRange = insertHelper( 'aaabbbccc' ); + + expect( getData( model ) ).to.equal( + 'aaabbbccc[]1234' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '[aaabbbccc]1234' + ); + } ); + } ); + + describe( 'with inline-object auto-paragraphing', () => { + it( 'inserts inlineObject + paragraph + inlineObject in the middle of a paragraph text', () => { + setData( model, '12[]34' ); + const affectedRange = insertHelper( + 'bbb' + ); + + expect( getData( model ) ).to.equal( + '12' + + 'bbb' + + '[]34' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '12[' + + 'bbb' + + ']34' + ); + } ); + + it( 'inserts inlineObject + paragraph + inlineObject in the end of a paragraph text', () => { + setData( model, '1234[]' ); + const affectedRange = insertHelper( + 'bbb' + ); + + expect( getData( model ) ).to.equal( + '1234' + + 'bbb' + + '[]' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '1234[' + + 'bbb' + + ']' + ); + } ); + + it( 'inserts inlineObject + paragraph + inlineObject in the start of a paragraph text', () => { + setData( model, '[]1234' ); + const affectedRange = insertHelper( + 'bbb' + ); + + expect( getData( model ) ).to.equal( + '' + + 'bbb' + + '[]1234' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '[' + + 'bbb' + + ']1234' + ); + } ); + + it( 'inserts inlineObject + text + inlineObject in the middle of a paragraph text', () => { + setData( model, '12[]34' ); + const affectedRange = insertHelper( 'bbb' ); + + expect( getData( model ) ).to.equal( + '12bbb[]34' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '12[bbb]34' + ); + } ); + + it( 'inserts inlineObject + text + inlineObject in the end of a paragraph text', () => { + setData( model, '1234[]' ); + const affectedRange = insertHelper( 'bbb' ); + + expect( getData( model ) ).to.equal( + '1234bbb[]' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '1234[bbb]' + ); + } ); + + it( 'inserts inlineObject + text + inlineObject in the start of a paragraph text', () => { + setData( model, '[]1234' ); + const affectedRange = insertHelper( 'bbb' ); + + expect( getData( model ) ).to.equal( + 'bbb[]1234' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '[bbb]1234' + ); + } ); + + it( 'inserts inlineObject + text + inlineObject between paragraphs', () => { + setData( model, '12[]34' ); + const affectedRange = insertHelper( 'bbb', null, [ 1 ] ); + + expect( getData( model ) ).to.equal( + '12[]' + + 'bbb' + + '34' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '12[]' + + '[bbb]' + + '34' + ); + } ); + } ); + + describe( 'with text between block widgets auto-paragraphing', () => { + it( 'inserts blockObject + text + blockObject in the middle of a paragraph text', () => { + setData( model, '12[]34' ); + const affectedRange = insertHelper( 'bbb' ); + + expect( getData( model ) ).to.equal( + '12' + + '' + + 'bbb' + + '[]' + + '34' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '12[' + + '' + + 'bbb' + + '' + + ']34' + ); + } ); + + it( 'inserts blockObject + text + blockObject in the end of a paragraph text', () => { + setData( model, '1234[]' ); + const affectedRange = insertHelper( 'bbb' ); + + expect( getData( model ) ).to.equal( + '1234' + + '' + + 'bbb' + + '[]' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '1234' + + '[' + + 'bbb' + + ']' + ); + } ); + + it( 'inserts blockObject + text + blockObject in the start of a paragraph text', () => { + setData( model, '[]1234' ); + const affectedRange = insertHelper( 'bbb' ); + + expect( getData( model ) ).to.equal( + '' + + 'bbb' + + '[]' + + '1234' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '[' + + 'bbb' + + ']' + + '1234' + ); + } ); + } ); + } ); } ); describe( 'filtering out', () => { From 8f87b1d56b115a4bb335218acaec2857a6d98dbe Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 16 Jul 2024 13:16:22 +0200 Subject: [PATCH 4/6] Insert content should handle inline objects the same way as text nodes (keep in a common auto-paragraph). --- packages/ckeditor5-engine/src/model/schema.ts | 3 +- .../src/model/utils/insertcontent.ts | 69 ++++--------------- .../ckeditor5-engine/tests/model/schema.js | 39 +++++++++++ .../tests/model/utils/insertcontent.js | 2 +- .../tests/imageupload/imageuploadediting.js | 21 ++++-- 5 files changed, 70 insertions(+), 64 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/schema.ts b/packages/ckeditor5-engine/src/model/schema.ts index cd30e48f252..7b201aed2f0 100644 --- a/packages/ckeditor5-engine/src/model/schema.ts +++ b/packages/ckeditor5-engine/src/model/schema.ts @@ -424,7 +424,8 @@ export default class Schema extends /* #__PURE__ */ ObservableMixin() { /** * 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. diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts index 2e4a9de15aa..1091791cad3 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts @@ -408,34 +408,28 @@ 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 ); - } else { - // 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 ); - // 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. - isAllowed = this._checkAndSplitToAllowedPosition( node ); - - if ( !isAllowed ) { + // Handle element children if it's not an object (strip container). + if ( !this.schema.isObject( node ) ) { this._handleDisallowedNode( node ); - - return; } - } - // Add node to the current temporary DocumentFragment. - this._appendToFragment( node ); + return; + } } + // Add node to the current temporary DocumentFragment. + this._appendToFragment( node ); + // Store the first and last nodes for easy access for merging with sibling nodes. if ( !this._firstNode ) { this._firstNode = node; @@ -480,20 +474,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. */ @@ -502,10 +482,6 @@ class Insertion { if ( node.is( 'element' ) ) { this.handleNodes( node.getChildren() ); } - // If text is not allowed, try autoparagraphing it. - else { - this._tryAutoparagraphing( node ); - } } /** @@ -762,23 +738,6 @@ class Insertion { this.model.schema.checkMerge( node, nextSibling ); } - /** - * 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. diff --git a/packages/ckeditor5-engine/tests/model/schema.js b/packages/ckeditor5-engine/tests/model/schema.js index 69f72c2ad11..ec0b4abca4d 100644 --- a/packages/ckeditor5-engine/tests/model/schema.js +++ b/packages/ckeditor5-engine/tests/model/schema.js @@ -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)', () => { @@ -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' ) diff --git a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js index 63793ac2101..2d07ba9816c 100644 --- a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js @@ -1201,7 +1201,7 @@ describe( 'DataController utils', () => { '34' ); expect( stringify( root, affectedRange ) ).to.equal( - '12[]' + + '12' + '[bbb]' + '34' ); diff --git a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js index 69d14c45389..aa9ecd07997 100644 --- a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js +++ b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js @@ -1142,14 +1142,19 @@ describe( 'ImageUploadEditing', () => { const expectedModel = 'bar' + - '' + - '' + - '[]foo'; + '' + + '' + + '' + + '' + + '[]foo' + + ''; const expectedFinalModel = 'bar' + - '' + - '' + - '[]foo'; + '' + + '' + + '' + + '[]foo' + + ''; setModelData( model, '[]foo' ); @@ -1217,7 +1222,9 @@ describe( 'ImageUploadEditing', () => { expectData( '

baz

', - 'baz[]foo', + '' + + 'baz[]foo', + '' + 'baz[]foo', content, err => { From 98504384464892d336e0ec625e2bd8477aa578eb Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 16 Jul 2024 16:34:31 +0200 Subject: [PATCH 5/6] Insert content should handle split and auto-paragraph scenarios. --- packages/ckeditor5-engine/src/model/schema.ts | 6 +- .../src/model/utils/insertcontent.ts | 57 +++++-------- .../tests/model/utils/insertcontent.js | 84 +++++++++++++++++++ 3 files changed, 107 insertions(+), 40 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/schema.ts b/packages/ckeditor5-engine/src/model/schema.ts index 7b201aed2f0..3c529c81cbb 100644 --- a/packages/ckeditor5-engine/src/model/schema.ts +++ b/packages/ckeditor5-engine/src/model/schema.ts @@ -437,10 +437,6 @@ export default class Schema extends /* #__PURE__ */ ObservableMixin() { * @param elementToMerge The element to merge. Required if `positionOrBaseElement` is an element. */ public checkMerge( positionOrBaseElement: Position | Element, elementToMerge?: Element ): boolean { - if ( elementToMerge && this.isLimit( elementToMerge ) ) { - return false; - } - if ( positionOrBaseElement instanceof Position ) { const nodeBefore = positionOrBaseElement.nodeBefore; const nodeAfter = positionOrBaseElement.nodeAfter; @@ -472,7 +468,7 @@ export default class Schema extends /* #__PURE__ */ ObservableMixin() { return this.checkMerge( nodeBefore, nodeAfter ); } - if ( this.isLimit( positionOrBaseElement ) ) { + if ( this.isLimit( positionOrBaseElement ) || this.isLimit( elementToMerge! ) ) { return false; } diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts index 1091791cad3..9f73a8ab31c 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts @@ -408,23 +408,15 @@ class Insertion { * Handles insertion of a single node. */ private _handleNode( node: Node ): void { - // 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 ) { - // Handle element children if it's not an object (strip container). - if ( !this.schema.isObject( node ) ) { - this._handleDisallowedNode( node ); - } - - return; + // 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; } // Add node to the current temporary DocumentFragment. @@ -739,24 +731,9 @@ 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. - * - * @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(); @@ -768,8 +745,6 @@ class Insertion { this._lastAutoParagraph = paragraph; this.position = this.writer.createPositionAt( paragraph, 0 ); - - return true; } /** @@ -824,20 +799,32 @@ class Insertion { } } + 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 diff --git a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js index 2d07ba9816c..7ebed76cb5c 100644 --- a/packages/ckeditor5-engine/tests/model/utils/insertcontent.js +++ b/packages/ckeditor5-engine/tests/model/utils/insertcontent.js @@ -426,6 +426,90 @@ describe( 'DataController utils', () => { expect( stringify( root, affectedRange ) ).to.equal( 'fo[xyz]ar' ); } ); + it( 'should split and auto-paragraph if needed', () => { + model.schema.register( 'widgetContainer', { + allowWhere: '$block' + } ); + model.schema.extend( 'blockWidget', { + allowIn: 'widgetContainer' + } ); + + setData( model, '[]' ); + + const affectedRange = insertHelper( 'abc', null, [ 0, 1 ] ); + + expect( getData( model ) ).to.equal( + '' + + 'abc' + + '[]' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '[' + + 'abc' + + ']' + ); + } ); + + it( 'should split and auto-paragraph multiple levels if needed', () => { + model.schema.register( 'blockContainer', { + allowIn: '$root', + allowChildren: '$block' + } ); + model.schema.register( 'outerContainer', { + allowWhere: '$block' + } ); + model.schema.register( 'widgetContainer', { + allowIn: 'outerContainer' + } ); + model.schema.extend( 'blockWidget', { + allowIn: 'widgetContainer' + } ); + + setData( model, + '' + + '' + + '' + + '' + + '[]' + + '' + + '' + + '' + ); + + const affectedRange = insertHelper( 'abc', null, [ 0, 0, 0, 1 ] ); + + expect( getData( model ) ).to.equal( + '' + + '' + + '' + + '' + + '' + + '' + + 'abc' + + '' + + '' + + '[]' + + '' + + '' + + '' + ); + expect( stringify( root, affectedRange ) ).to.equal( + '' + + '' + + '' + + '' + + '[' + + '' + + 'abc' + + '' + + ']' + + '' + + '' + + '' + + '' + ); + } ); + it( 'not insert autoparagraph when paragraph is disallowed at the current position', () => { // Disallow paragraph in $root. model.schema.addChildCheck( ( ctx, childDef ) => { From c08d7d83288246f887307e002a998cf0439722f1 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 17 Jul 2024 14:20:08 +0200 Subject: [PATCH 6/6] Added code comment. --- packages/ckeditor5-engine/src/model/utils/insertcontent.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts index 9f73a8ab31c..3a595457bab 100644 --- a/packages/ckeditor5-engine/src/model/utils/insertcontent.ts +++ b/packages/ckeditor5-engine/src/model/utils/insertcontent.ts @@ -799,6 +799,9 @@ 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(); }