Cursor position messed up after inserting image followed by undo

[Jonas Walldén]

Tested with ​http://ckeditor.com/demo (which reports v4.4.5) using Safari 7.1 (9537.85.10.17.1).

Steps to reproduce:

1. Visit ​http://ckeditor.com/demo.
2. Type some text, e.g. "foo".
3. Click "Image" in the toolbar and pick one of the server-side ones.
4. Type a bit more text directly following the image, e.g. "bar".
5. Click the "Undo" toolbar button.

After step 5 the text entered last is removed, but the insertion point moves before the first text instead of remaining after the image.

(Attempting Redo at this time also positions the cursor incorrectly, so it seems several snapshots are affected.)

[Piotrek Koszuliński]

Confirmed. It's a regression in 4.4.5.

[Artur Delura]

To reproduce this issue we have to preload image in image dialog (we can do this by blur src input). If we don't then issue break when starting redo. Same behaviour on Chrome.

[Piotrek Koszuliński]

Ok, I'm able to reproduce it from 4.0.0 and perhaps earlier.

What's very surprising, it really makes a difference whether I type a working image URL or some that gives 404 error.

[Artur Delura]

The problem is that after creating image using image dialog right after image dialog there is added empty text node.

When we are going to make a snapshot we end up with something like this:

contents: <p>foo<img src="some.jps"></p>

and bookmark path like [1,0,2]

1 - body

0 - paragraph

2 - empty text node which is not considered in contents

When undomanager tries to restore image empty text node is not created after image element. But bookmarks says that selection is at index 2 and at this index nothing exists. We end up with silent out of range exception which put caret at the beginnig.

Question is whether empty text node should be created or not? I think that empty text node is valid, so we should handle it when restoring image. Such fix is very general - it's good.

So while restoring image described above there should be created an extra empty text node.

[Piotrek Koszuliński]

There are two possible scenarios that happen:

1. The text node isn't empty, but it contains only ZWS. That would explain why this issue occurs on Webkit and Blink.
2. There really is an empty text node created when the image is inserted at the end of the "foo" text node.

In both cases the range.createBookmark2 function executed with normalized argument set to true should be able to handle this.

I checked that in tests/dom/range/bookmarks.js there's no test for this case. You can create one and check what happens. Although, you won't be able to create this test using the addBookmarks2TCs function, because it won't to generate it. So you need to write it more manually.

[Jonas Walldén]

As the original poster I'd like to elaborate that the steps to reproduce I posted initially only includes the Image picker dialog box for simplicity's sake. In our real use case we use a sequence like

editor.execCommand("undo");
editor.insertHtml("...<img>...");

to replace the most recently entered text (originating from the Special Characters dialog box) with an image tag that we construct programmatically. It's on the second execution of that sequence that CK 4.4.5 starts to exhibit the described problem with the misplaced cursor. (I'm pretty sure we didn't encounter this issue in CK 4.3.x but I don't have a test setup at hand to state this with full confidence.)

In other words, any fixes to the Image picker dialog that you might be looking at won't address the underlying problem we're experiencing. If you feel this clarification counts as a different CK bug I'd be happy to open another ticket and/or provide more real code to assist in tracking down the problem. Conversely, if you have a fix candidate I can quickly verify it against our code base to confirm a successful resolution.

[Piotrek Koszuliński]

Thanks for more details. You're right that the root of the problem isn't in the image dialog and we are not planning to touch it. We think that the problem lays in the range.createBookmark2 method which is heavily used by the undo manager. It has a problem with selection which is located in an empty text node and we are working on fix for this issue.

It's very likely that this issue has not been reproducible before because something changed not in the editor, but in the browsers. Webkit and Blink are very similar and still some patches are ported from one to another engine.

We'll of course keep this issue updated, so you'll be notified when there'll be a patch on review.

[Artur Delura]

Changes and tests on ​branch:t/12515. The main problem was in range.createBookmark2. Bookmark offset have been broken while normalization process.

Unfortunatelly there is an browser bug, which still breaks {{redo}} functionality. When getting selection range, it returns wrong values. I'm gonna create a new ticket for this.

[Jonas Walldén]

Thanks for the patch. I downloaded the 12515 branch locally, ran the builder script and dropped it into our application. Unfortunately it did not solve my test case. I will try to provide a minimized code example soon.

[Piotrek Koszuliński]

The patch works for me. Although, it needs a thorough review.

[Piotrek Koszuliński]

I reviewed the patch and changed some fragments:

1. I started from simplifications and API docs.
2. Then I extracted the getIndex( false ) case to a fast track. This case must be fast, because it's heavily used.
3. Then I found out that some old getIndex test were incorrect, because they should return -1 but they didn't. I fixed that.
4. Then I wanted to simplify the code in getIndex and I achieved that by handling empty text node case separately.
5. The refactoring made in 4. allowed me to merge back the getIndex( false ) case into the normal execution flow.
6. Then I fixed the createBookmark2 method because the change there wasn't good. The new solution is longer but it fix more cases what was proved by new tests. I also avoided using document.getByAddress about which I'm very uncertain.
7. The last thing was workarounding IE issues... ​https://twitter.com/reinmarpl/status/532891840878755840

This ticket needs another review after my changes. The code is pushed to ​branch:t/12515.

[Jonas Walldén]

Previous comment contains a minimal test page to reproduce the issue I mentioned in comment 8. It also fails on the updated branch in comment 13.

[Piotrek Koszuliński]

Replying to jonasw:

Previous comment contains a minimal test page to reproduce the issue I mentioned in comment 8. It also fails on the updated branch in comment 13.

Thanks for taking time to create this sample.

However, this is a different TC and a different issue. It's very likely that this is related to #12658, so I'll move your scenario there. In this ticket we are going to fix the issue which you originally reported in the ticket description.

[Piotrek Koszuliński]

I added your case to #12658's description.

[Artur Delura]

Fixed on master with ​git:ee67715.