Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12729 closed Bug (fixed)

Incorrect structure created when merging block into list item on Backspace and Delete

Reported by: Boris Lykah Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.8
Component: General Version: 3.6.4
Keywords: Support Cc:

Description

Steps:

  1. Create a list with two items that contain links
  2. Put caret at the beginning of the second item and hit backspace

DOM result:

<a data-cke-saved-href="http://link" href="http://link">
  first
  <a data-cke-saved-href="http://link" href="http://hg">second</a>
</a>

Change History (19)

comment:1 Changed 4 years ago by Boris Lykah

The bug also happens when a link is after the list that has link in the last item.

    ..
    <li><a>link1</a></li>
  </ul>
  <a>|link2</a>

comment:2 Changed 4 years ago by Jakub Ś

Keywords: Webkit Blink added
Status: newconfirmed
Version: 3.6.4

What you are showing us is code that you see with Chrome dev-tools. Once once you switch to Source Mode or use getData() method (used when form is submitted and data is taken from editor or when switching to source mode) - you will see that code gets fixed. Working with such links is also possible - the link plugin recognized and modified both links correctly.

All the above however can be used as workaround. The reality is that CKEditor doesn't act like native browser in this case. I have checked native code and it produced expected two links next to each other while editor nests links.

Problem can be reproduced in Webkit and Blink from CKEditor 3.6.4 rev.[7540].

comment:3 Changed 4 years ago by Wiktor Walc

Keywords: Support added

comment:4 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:5 Changed 4 years ago by Artur Delura

Status: assignedreview

The problem was because merge list item functionality tried to look for last editable element inside previous list sibling.

Which in above example ws <a> tag. Second step was to inject data from removing list item to that element. In described case it ends up with injecting <a> element into another one which is not allowed.

I added conditional statement which catch this case and then move selection outside outer anchor element.

Changes in branch:t/12729.

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:6 Changed 4 years ago by Piotr Jasiun

Status: reviewreview_failed

You check here if the start container is 'a' but there could be another element inside that a (e.g. span with color), so you should check if elements path contains a instead.

Also maybe it is possible it make this fix more generic. It could check if the result html is compatible with the DTD.

comment:7 Changed 4 years ago by Artur Delura

Status: review_failedassigned

comment:8 Changed 4 years ago by Piotrek Koszuliński

See my comment https://gist.github.com/adelura/50749f9c3b32e88c7bb1#comment-1419076

The general solution here is to place the range before the end of the previous _editable_ block element. I stress the "editable" block because of cases like https://gist.github.com/adelura/50749f9c3b32e88c7bb1#comment-1416923 where the very first previous block contains a nested list, so position at its end is not editable.

comment:9 Changed 4 years ago by Artur Delura

Status: assignedreview

My first thought was to implement in similar way. But I didn't connect it with block/inline elements. Changes in branch:t/12729b.

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

Status: reviewreview_failed

I pushed rebased branch:t/12729b. It should be based on master, not major. I also pushed two small commits there.

The structure created by the patched algorithm is correct now. However, the selection is placed incorrectly between inline elements rather than staying in the inline element in which it previously was. It doesn't work on master well too, so it's not a regression fortunately.

However, the patch fixed backspace's behaviour only, while delete (forward delete) is still broken.

<ul>
	<li><a href="http://sdfsdf">sdfdsf^</a></li>
	<li><a href="http://sdfsdf">sdf</a></li>
</ul>

comment:11 Changed 4 years ago by Artur Delura

Status: review_failedassigned

comment:12 Changed 4 years ago by Artur Delura

Status: assignedreview

I pushed changes to a branch:t/12729b. Problem was more general than just nesting anchors. It was caused because before mergins list items, the selection was moved into inline elements. Now it is in first block element.

I also updated tests to check selection as well. So I found out that there are issues related to placing selection after merging list items.

I tried to fix this, and current changes are in branch:t/12729c. I'm going to report an issue for this.

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

Status: reviewreview_failed

I rebased the branch and pushed few additional commits:

  • I removed all the new code that played with bookmarks. This needs to be fixed once for all. Adding now some code which fixes few cases only means that it can be incorrect and only strengthen the incorrect behaviour. It would be confusing for someone working on this issue in the future. For instance - bookmark was created after moving to next line's editable position... why? The bookmark should be created at the caret position - not in the next line.
  • I changed tests' expected patterns so they pass even though there's problem with selection which you reported in #13098.
  • I fixed tests so they pass on FF and IE11 (bogus brs patterns were missing).

That would be pretty much it, but there's one red test in http://tests.ckeditor.dev:1030/tests/plugins/list/backspace_enterBr

comment:14 Changed 4 years ago by Piotrek Koszuliński

Summary: Invalid nested anchorsIncorrect structure created when merging block into list item on Backspace and Delete

comment:15 Changed 4 years ago by Piotrek Koszuliński

Keywords: Webkit Blink removed

comment:16 Changed 4 years ago by Artur Delura

Status: review_failedassigned

comment:17 Changed 4 years ago by Artur Delura

Status: assignedreview

So there is a use case when end path doesn't have block element. Changes in the same branch.

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:18 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Fixed on master with git:765a28c. I made the manual test totally generic, because the whole feature (bakspace/del handling around lists) needed testing.

comment:19 Changed 3 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.8
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy