Opened 10 years ago

Closed 10 years ago

#12515 closed Bug (fixed)

Cursor position messed up after inserting image followed by undo

Reported by: Jonas Walldén Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.4.6
Component: Core : Undo & Redo Version:
Keywords: Cc: andersjo@…

Description

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.)

Attachments (1)

ck-12515.zip (3.2 KB) - added by Jonas Walldén 10 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by Anders Johansson

Cc: andersjo@… added

comment:2 Changed 10 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.6
Status: newconfirmed

Confirmed. It's a regression in 4.4.5.

comment:3 Changed 10 years ago by 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.

comment:4 Changed 10 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:5 Changed 10 years ago by Piotrek Koszuliński

Version: 4.4.5

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.

comment:6 Changed 10 years ago by 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><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.

Version 1, edited 10 years ago by Artur Delura (previous) (next) (diff)

comment:7 Changed 10 years ago by 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.

comment:8 Changed 10 years ago by 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.

comment:9 Changed 10 years ago by 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 ticket updated, so you'll be notified when there'll be a patch on review.

Last edited 10 years ago by Piotrek Koszuliński (previous) (diff)

comment:10 Changed 10 years ago by Artur Delura

Status: assignedreview

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.

comment:11 Changed 10 years ago by 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.

comment:12 Changed 10 years ago by Piotrek Koszuliński

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

comment:13 Changed 10 years ago by Piotrek Koszuliński

Owner: changed from Artur Delura to 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.

Changed 10 years ago by Jonas Walldén

Attachment: ck-12515.zip added

comment:14 Changed 10 years ago by 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.

comment:15 in reply to:  14 Changed 10 years ago by 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.

Last edited 10 years ago by Piotrek Koszuliński (previous) (diff)

comment:16 Changed 10 years ago by Piotrek Koszuliński

I added your case to #12658's description.

comment:17 Changed 10 years ago by Artur Delura

Resolution: fixed
Status: reviewclosed

Fixed on master with git:ee67715.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy