Explosion when playing with the basic styles

[oskarwrobel]

[oskarwrobel]

The same is on https://ckeditor5.github.io/

[ma2ciek]

I can also reproduce it - hitting twice italic button on the text containing an edge of the bold fragment.

[Reinmar]

Similar one (in terms of steps) https://github.com/ckeditor/ckeditor5-engine/issues/711.

[oleq]

I spent a couple of minutes debugging this issue and here's a little I have found. For the following data in the editor (after bold and italic)

<p>This <i>[is <strong>an]</strong></i><strong> editor</strong> instance.</p>

the second italic command execution (like in the screencast) fails in DomConverter. In viewPositionToDom() there's an attempt to getCorrespondingDomText() for a Text "an editor", which is a child of AttributeElement (strong).

The algorithm falls into this condition and this.getCorrespondingDom( viewText.parent ) returns null because getCorrespondingDomElement() for such AttributeElement is not found in _viewToDomMapping.

I'm not quite sure why it is so but something tells me either:

1. the AttributeElement has not been properly registered in the mapping,
2. something removed it from it in the meantime so the algorithm works on desynced DOM/view,
3. the algorithm should have never fallen in this flow for some other reason.

I suppose some more serious view vs. DOM debugging is necessary to figure it out. I hoped a simple fix will do the trick but I'm afraid there's none 😞.

[Reinmar]

I think that #453 is a DUP of this ticket, but we need to confirm it after fixing this one.

[Reinmar]

One more TC to test after fixing this issue: https://github.com/ckeditor/ckeditor5-basic-styles/issues/46. Since I closed it as a DUP of this, if it turns out that it's a different issue, it needs to be reported here.

[Reinmar]

Two more similar tickets: https://github.com/ckeditor/ckeditor5-list/issues/58 and https://github.com/ckeditor/ckeditor5-engine/issues/701.

[szymonkups]

We had a debugging session with @Reinmar and we came up to some conclusions. We mainly focused on test case reported by @oskarwrobel. Thing is that there are situations when Renderer is not rendering text nodes when its parent node is marked to render (here). This happens in our case so the text node is not updated correctly and incorrect DOM is rendered. This way we are out of sync and probably further selection rendering is also incorrect (uncaught exception).

[Reinmar]

During a short discussion with @pjasiun we agreed that it should be _updateChildren() who updates its text nodes.

Why we can't rely on render() calling _updateText()? Because at that point (before _updateChildren() is executed) text nodes may not be in the same order and places where they are in the view. View to DOM mapping uses the nodes order to find the matching DOM text nodes, so if we tried to update texts at that point we might update completely different text nodes.

Therefore, we prevent updating text nodes which parent elements are about to be updated too. But this means that we may get outdated text nodes in the DOM after _updateChildren().

And therefore, _updateChildren() needs to update text nodes after updating the order of nodes.

[Reinmar]

And therefore, _updateChildren() needs to update text nodes after updating the order of nodes.

It turned out to not be true.

The _updateChildren() uses viewToDom() which always returns a new text node. So:

• if children of elements were modified, this ensures that mapped DOM children are fine.
• if not, then _updateText() is called.

The issue, as found by @szymonkups, is somewhere else. Or at least part of the issue.

Let's consider this example. We're changing:

<p><i><b1>foo</b1></i><b2>bar</b2></p>

to:

<p><b1>foobar</b1></p>

markedChildren contains <p> and <b1>.

First, we call _updateChildren( p ):

1. Remove <i><b1>foo</b1></i>.
2. Remove <b2>bar</b2>.
3. Insert <b1>foo</b1>.

Unfortunately, on step 1, we unbind <b1> (which we already have cached in https://github.com/ckeditor/ckeditor5-engine/blob/e6e92e983b75a036761eb8fc253ebf805d14f771/src/view/renderer.js#L452). But then (in step 3) we insert the unbinded <b1> again which creates a completely invalid DOM which isn't bound to the view and which isn't updated in _updateChildren( b1 ) which is called right after this.

The solution: don't unbind elements which are in the view or will be in the DOM after the changes.

[Reinmar]

While analysing this we also noticed that it's unclear where the renderer uses getCorrespondingDomText(). This method has a very narrow use case because it assumes a lot about the position of the text node that it looks for in the DOM.

So, our idea is to remove getCorrespondingDom() method and use the underlying getCorrespondingDomElement() and getCorrespondingDomText() directly. It will turn out that the latter is used in just one place which is _updateText(). In this case it's safe because _updateText() is only called if parent element wasn't modified at the same time.

We should also warn about this in getCorrespondingDomText()'s documentation.

[Reinmar]

It's also worth linking to https://github.com/ckeditor/ckeditor5-engine/issues/888 as it's the change which introduced unbinding. It may contain interesting information.