Bug 111556

Summary: Improve drag&drop of list items in contentEditable divs
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: HTML EditingAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dglazkov, enrica, mifenton, rniwa, shezbaig.wk, svillar, tkent, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 111853    
Bug Blocks: 110610    
Attachments:
Description Flags
Patch
none
Patch
none
Patch rniwa: review+, buildbot: commit-queue-

Sergio Villar Senin
Reported 2013-03-06 04:11:49 PST
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.
Attachments
Patch (9.65 KB, patch)
2013-03-06 04:56 PST, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Patch (8.09 KB, patch)
2013-03-06 12:05 PST, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Patch (8.72 KB, patch)
2013-03-07 09:54 PST, Sergio Villar Senin 		rniwa: review+
buildbot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2013-03-06 04:56:55 PST
Created attachment 191720 [details] Patch
WebKit Review Bot
Comment 2 2013-03-06 07:00:54 PST
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
Build Bot
Comment 3 2013-03-06 08:15:57 PST
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
Ryosuke Niwa
Comment 4 2013-03-06 11:04:41 PST
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.
Sergio Villar Senin
Comment 5 2013-03-06 11:11:02 PST
(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.
Sergio Villar Senin
Comment 6 2013-03-06 12:05:48 PST
Created attachment 191807 [details] Patch
Build Bot
Comment 7 2013-03-06 12:59:10 PST
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
Sergio Villar Senin
Comment 8 2013-03-07 09:54:54 PST
Created attachment 192026 [details] Patch
Sergio Villar Senin
Comment 9 2013-03-07 10:12:58 PST
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
Ryosuke Niwa
Comment 10 2013-03-07 10:34:21 PST
Comment on attachment 192026 [details] Patch The test looks fantastic now!
Build Bot
Comment 11 2013-03-07 16:58:24 PST
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
Build Bot
Comment 12 2013-03-07 23:10:04 PST
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
Sergio Villar Senin
Comment 13 2013-03-08 02:00:43 PST
Committed r145195: <http://trac.webkit.org/changeset/145195>
Note You need to log in before you can comment on or make changes to this bug.