T/61: Improved the typing when the whole content is selected

[pomek]

Suggested merge commit message (convention)

Feature: Improved the typing when the whole content is selected. Closes ckeditor/ckeditor5#3065.

• After typing when the entire content was selected – text will be wrapped in the paragraph.
• In an empty editor – press and hold the Backspace key will not remove styles of the current element. The entire content will be removed but the element will left.

[Reinmar]

It's a private method so just pass the sequence as its direct param. We don't need the options object right now here and since it's private we can do whatever we want with the params in the future.

[Reinmar]

A number describing which subsequent delete event it is without the key being released. If it's 2 or more it means that the key was pressed and hold.

[Reinmar]

The PR is really good. I had a bit different image of where the code should land, but it makes a lot of sense what you did (putting the code to the command).

The only thing is... the code duplication with the engine ;| We need to figure out how to reuse the engine because this is quite a lot of code which I don't know where could land otherwise.

So, there are at least two options:

• add an option to deleteContent() which will make it skip the "multiple elements selected" check,
• make the code used by deleteContent() and this command reusable.

The latter has a lot more sense. But it's tricky because we need to find the right pieces of functionality which make sense in some broader context. Like "is entire content selected" and maybe something more.

[Reinmar]

Blank line before a list in MD. And start items from * (without additional indentation).

[Reinmar]

"get limit element" would be another piece of logic to be exposed somewhere else.

[Reinmar]

and finally, in order to leave just a paragraph we can do this:

• select entire content of the root (for real now),
• call deleteContent()

In sucj case, deleteContent() will really remove all content and leave just a paragraph (if it can be there).

So, these 3 things would allow us removing nearly entire duplication.

[pomek]

I introduced some fixes but it still requires some work…

• duplicated checks "whether the entire content should be replaced with a paragraph" have been removed,
• API of the private method – DeleteCommand#_shouldEntireContentBeReplacedWithParagraph() – was simplified (as you wrote),
• fixes for docs,
• replacing with the paragraph is done by engine,
• an option skipParentsCheck in the engine has been added – https://github.com/ckeditor/ckeditor5-engine/compare/t/ckeditor5-typing/61.

Unfortunately, getLimitElement() function still occurs twice (in the engine and in the typing).

I am not sure whether I'm following a good way. Maybe should I extract requires function from DataController#deleteContent as the utils?

And the one more (and the most important) thing – one of the test does not pass after my changes.

Because the paragraph is not allowed in current element, replacing with the paragraps is not executed and at the end we have an empty document with the selection: [].

[pomek]

I've just created a ticket for your suggestion – https://github.com/ckeditor/ckeditor5-engine/issues/1041. It's required before merging this PR.

[Reinmar]

That ticket mentioned 3 methods, out of which only getLimitElement() made sense. The other two will not be useful.

From this PR I can see that there are, in general, two methods which might be added to the engine – getLimitElement() and isEntireContentSelected(). The former will make sense in Schema. The latter perhaps in Selection (I've been considering also EditingController or Document).

[Reinmar]

Could you refresh this PR so it's ready to review? Now, the build was failing. If there are some issues that you'd like to work on together with me, please describe them.

[pomek]

If there are some issues that you'd like to work on together with me, please describe them.

I guess only the one thing is adding a new method to Selection – Selection#isEntireContentSelected( element ). Is it correct?

[Reinmar]

What do you mean?

[pomek]

Can we add a new method to Selection class as you wrote a few days ago? The same as we did it for getLimitElement() function (added as a method in Schema). I missed the second function.

[Reinmar]

Yes, we can.

[pomek]

This PR requires https://github.com/ckeditor/ckeditor5-engine/issues/1063.

[Reinmar]

Your last change made the code even more unclear than it should be. You focused on code itself not on behaviour requirements.

What is that common ancestor? Why paragraph can't be inside it? This code is neither correct nor clear.

Please, before making any changes, think on WHEN we shouldn't replace the entire limit element content with a paragraph.

[pomek]

TBH I don't understand what I should do.

A part of the code prevents from replacing an empty paragraph with another paragraph. If you want, we can skip it and replace the entire content all the time.

[Reinmar]

A part of the code prevents from replacing an empty paragraph with another paragraph.

First of all – the code says something different:

other empty paragraph does not occur in the limit element.

So, this is the first problem – you're saying now something different than you documented. Which is wrong? The code.

Second – you still didn't describe it precisely. The precise situation is that we don't want to replace this paragraph with a new paragraph:

<limit>
    <p>^</p>
</limit>

However, we still want to replace this block quote with a new paragraph (the expected result is <limit><p>^</p></limit>:

<limit>
    <blockQuote>
        <p>^</p>
    </blockQuote>
</limit>

Will your code do that? I don't think so. Your documentation neither explains what is supposed to happen nor implement it that. Start from writing precisely what you want achieve and the code will follow.

[pomek]

ATM, the test below even does not work:

it( 'leaves an empty paragraph after removing another paragraph from block element', () => {
	doc.schema.registerItem( 'section', '$block' );
	doc.schema.registerItem( 'blockQuote', '$block' );
	doc.schema.limits.add( 'section' );
	doc.schema.allow( { name: 'section', inside: '$root' } );
	doc.schema.allow( { name: 'blockQuote', inside: 'section' } );
	doc.schema.allow( { name: 'paragraph', inside: 'blockQuote' } );

	setData( doc,
		'<section>' +
			'<blockQuote>' +
				'<paragraph>[]</paragraph>' +
			'</blockQuote>' +
		'</section>'
	);


	editor.execute( 'delete' );

	expect( getData( doc, { selection: true } ) ).to.equal(
		'<section>' +
			'<paragraph>[]</paragraph>' +
		'</section>'
	);
} );

because of:

And this error occurs even when I remove code which I added.

[Reinmar]

Hm... I think I experienced this error in the past.

Yep... https://github.com/ckeditor/ckeditor5-engine/issues/905

Set data without setting selection and then set it using the normal selection API. Or check how I worked around this in block-quote tests.

[Reinmar]

BTW, I don't think you need to pass selection:true to getData().

[pomek]

setData works fine. It throws an error when I execute delete command.

Something does not work in mergeBranches function (in DeleteController#deleteContent()).

BTW, I don't think you need to pass selection:true to getData().

👍

[Reinmar]

setData works fine. It throws an error when I execute delete command.

But DataController#deleteContent() should not be called at all in this scenario. Should it?

[pomek]

Yes, you're right. Thanks for helping! I forgot to allow appending paragraph to section…

[pomek]

Ok, I guess it's ready for review once again.