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?
Aryeh may have an opinion here. Also, the current IE/WebKit behavior matches TextEdit on OS X.
Created attachment 190972 [details] Patch
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 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 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 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 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
Created attachment 191725 [details] Patch
(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)
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.
Created attachment 192021 [details] Patch
Created attachment 192036 [details] Patch
Rebased against the latest changes to the patch for bug 111556
Looks like the patch is still failing to apply?
(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.
Created attachment 192182 [details] Patch Resubmitting the patch for EWS analysis now that the dependent bug landed
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 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 on attachment 192182 [details] Patch Attachment 192182 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17114137
Created attachment 192431 [details] Patch
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
Created attachment 192897 [details] Patch Now properly rebased
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.
Created attachment 193092 [details] Patch Addressing Ryosuke's concerns
Comment on attachment 193092 [details] Patch Clearing flags on attachment: 193092 Committed r145798: <http://trac.webkit.org/changeset/145798>
All reviewed patches have been landed. Closing bug.