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.

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

Editor crashes after pasting multi-block content over existing content in a blockquote #1921

Closed
Mgsy opened this issue Jul 29, 2019 · 4 comments · Fixed by #12309
Closed
Assignees
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@Mgsy
Copy link
Member

Mgsy commented Jul 29, 2019

Is this a bug report or feature request? (choose one)

🐞 Bug report

💻 Version of CKEditor

Latest master

📋 Steps to reproduce

  1. Set the following data: editor.setData( '<blockquote><p>test</p><p>test</p></blockquote>' );
  2. Copy those paragraphs.
  3. Press Ctrl + A.
  4. Paste.

✅ Expected result

Content is pasted properly.

❎ Actual result

The editor crashes.

📃 Other details that might be useful

Error

schema.js:1156 Uncaught TypeError: Cannot read property 'getAncestors' of null
    at new me (schema.js:1156)
    at fe.on.priority (schema.js:58)
    at fe.fire (emittermixin.js:207)
    at fe.<computed> [as checkChild] (observablemixin.js:259)
    at fn._getAllowedIn (insertcontent.js:603)
    at fn._checkAndSplitToAllowedPosition (insertcontent.js:544)
    at fn._handleNode (insertcontent.js:259)
    at fn.handleNodes (insertcontent.js:185)
    at Object.callback (insertcontent.js:69)
    at xn._runPendingChanges (model.js:738)

GIF
bug_cke5

It's a regression. I've bisected it and it looks like these changes introduced a bug - ckeditor/ckeditor5-engine@f4e4644

@Mgsy Mgsy added type:bug This issue reports a buggy (incorrect) behavior. status:confirmed package:engine labels Jul 29, 2019
@mlewand mlewand modified the milestones: iteration 26, next Jul 29, 2019
@Reinmar Reinmar modified the milestones: next, iteration 27 Aug 7, 2019
@pomek
Copy link
Member

pomek commented Sep 16, 2019

I wrote a test that reproduces the issue:

it( 'does not blow up after pasting items over block-quote', () => {
	setModelData( model,
		'[<blockQuote>' +
			'<paragraph>Foo</paragraph>' +
			'<paragraph>Bar</paragraph>' +
		'</blockQuote>]'
	);

	console.log( getModelData( model ) );

	pasteHtml( editor, '<p>FooBar</p>' );

	console.log( getModelData( model ) );
} );

function pasteHtml( editor, html ) {
	editor.editing.view.document.fire( 'paste', {
		dataTransfer: createDataTransfer( { 'text/html': html } ),
		preventDefault() {
		}
	} );
}

function createDataTransfer( data ) {
	return {
		getData( type ) {
			return data[ type ];
		}
	};
}

And it fails (as expected):

CKEditorError: model-position-path-incorrect: The position's path is incorrect. Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-model-position-path-incorrect
       {"position":{"root":"main","path":[0,0,0],"stickiness":"toNext"}}

A place in the code where the error was thrown: https://github.com/ckeditor/ckeditor5-engine/blob/04fa92058a83d70c7384713cab9f462d2f1ae90f/src/model/utils/insertcontent.js#L139

And I guess, I understand why it happens. The position after removing all nodes from the editor is invalid. After switching the position to be an instance of LivePosition, the error does not occur. But it isn't a solution. That gave me an idea of what could be wrong.

Then, I decided to write one more test for the insertcontent util that does more or less the same as the previous one:

it.only( 'deletes selection with elements before inserting the content', () => {
	model.schema.register( 'p', { inheritAllFrom: '$block' } );
	model.schema.register( 'div', {
		allowWhere: '$block',
		allowContentOf: '$root'
	} );

	setData( model, '[<div><p>Foo</p><p>Bar</p></div>]' );
	const affectedRange = insertHelper( '<p>FooBar</p>' );

	expect( getData( model ) ).to.equal( '<p>FooBar</p>[]' );
	expect( stringify( root, affectedRange ) ).to.equal( '[<p>FooBar</p>]' );
} );

Unfortunately, it didn't fail as I expected. For such case, a position used to create an instance of Insertion class seems to be valid.

What am I doing wrong here?

@pomek
Copy link
Member

pomek commented Sep 16, 2019

Thanks to @Reinmar, we figured out what was wrong with the second test. Instead of using a short name of paragraph (p), I should use a full name of the element because the engine assumes that the element exists.

The corrected tests should look like:

it( 'deletes selection with elements before inserting the content', () => {
	model.schema.register( 'div', {
		allowWhere: '$block',
		allowContentOf: '$root'
	} );

	setData( model, '<div><paragraph>[Foo</paragraph><paragraph>Bar]</paragraph></div>' );
	const affectedRange = insertHelper( '<paragraph>FooBar</paragraph>' );

	expect( getData( model ) ).to.equal( '<paragraph>FooBar[]</paragraph>' );
	expect( stringify( root, affectedRange ) ).to.equal( '[<paragraph>FooBar</paragraph>]' );
} );

And this one, as expected, fails. Going into deep, it leads to this issue – https://github.com/ckeditor/ckeditor5-engine/issues/1722 and I'll continue posting my thoughts here.

@mlewand mlewand modified the milestones: iteration 27, iteration 28 Sep 18, 2019
@pomek
Copy link
Member

pomek commented Oct 4, 2019

Using the Title plugin, the issue is even "easier" to reproduce.

Type whatever in the title section, then select all (CTRL/CMD+A) then paste anything.

@Reinmar Reinmar assigned Reinmar and unassigned pomek Oct 29, 2019
@mlewand mlewand added the type:regression This issue reports a bug that was not present in the previous releases. label Oct 31, 2019
@Reinmar Reinmar modified the milestones: iteration 28, iteration 29 Nov 26, 2019
@Reinmar Reinmar modified the milestones: iteration 29, next Jan 28, 2020
@Reinmar
Copy link
Member

Reinmar commented Mar 10, 2020

DUP of #2010. Fixed with ckeditor/ckeditor5-engine#1830.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants