model#deleteContent() will proper merge elements inside limit element

[pomek]

Suggested merge commit message (convention)

Fix: model#deleteContent() will proper merge elements inside limit element. Closes ckeditor/ckeditor5#1265.

[coveralls]

Coverage remained the same at 100.0% when pulling feb015e on t/ckeditor5/1265 into 08b2a34 on master.

[Reinmar]

This code should land in checkCanBeMerged(). I'm also not sure if this is the right check. See the comment above checkCanBeMerged():

// Check if parents of two positions can be merged by checking if there are no limit/object
// boundaries between those two positions.
//
// E.g. in <bQ><p>x[]</p></bQ><widget><caption>{}</caption></widget>
// we'll check <p>, <bQ>, <widget> and <caption>.
// Usually, widget and caption are marked as objects/limits in the schema, so in this case merging will be blocked.

This is a more general implementation – it's based on checking whether there's no limit elements involved. We know that the case we're currently solving requires that everything happens inside a limit element. I can also see that the condition you wrote is true when endParent is a tableCell (which is a limit element). That makes me suspicious whether the check should be about endParent being equal to startPos.parent.

[pomek]

I changed my solution. Now the merge is blocking if an only single element is between positions (leftPos and rightPos). From my point of view, such merge does not have any sense because there is nothing to merge.

[Reinmar]

I changed my solution. Now the merge is blocking if an only single element is between positions (leftPos and rightPos). From my point of view, such merge does not have any sense because there is nothing to merge.

First of all, I don't understand what's wrong with this scenario. We talk about merging, so if the positions look like this:

<paragraph>foo[</paragraph>
<paragraph>]bar</paragraph>

(nothing between these two positions) then there's still something to merge there – those two paragraphs together. Or do the positions look differently in this case? I can't find how they look in any of your comments.

Secondly, we also need to understand why does the previous code lead this case to such a situation. Perhaps some code that's executed earlier works incorrectly. You say that merge cannot happen in this case. But how we even got to this place?

Those are the questions you need to answer. Step by step you need to investigate this algorithm and we need to see if at any of those steps something wrong happens. I recommend that you list here what are the positions/ranges and model structure at various important points before so we can check this together.

[pomek]

(nothing between these two positions)

I see two elements to merge. Closing a paragraph and opening a paragraph.

[Reinmar]

I see two elements to merge. Closing a paragraph and opening a paragraph.

So it seems that I still don't know how those positions look in the scenario you debug. Please provide more information on what's happening there so we could analyse it together.

[pomek]

I will go with the test case that allows reproducing the issue.

1. The error occurs in mergeBranches() function so I put a debugger in a line before the function call:

ckeditor5-engine/src/model/utils/deletecontent.js

Line 82 in a391790

mergeBranches( writer, startPos, endPos );

2. What we have in the model? getData( model )

<blockLimit>
  <blockQuote>
    <paragraph>Foo[</paragraph>
  </blockQuote>
  <paragraph>]Bar</paragraph>
</blockLimit>

3. First call of mergeBranches() function does not throw any error. Before recursively calling, let's print some details:

<!-- getDat( model ) -->

<blockLimit>
  <blockQuote>
    <paragraph>Foo[]Bar</paragraph>
  </blockQuote>
</blockLimit>

In fact, we have already the correct result. We shouldn't call mergeBranches() again. But we will do it with params listed below:

startPos.path = [0, 0, 1]
endPos.path = [0, 1]

It means that the function wants to merge </blockQuote> with </paragraph> which produces an error (mentioned in the issue).

[Reinmar]

I think that the bug is here:

ckeditor5-engine/src/model/utils/deletecontent.js

Lines 124 to 126 in f6ad9af

	if ( !startParent.parent || !endParent.parent ) {
	return;
	}

See the comment:

	// If one of the positions is a root, then there's nothing more to merge (at least in the current state of implementation).
	// Theoretically in this case we could unwrap the <p>: <$root>x[]<p>{}y</p></$root>, but we don't need to support it yet
	// so let's just abort.

Why is this applying only to the root? As the comment says, it's the situation in the current state of implementation when we didn't have limit elements. Now we have them and the endParent is one of them. Also, the $root is actually a limit too.

So, in short, the check could be changed to:

if ( writer.model.schema.isLimit( startParent ) || writer.model.schema.isLimit( endParent ) ) {

The comment needs to be updated a bit too. It seems to cover the raported scenario. However, I'd like to see more tests with deeper nested blocks. Like, if there were several <bQ> elements one in another.

[pomek]

Seems to work. Thx.