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: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 111853 | ||||||||||
Bug Blocks: | 110610 | ||||||||||
Attachments: |
|
Description
Sergio Villar Senin
2013-03-06 04:11:49 PST
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> |