Bug 111556

Summary:

Improve drag&drop of list items in contentEditable divs

Product:

WebKit

Reporter:

Sergio Villar Senin <svillar>

Component:

HTML Editing

Assignee:

Sergio Villar Senin <svillar>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, dglazkov, enrica, mifenton, rniwa, shezbaig.wk, svillar, tkent, tony, webkit.review.bot

Priority:

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	Details Formatted Diff Diff
Patch (8.09 KB, patch) 2013-03-06 12:05 PST, Sergio Villar Senin	no flags	Details Formatted Diff Diff
Patch (8.72 KB, patch) 2013-03-07 09:54 PST, Sergio Villar Senin	rniwa: review+ buildbot: commit-queue-	Details Formatted Diff Diff
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.