Cannot type after inserting break line element in a paragraph that ends with a space

[pomek]

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

🐞 Bug report

💻 Version of CKEditor

Latest master in all packages.

📋 Steps to reproduce

During work on ShiftEnter (soft enter, soft return) plugin, I noticed that you cannot type after inserting a <br> directly after space.

The ShiftEnter plugin is on the review (ckeditor/ckeditor5-enter#54), but the PR comes from the community. Instead of cloning the new repo and linking everything, I decided to copy the changes to the new branch that speed up reproducing the issue.

1. Check out to the #t/2 branch in ckeditor5-engine package.
2. Run the manual tests server (npm run test:manual -- --files=enter)
3. Run the sample: http://localhost:8125/ckeditor5-enter/tests/manual/enter.html
4. You should see an editor that contains two paragraphs. Put the selection in the first one, type a Space then press Shift+Enter. Then if you want to type (e.g. "a"), nothing will happen.

✅ Expected result

MT defines two helpers:

window.getModelData = () => getModelData( editor.model );
window.getViewData = () => getViewData( editor.editing.view );

After reproducing the steps above, the functions should return:

getModelData()
"<paragraph>Paragraph 1 <break></break>a[]</paragraph><paragraph>Paragraph 2</paragraph>"
getViewData()
"<p>Paragraph 1 <br></br>a[]</p><p>Paragraph 2</p>"
editor.getData()
"<p>Paragraph 1&nbsp;<br>a</p><p>Paragraph 2</p>"

❎ Actual result

But they return:

getModelData()
"<paragraph>Paragraph 1 <break></break>[]</paragraph><paragraph>Paragraph 2</paragraph>"
getViewData()
"<p>Paragraph 1 <br></br>[]</p><p>Paragraph 2</p>"
editor.getData()
"<p>Paragraph 1&nbsp;<br></p><p>Paragraph 2</p>"

📃 Other details that might be useful

The same thing will happen in image caption but the <br> element will not be visible in the editor but occurs in the DOM.

[pomek]

Some debug notes:

• After typing any letter in that model: <paragraph>Paragraph 1 <break></break>[], the MutationHandler catches the insertion

• This kind of mutation is handled by the _handleContainerChildrenMutations() function

• Everything works well until check whether modelFromDomChildren or currentModelChildren contain text nodes only

  modelFromDomChildren contains 3 nodes:

  1. Text ("Paragraph 1 ")
  2. Element (<break></break>)
  3. Text (letter that was typed)

  Because the model contains an element, this condition returns fail and the mutation is ignored (and the letter is being removed).

  I checked how it would work if the <break> element could be available in the hasOnlyTextNodes function:

function hasOnlyTextNodes( children ) {
	return children.every( child => child.is( 'text' ) || child.is( 'element', 'break' ) );
}

The mutation wasn't ignored but the letter was inserted in invalid place: <paragraph>Paragraph 1 a[]<break></break>

I assume that the comment:

// Skip situations when common ancestor has any elements (cause they are too hard).

should be somehow fixed.

[Reinmar]

May be related to #692.

[Reinmar]

This kind of mutation is handled by the _handleContainerChildrenMutations() function

That's a first thing to check. Why typing a letter causes a children mutation, not a text node mutation. You need to check the native mutations to see what they contain. There's a chance that this is really a children mutation and will need to be handled by us. But it's also possible that due to issues mentioned in #692 we incorrectly discover this mutation as a children mutation.

[pomek]

That's a first thing to check.

Let's start with the editor that contains:

<p>Paragraph 1&nbsp;<br>[]</p>
<p>Paragraph 2</p>

After pressing the letter (a), we have a DOM mutation caught by the MutationObserver#_onMutations():

domMutations.length // 1
domMutations[ 0 ] instanceof MutationRecord // true
domMutations[ 0 ].addedNodes // NodeList (empty)
domMutations[ 0 ].attributeName // null
domMutations[ 0 ].attributeNamespace // null
domMutations[ 0 ].oldValue // ""
domMutations[ 0 ].removedNodes // (empty)
domMutations[ 0 ].type // "characterData"
domMutations[ 0 ].target // Text "​​​​​​​a"

In L174 of the same file, we're trying to find corresponding view text but it fails and the mutation is tracked as mutatedElements instead of mutatedTexts.

We even have a comment that describes what happened (some lines later):

// When we added first letter to the text node which had only inline filler, for the DOM it is mutation
// on text, but for the view, where filler text node did not existed, new text node was created, so we
// need to fire 'children' mutation instead of 'text'.

[Reinmar]

Hehe :) Nice!

So you were right about this:

I assume that the comment:

// Skip situations when common ancestor has any elements (cause they are too hard).

should be somehow fixed.

Inline elements need to be handled by this function. We could change this to:

// Skip situations when the common ancestor has any non-empty elements (cause they are too hard).

This will be enough for now.

[Reinmar]

I've just realised – why does it happen only when there's a space at the end of the previous line? If the issue was (only) in _handleContainerChildrenMutations() it would, I think, always occur.

And there are is one more issue here. I think it may be related.

Space after (or before) <br> is lost:

<p>XX<br> Y</p>

It's only rendered as long as there's selection in the " Y" text node. This is thanks to the fact that the inline filler is rendered there (<p>XX<br>FILLER Y</p>). The space disappears (visually) as soon as you move the selection away because then we remove the filler.

Spaces around <br> should be rendered as  . Just like at the boundaries of block elements.

This algorithm is controlled by DomConverter#_processDataFromViewText() and needs to be fixed. Let's fix it first and see if it helps with the issue you originally reported.

[pomek]

One more case to check:

<p>Text<br> Text[]</p> => <p>Text[]<br>Text</p> Space => <p>Text []<br> Text</p>

A space after br was not visible when the selection was moved to the first line but it appears if a new space will be inserted before br.

[Reinmar]

I don't understand what do you mean by this case.

But I've just noticed that sometimes we render   around <br>s. E.g. when typing a space in this case:

<p>XX <br>[]YY</p>

It will become:

<p>XX <br>&nbsp;[]YY</p>

However, if you'd start with this:

<p>XX<br>[]YY</p>

We'd render:

<p>XX<br> []YY</p>

So, there's certainly something wrong there. I guess that that algorithm skips <br> so in the first case it thinks that the content looks like this: <p>XX YY</p> (two spaces) and in the second case <p>XX Y</p> (single space). And that's why only in the first case the inserted space is encoded.

[Reinmar]

PS. When testing this manually, make sure that there's no inline filler rendered because it will give you different visual results. You must be looking at a clean DOM.

[Reinmar]

Just to be sure where we need to render   I checked this simple scenario:

<div contenteditable>
  <p>XX <br>YY</p>
  <p>XX&nbsp;<br>YY</p>
  <p>XX<br> YY</p>
  <p>XX<br>&nbsp;YY</p>
</div>

(see: https://jsfiddle.net/zh51rd9s/)

The first two paragraphs are ok. You can place the caret at "XX []". So we don't need to render   before <br>. I think, though, that perhaps we can do that anyway to simplify the implementation.

Regarding cases 3 and 4 we can see that we need to render   after <br> because otherwise it's collapsed:

[Reinmar]

Fixing view to DOM conversion wasn't hard. Spaces touching <br> elements will be always rendered as   after my changes.

DOM to view conversion is also broken and it's more tricky due to how it's implemented now. We use the _getTouchingDomTextNode() function which internally does this:

const treeWalker = document.createTreeWalker( topmostParent, NodeFilter.SHOW_TEXT );

So it only cares for text nodes, ignoring everything on the way to them. Then, it only checks the common ancestors. Naturally, in such a case:

<p>foo<br>bar</p>

when we're processing the foo node, we're interested that there's <br> next to it. And since the structure may look like this:

<p><b><i>foo</i></b><br>bar</p>

We can't implement a simple check like foo.nextSibling.tagName == br. We need to change _getTouchingDomTextNode() to something like _getTouchingSignificantDomInlineNode().

[Reinmar]

We can't implement a simple check like foo.nextSibling.tagName == br. We need to change _getTouchingDomTextNode() to something like _getTouchingSignificantDomInlineNode().

BTW, I'm talking about "significant inline nodes" because I have a feeling that it's not only about <br>s.

Alternatively, if we're focused on <br>s for now, I can add another fwd and backward look for <br>s only. But this happens for every single text node, so we must be careful.

[jodator]

@Reinmar: Ugh.. so simple fix:

https://github.com/ckeditor/ckeditor5-typing/blob/e553afb1592c66a45f0964ccb41d6f1672ad3702/src/utils/injecttypingmutationshandling.js#L277-L279

should be changed to (to allow placeholder to work):

function isSafeForTextMutation( children ) {
	return children.every( child => child.is( 'text' ) || child.is( 'softBreak' ) || child.is( 'placeholder' ) );
}

Now I havn't tested this yet but I'm pretty sure that this cannot be 'container' - as this might be anything and is not safe for text mutation.

As for the solution I tend to creating either:

• schema type for inline/text-like elements
• new child class of the Element, like InlineElement() so both ilnine widgets and softBrakes (to check also) could be of such type

As on the API updates needed there must be some additions at least so either solution looks OK.