Bug 110610 – Empty list items after drag&drop in contentEditable divs

• Component: HTML Editing
• Status: RESOLVED FIXED

[Sergio Villar Senin]

Let's consider a very simple example like this one:

<div contenteditable="true">
<ol><li>first</li><li>second</li><li>third</li></ol>
</div>

If we select the first and two items, and then drag&drop the selection to the end of the third one, we'll end up with something like:

1.
2. third
3. first
4. second

The first <li> is not really empty but contains a <br> inside (because the code considers that it needs a placeholder). That's exactly the behaviour of IE but in the same use case Firefox outcome seems more logical to me:

1. third
2. first
3. second

I have created a test case that reproduces this behaviour and have some patch that fixes it. The question is, is this the desired behaviour for webkit?

[Tony Chang]

Aryeh may have an opinion here.  Also, the current IE/WebKit behavior matches TextEdit on OS X.

[Sergio Villar Senin]

Created attachment 190972 [details]
Patch

[Sergio Villar Senin]

So in order to give it some more context, let's put some examples about how this currently works and the proposal behind this patch.

Notation:
  * [ ] determine the selection
  * | position to drop the selection

Current WebKit for a single fully selected <li> does

1. one                 1. one
2. [two]               2. 
3. three|      ===>    3. threetwo
4. four                4. four

while for some of them:

1. one                 1. one
2. [two]               2. 
3. three|      ===>    3. four
4. four                4. two
                       5. three

The (technical) reason for that is that when creating the markup for the drag data, selections involving a single item or several do not work the same way because for the case of multiple items the node tree is traversed to create a markup with the same appearance than the original.

With the changes I'm proposing both cases will work the same way and we'll get rid of those empty items (which actually contain the <br> placeholder) resulting in:

1. one                 1. one
2. [two]               2. three
3. three|      ===>    3. two
4. four                4. four

and for more than one item

1. one                 1. one
2. [two]               2. four
3. three|      ===>    3. two
4. four                4. three

[WebKit Review Bot]

Comment on attachment 190972 [details]
Patch

Attachment 190972 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16872009

New failing tests:
editing/pasteboard/drag-drop-list.html
fast/events/ondragenter.html
editing/selection/drag-list-item.html

[Build Bot]

Comment on attachment 190972 [details]
Patch

Attachment 190972 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16867013

New failing tests:
editing/pasteboard/drag-drop-list.html
fast/events/ondragenter.html
editing/selection/drag-list-item.html

[Build Bot]

Comment on attachment 190972 [details]
Patch

Attachment 190972 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16803358

New failing tests:
editing/selection/drag-list-item.html

[Sergio Villar Senin]

Comment on attachment 190972 [details]
Patch

I decided to split this in two different bugs as I'm fixing two different issues:

* the first one is removing the empty list items (actually items with a <br> placeholder) which will be covered by this bug
* the second one is about homogeinizing the way we treat moving multi and single item selections to be handled in bug 111556

[Sergio Villar Senin]

Created attachment 191725 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #8)
> Created an attachment (id=191725) [details]
> Patch

This won't pass EWS as it depends on the other patch (because I rebaselined the expected results of the new test added in bug 111556)

[Aryeh Gregor]

I never specced drag/drop, so I haven't looked in detail at how browsers behave or anything, and don't have any firm opinions.  Offhand, the change in comment #3 does seem like an improvement.

[Sergio Villar Senin]

Created attachment 192021 [details]
Patch

[Sergio Villar Senin]

Created attachment 192036 [details]
Patch

[Sergio Villar Senin]

Rebased against the latest changes to the patch for bug 111556

[Ryosuke Niwa]

Looks like the patch is still failing to apply?

[Sergio Villar Senin]

(In reply to comment #14)
> Looks like the patch is still failing to apply?

It should because this patch should be applied on top of the one attached to 111556. The reason I did that is because the test added in that bug is used also to check the changes in this one.

[Sergio Villar Senin]

Created attachment 192182 [details]
Patch

Resubmitting the patch for EWS analysis now that the dependent bug landed

[WebKit Review Bot]

Comment on attachment 192182 [details]
Patch

Attachment 192182 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17037402

New failing tests:
editing/pasteboard/drag-drop-list.html
fast/events/ondragenter.html

[Build Bot]

Comment on attachment 192182 [details]
Patch

Attachment 192182 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17060347

New failing tests:
editing/pasteboard/drag-drop-list.html
fast/events/ondragenter.html
editing/selection/selection-modify-crash.html

[Build Bot]

Comment on attachment 192182 [details]
Patch

Attachment 192182 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17114137

[Sergio Villar Senin]

Created attachment 192431 [details]
Patch

[Sergio Villar Senin]

Created attachment 192894 [details]
Patch

This new version adds a new test to editing/moving (new dir). I'm also moving editing/selection/drag-list-item.html to this new location as it fits much better there

[Sergio Villar Senin]

Created attachment 192897 [details]
Patch

Now properly rebased

[Ryosuke Niwa]

Comment on attachment 192897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192897&action=review

> Source/WebCore/ChangeLog:13
> +        Tests: editing/moving/cleanup-on-move.html
> +               editing/moving/drag-list-item.html

Why are we adding new moving directory? These tests clearly belong in pasteboard directory.

> LayoutTests/editing/moving/drag-list-item.html:18
> +    Markup.dump("test", "The original list looks like this. 'two' is going to be selected and pasted after 'four'");
> +    selectAndDragToTarget("test", "two", "two", 4, "four");

It should have been much better if selection part was a different step and we had dumped the markup after we've selected the content
because Markup.dump is going to show that in the dump.

[Sergio Villar Senin]

Created attachment 193092 [details]
Patch

Addressing Ryosuke's concerns

[Sergio Villar Senin]

Comment on attachment 193092 [details]
Patch

Clearing flags on attachment: 193092

Committed r145798: <http://trac.webkit.org/changeset/145798>

[Sergio Villar Senin]

All reviewed patches have been landed.  Closing bug.