Bug 110610 - Empty list items after drag&drop in contentEditable divs
Summary: Empty list items after drag&drop in contentEditable divs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on: 111556
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-22 07:57 PST by Sergio Villar Senin
Modified: 2013-03-14 02:45 PDT (History)
16 users (show)

See Also:


Attachments
Patch (11.56 KB, patch)
2013-03-01 08:47 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (7.26 KB, patch)
2013-03-06 05:23 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (7.88 KB, patch)
2013-03-07 09:38 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (7.88 KB, patch)
2013-03-07 10:09 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (7.88 KB, patch)
2013-03-08 02:16 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2013-03-11 03:02 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (24.04 KB, patch)
2013-03-13 03:25 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (24.78 KB, patch)
2013-03-13 03:50 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (25.43 KB, patch)
2013-03-14 02:08 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2013-02-22 07:57:55 PST
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?
Comment 1 Tony Chang 2013-02-22 14:51:07 PST
Aryeh may have an opinion here.  Also, the current IE/WebKit behavior matches TextEdit on OS X.
Comment 2 Sergio Villar Senin 2013-03-01 08:47:49 PST
Created attachment 190972 [details]
Patch
Comment 3 Sergio Villar Senin 2013-03-01 08:56:08 PST
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
Comment 4 WebKit Review Bot 2013-03-01 09:51:46 PST
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
Comment 5 Build Bot 2013-03-01 15:48:59 PST
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
Comment 6 Build Bot 2013-03-01 17:02:47 PST
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
Comment 7 Sergio Villar Senin 2013-03-06 04:14:14 PST
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
Comment 8 Sergio Villar Senin 2013-03-06 05:23:49 PST
Created attachment 191725 [details]
Patch
Comment 9 Sergio Villar Senin 2013-03-06 05:43:53 PST
(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)
Comment 10 Aryeh Gregor 2013-03-07 04:17:44 PST
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.
Comment 11 Sergio Villar Senin 2013-03-07 09:38:38 PST
Created attachment 192021 [details]
Patch
Comment 12 Sergio Villar Senin 2013-03-07 10:09:50 PST
Created attachment 192036 [details]
Patch
Comment 13 Sergio Villar Senin 2013-03-07 10:13:30 PST
Rebased against the latest changes to the patch for bug 111556
Comment 14 Ryosuke Niwa 2013-03-07 10:34:55 PST
Looks like the patch is still failing to apply?
Comment 15 Sergio Villar Senin 2013-03-07 11:16:52 PST
(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.
Comment 16 Sergio Villar Senin 2013-03-08 02:16:22 PST
Created attachment 192182 [details]
Patch

Resubmitting the patch for EWS analysis now that the dependent bug landed
Comment 17 WebKit Review Bot 2013-03-08 03:16:02 PST
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
Comment 18 Build Bot 2013-03-08 18:35:36 PST
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
Comment 19 Build Bot 2013-03-08 19:17:00 PST
Comment on attachment 192182 [details]
Patch

Attachment 192182 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17114137
Comment 20 Sergio Villar Senin 2013-03-11 03:02:00 PDT
Created attachment 192431 [details]
Patch
Comment 21 Sergio Villar Senin 2013-03-13 03:25:27 PDT
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
Comment 22 Sergio Villar Senin 2013-03-13 03:50:09 PDT
Created attachment 192897 [details]
Patch

Now properly rebased
Comment 23 Ryosuke Niwa 2013-03-13 19:34:51 PDT
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.
Comment 24 Sergio Villar Senin 2013-03-14 02:08:06 PDT
Created attachment 193092 [details]
Patch

Addressing Ryosuke's concerns
Comment 25 Sergio Villar Senin 2013-03-14 02:45:51 PDT
Comment on attachment 193092 [details]
Patch

Clearing flags on attachment: 193092

Committed r145798: <http://trac.webkit.org/changeset/145798>
Comment 26 Sergio Villar Senin 2013-03-14 02:45:59 PDT
All reviewed patches have been landed.  Closing bug.