Mismatch in logic between getByAddress and getIndex regarding empty text nodes

[mweathers]

The symptom I am experiencing is during an Undo, the bookmarks that are being created are not matching up when getByAddress tries to look them up later. I have tracked this down to the fact that "getIndex" inside core/dom/node.js skips blank nodes:

Bypass blank node and adjacent text nodes.

if ( normalized && current != this.$ && current.nodeType == CKEDITOR.NODE_TEXT && ( isNormalizing

!current.nodeValue ) )

continue;

However, getByAddress in document.js does not skip blank text nodes. It seems to compress multiple text nodes into one, but does not skip blank nodes. This means that if I have a blank node above my editor, the undo action is constantly encountering warnings about "<node> is not a descendent of root" that eventually can result in my users losing large amounts of text when they undo. Here is the stack trace of the error that eventually occurs and causes a big problem:

IndexSizeError: Failed to execute 'setEnd' on 'Range': The offset 19 is larger than or equal to the node's length (0).

at Error in native:0 at Object.CKEDITOR.dom.selection.selectRanges in ​http://localhost:5000/ckeditor/core/selection.js:1926:19 at Object.CKEDITOR.dom.selection.selectBookmarks in ​http://localhost:5000/ckeditor/core/selection.js:2069:10 at Object.UndoManager.restoreImage in ​http://localhost:5000/ckeditor/plugins/undo/plugin.js:423:9 at Object.UndoManager.undo in ​http://localhost:5000/ckeditor/plugins/undo/plugin.js:508:18 at Object.CKEDITOR.plugins.add.init.editor.addCommand.exec in ​http://localhost:5000/ckeditor/plugins/undo/plugin.js:29:23 at Object.exec in ​http://localhost:5000/ckeditor/core/command.js:52:35 at CKEDITOR.tools.extend.execCommand in ​http://localhost:5000/ckeditor/core/editor.js:829:38 at Object.onKeyDown in ​http://localhost:5000/ckeditor/core/keystrokehandler.js:56:24 at listenerFirer in ​http://localhost:5000/ckeditor/core/event.js:144:33

My application is an Ember app, so ideally the fix would be to resolve ​https://dev.ckeditor.com/ticket/10411 as it is quite common to have elements appearing/disappearing around the editor in such an application, which would completely mess up the bookmarks. But in the mean time, it would be great to have this particular issue fixed, as I can't remove the empty text node that is causing the problem.

[mweathers]

I don't know if I explained what I was saying about "This means that if I have a blank node above my editor..." very well above. I'm trying to say that I have a blank node that causes getByAddress to wind up in the wrong place in the DOM entirely when it tries to handle the Undo. Because it counts that extra blank node as a separate element, it winds up thinking the address resolves to a menu elsewhere on the page, and eventually causes problems.

I am using the "inline" mode, so the editor is just embedded directly in my page.

[mweathers]

I added a pull request here to show the code I was talking about: ​https://github.com/ckeditor/ckeditor-dev/pull/176

[Piotrek Koszuliński]

Do I understand correctly that the empty node is outside editor? Outside the editor editable?

If so, then I thin there's a more general solution to this issue - #10411. Then getByAddress() would not need to worry about empty nodes, because the idea is that when restoring normalised bookmark2 there should be none in the editable. This is always true inside the editable because bookmark2 is used after setting editable's innerHTML.

[mweathers]

Yes, it is outside the editor editable. I definitely agree that solving #10411 would be better in many ways, and would solve my problem as well -- I just wasn't sure if that would be happening soon or not, and if not, it might make sense to fix this particular issue in the mean time.

Either way, thanks for looking into it!

[Piotrek Koszuliński]

it might make sense to fix this particular issue in the mean time.

We are usually reluctant to make such decisions, because in a long term perspective they mean wasted time and bloating editor with unnecessary code.

You proposed a patch, but now tests need to be written, we need to run them on all browsers, verify that some issue is really fixed and document the change. A lot of side-tasks. Then we need to maintain such patch pretty much forever or remove it when we fix the root issue, but changing behaviour of such low level method as getByAddress twice in a short period of time is never nice for its possible users.

Another thing is that once the issue is partially resolved the pressure to fix the real root of it is lowered. I want to keep it high because it increases the chance that this issue will be fixed. By making PRs you are of course increasing the pressure :). But I can't promise when this issue will be fixed.

[mweathers]

I understand :) Thanks for your time. I was able to get a custom build with my fix in it to use in the mean time (thanks to your nice build script). I think #10411 will be an issue for anyone using Ember since I think it's the Ember outlets that are adding the empty text node, but it sounds like you're aware of it. Feel free to close the PR if you like.

[Jakub Ś]

@mweathers, since this issue is a subset of #10411 we will close this issue as duplicate of that ticket.

Thanks for digging into it.