insertContent should have a better way of figuring out insertion position

[scofalik]

This is a snippet from insertContent function:

const insertionPosition = selection.getFirstPosition();

if ( !selection.isCollapsed ) {
	model.deleteContent( selection, { doNotAutoparagraph: true } );
}

const insertion = new Insertion( model, writer, insertionPosition );

We assume here that deleteContent will delete not more than is in the selection, so the first selection position is always a safe place for insertion. However, this does not have to be true.

Earlier, this part of the code used the fact deleteContent may change the selection and that the selection will be always collapsed after using deleteContent:

const insertion = new Insertion( model, writer, selection.anchor );

Which made perfect sense, especially since Model#deleteContent() can be overwritten (it is a decorated method).

However, this proved to be problematic for track changes features and I changed this behavior when fixing track changes bugs. One of the cases I remember was an editor that had only one image as the content.

We need to rethink what assumptions we have to make about insertContent and deleteContent and how they should work together given that we have a new set of cases thanks to track changes feature.

[pomek]

In order to resolve an issue described here – #1921, I figured out that the position (insertionPosition) can be wrong.

My first thought was creating an instance of LivePosition. I replaced the snippet above with the following code:

let insertionPosition = LivePosition.fromPosition( selection.getFirstPosition() );

if ( !selection.isCollapsed ) {
	model.deleteContent( selection, { doNotAutoparagraph: true } );
}

// The entire position could be moved to the graveyard while execuing the `model.deleteContent()` function.
if ( insertionPosition.root.rootName === '$graveyard' ) {
	insertionPosition.detach();

	insertionPosition = selection.getFirstPosition();
}

And it works. Issue #1921 does not occur anymore. Unfortunately, a few tests in the insertcontent util started to fail.

Chrome 77.0.3865 (Mac OS X 10.14.6) transform paste inside paste + undo, undo + redo, redo FAILED
        CKEditorError: merge-operation-how-many-invalid: Merge operation specifies wrong number of nodes to move

Chrome 77.0.3865 (Mac OS X 10.14.6) transform paste, remove, undo, undo, redo, redo FAILED
        CKEditorError: model-position-path-incorrect-format: Position path must be an array with at least one item.

Chrome 77.0.3865 (Mac OS X 10.14.6) DataController utils insertContent should be able to insert content at model element (offset="on") FAILED
        AssertionError: expected '<paragraph>foo[]</paragraph>' to equal '<paragraph>foo[]</paragraph><foo></foo>'

Chrome 77.0.3865 (Mac OS X 10.14.6) DataController utils insertContent in blocks content over a block object inserts text FAILED
        AssertionError: expected '<paragraph>fooxxx[]</paragraph><paragraph>bar</paragraph>' to equal '<paragraph>foo</paragraph><paragraph>xxx[]</paragraph><paragraph>bar</paragraph>'

Chrome 77.0.3865 (Mac OS X 10.14.6) DataController utils insertContent in blocks content over a block object inserts paragraph FAILED
        AssertionError: expected '<paragraph>fooxxx[]</paragraph><paragraph>bar</paragraph>' to equal '<paragraph>foo</paragraph><paragraph>xxx[]</paragraph><paragraph>bar</paragraph>'

Chrome 77.0.3865 (Mac OS X 10.14.6) DataController utils insertContent in blocks content over a block object inserts text + paragraph FAILED
        AssertionError: expected '<paragraph>fooyyy</paragraph><paragraph>xxx[]</paragraph><paragraph>bar</paragraph>' to equal '<paragraph>foo</paragraph><paragraph>yyy</paragraph><paragraph>xxx[]</paragraph><paragraph>bar</paragraph>'

Chrome 77.0.3865 (Mac OS X 10.14.6) DataController utils insertContent in blocks content over a block object inserts two blocks FAILED
        AssertionError: expected '<paragraph>fooxxx</paragraph><paragraph>yyy[]</paragraph><paragraph>bar</paragraph>' to equal '<paragraph>foo</paragraph><heading1>xxx</heading1><paragraph>yyy[]</paragraph><paragraph>bar</paragraph>'

Chrome 77.0.3865 (Mac OS X 10.14.6) DataController utils insertContent in blocks content over a block object inserts inline object FAILED
        AssertionError: expected '<paragraph>foo<inlineWidget></inlineWidget>[]</paragraph><paragraph>bar</paragraph>' to equal '<paragraph>foo</paragraph><paragraph><inlineWidget></inlineWidget>[]</paragraph><paragraph>bar</paragraph>'

Chrome 77.0.3865 (Mac OS X 10.14.6) AutoMediaEmbed - integration use fake timers inserts a new media element if pasted a link when other media element was selected in correct place FAILED
        AssertionError: expected '<paragraph>Foo. <$text linkHref="https://cksource.com">Bar</$text>https://www.youtube.com/watch?v=H08tGjXNHO4[]</paragraph><paragraph><$text bold="true">Bar</$text>.</paragraph>' to equal '<paragraph>Foo.
 <$text linkHref="https://cksource.com">Bar</$text></paragraph>[<media url="https://www.youtube.com/watch?v=H08tGjXNHO4"></media>]<paragraph><$text bold="true">Bar</$text>.</paragraph>'

Chrome 77.0.3865 (Mac OS X 10.14.6): Executed 11210 of 11495 (9 FAILED) (skipped 21) (3 mins 40.85 secs / 1 min 25.764 secs)

[scofalik]

Please paste here what is the model and selection before deleteContent is executed and what is the position after deleteContent is executed.

[pomek]

Model before model.deleteContent():

'<div><paragraph>[Foo</paragraph><paragraph>Bar]</paragraph></div>'

Model after model.deleteContent():

'<paragraph>[]</paragraph>'

Position to insert:

Position{
  root: RootElement{parent: null, _attrs: Map{}, name: '$root', _children: NodeList{_nodes: ...}, _doc: Document{model: ..., version: ..., history: ..., selection: ..., roots: ..., differ: ..., _postFixers: ..., _hasSelectionChangedFromTheLastChangeBlock: ...}, rootName: 'main'}, 
  path: [0, 0, 0], 
  stickiness: 'toNext'
}

[scofalik]

Okay, so it seems that "replace all content with a paragraph" has been called in deleteContent(), which has changed the editor content "before" first selection position.

[pomek]

After blocking the condition for "replace all content with a paragraph"
https://github.com/ckeditor/ckeditor5-engine/blob/c5033b6bb0ef654fbe1d179841e8b5aed7ca8356/src/model/utils/deletecontent.js#L75-L79

this test does not throw but it fails:

AssertionError: expected '<div><paragraph>FooBar[]</paragraph></div>' to equal '<paragraph>FooBar[]</paragraph>'

+ expected - actual

-<div><paragraph>FooBar[]</paragraph></div>
+<paragraph>FooBar[]</paragraph>

After blocking inserting paragraph (L227) in the "replace all content with a paragraph" mechanism, the test throws an error (details in the previous issue).

https://github.com/ckeditor/ckeditor5-engine/blob/c5033b6bb0ef654fbe1d179841e8b5aed7ca8356/src/model/utils/deletecontent.js#L223-L228

[CKEditorBot]

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

[CKEditorBot]

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

[CKEditorBot]

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).