This bug comes from bug 110610 where I was trying to fix two issues at the same time, I think it's better to treat them separatedely. This one is about some inconsistency when drag&drop list items in content editable divs. 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. alpha 1. alpha 2. [beta] 2. 3. gamma| ===> 3. gammabeta 4. delta 4. delta while for some of them: 1. alpha 1. alpha 2. [beta 2. 3. gamma] ===> 3. delta 4. delta| 4. beta 5. gamma 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. Thus the first case will behave like this: 1. alpha 1. alpha 2. [beta] 2. 3. gamma| ===> 3. gamma 4. delta 4. beta 5. delta The empty list items actually contain a placeholder (<br>). In bug 110610 I'm proposing to remove them as well.
Created attachment 191720 [details] Patch
Comment on attachment 191720 [details] Patch Attachment 191720 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17015166 New failing tests: editing/selection/drag-list-item.html
Comment on attachment 191720 [details] Patch Attachment 191720 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17072080 New failing tests: editing/selection/drag-list-item.html
Comment on attachment 191720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191720&action=review > Source/WebCore/editing/markup.cpp:518 > + // If listItem contents are fully selected elements then wrap them with <li> to retain structure and appearance I think the code is self-evident enough that we wouldn't need this comment. > LayoutTests/editing/selection/drag-list-item.html:1 > +<html> Missing DOCTYPE > LayoutTests/editing/selection/drag-list-item.html:25 > + var elem = document.getElementById(testElement); Please don't use abbreviations like elem. Spell out the word. > LayoutTests/editing/selection/drag-list-item.html:35 > + eventSender.mouseDown(); You probably need a bunch of leapForward's here. > LayoutTests/editing/selection/drag-list-item.html:42 > + var target = document.getElementById(targetElement); > + var targetx = target.parentNode.offsetLeft + target.offsetLeft + target.offsetWidth / 2; > + var targety = target.offsetTop + target.offsetHeight / 2; > + > + eventSender.mouseMoveTo(targetx, targety); > + eventSender.mouseUp(); When does this test pass/fail? It's totally unclear from the test output. My suggestion is to use dump-as-markup.js. See other tests in editing that uses it.
(In reply to comment #4) > (From update of attachment 191720 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191720&action=review > > > Source/WebCore/editing/markup.cpp:518 > > + // If listItem contents are fully selected elements then wrap them with <li> to retain structure and appearance > > I think the code is self-evident enough that we wouldn't need this comment. Fair enough > > LayoutTests/editing/selection/drag-list-item.html:1 > > +<html> > > Missing DOCTYPE OK > > LayoutTests/editing/selection/drag-list-item.html:25 > > + var elem = document.getElementById(testElement); > > Please don't use abbreviations like elem. Spell out the word. > > > LayoutTests/editing/selection/drag-list-item.html:35 > > + eventSender.mouseDown(); > > You probably need a bunch of leapForward's here. Yeah, I had a leapForward(500) to perform the drag&drop but somehow I accidentally remove it in this last version. > > LayoutTests/editing/selection/drag-list-item.html:42 > > + var target = document.getElementById(targetElement); > > + var targetx = target.parentNode.offsetLeft + target.offsetLeft + target.offsetWidth / 2; > > + var targety = target.offsetTop + target.offsetHeight / 2; > > + > > + eventSender.mouseMoveTo(targetx, targety); > > + eventSender.mouseUp(); > > When does this test pass/fail? It's totally unclear from the test output. > My suggestion is to use dump-as-markup.js. See other tests in editing that uses it. Well, as explained in the description, current webkit does: 1. one 1. one 2. [two] => 2. 3. three| 3. threetwo when dragging "two" to the end of "three". With this patch we'd get 1. one 2. three 3. two as outcome. I used this type of output because I wanted to be sure that we weren't adding additional nesting levels in the lists, but maybe that can be also achieved with what you propose.
Created attachment 191807 [details] Patch
Comment on attachment 191807 [details] Patch Attachment 191807 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17056112 New failing tests: editing/selection/drag-list-item.html
Created attachment 192026 [details] Patch
Updated test expectations as requested by rniwa on IRC, basically: * replaced alpha, beta, etc... by one, two... * the test expectations include now the initial state of the lists and the expected results
Comment on attachment 192026 [details] Patch The test looks fantastic now!
Comment on attachment 192026 [details] Patch Attachment 192026 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17046406 New failing tests: editing/selection/drag-list-item.html
Comment on attachment 192026 [details] Patch Attachment 192026 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16992554 New failing tests: editing/selection/selection-modify-crash.html
Committed r145195: <http://trac.webkit.org/changeset/145195>