Created attachment 111333 [details] Test case In the attached test case, click in the content editable area to the right of the image, so the insertion point is after the image. Then drag the link to the editable area, and ASSERT(endingSelection().isNonOrphanedRange()); fires.
Created attachment 113037 [details] Proposed patch A MoveSelectionCommand is fired if an editable element is just focused even without a selection. The attached patch avoids firing of MoveSelectionCommand in case of a caret or no-selection. A ReplaceSelectionCommand is fired instead.
Comment on attachment 113037 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=113037&action=review I'd say r- due to various nits. > LayoutTests/fast/events/drag-link.html:5 > + function dragElement(srcElement, destElement) { Indenting scripts like this seems unnecessary. > LayoutTests/fast/events/drag-link.html:7 > + srcElement.offsetTop + srcElement.offsetHeight / 2); Wrong indentation. It should be eventSender.mouseMoveTo(srcElement.offsetLeft + srcElement.offsetWidth / 2, srcElement.offsetTop + srcElement.offsetHeight / 2); > LayoutTests/fast/events/drag-link.html:11 > + eventSender.mouseMoveTo(destElement.offsetLeft + destElement.offsetWidth / 2, > + destElement.offsetTop + destElement.offsetHeight / 2); Ditto. > LayoutTests/fast/events/drag-link.html:17 > + window.onload = function() { > + if (!window.layoutTestController) > + return; Do we really need to wait until the parsing has ended? Can't we just move the script element belog #result and run this function directly there? > LayoutTests/fast/events/drag-link.html:29 > +<div> Please explain what kind of test this is. In particular, some platform doesn't implement all features of eventSender, so it's crucial that you explain that this test is about dragging text not being enabled when selection is not a range. > LayoutTests/ChangeLog:11 > + Test to drag-drop anchor element on an already focused editable div > + element. Please add this description after Reviewed by (followed by a blank line) before the list of files being added (followed by a blank line).
Created attachment 113130 [details] Patch after incorporating review comments. Thanks for the review comments. Please find attached an updated patch.
Comment on attachment 113130 [details] Patch after incorporating review comments. View in context: https://bugs.webkit.org/attachment.cgi?id=113130&action=review > LayoutTests/fast/events/drag-link.html:17 > + function dragElement(srcElement, destElement) > + { As I said, I don't think we want to indent the entire script like this. > LayoutTests/fast/events/drag-link.html:34 > + > + document.getElementById("result").innerHTML = "PASS"; This test always passes?
(In reply to comment #4) > (From update of attachment 113130 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113130&action=review > > > LayoutTests/fast/events/drag-link.html:17 > > + function dragElement(srcElement, destElement) > > + { > > As I said, I don't think we want to indent the entire script like this. > Sorry I misinterpreted the comment. I will remove one level indentation in the script. > > LayoutTests/fast/events/drag-link.html:34 > > + > > + document.getElementById("result").innerHTML = "PASS"; > > This test always passes? This test results into an assert if the patch is not applied, and if the drag-drop fails the 'Test link' will not be added into the actual result. Thus I think the result div is redundant and can be removed. Also, I believe this will fix bug 61008 as well.
(In reply to comment #5) > > > LayoutTests/fast/events/drag-link.html:34 > > > + > > > + document.getElementById("result").innerHTML = "PASS"; > > > > This test always passes? > This test results into an assert if the patch is not applied, and if the drag-drop fails the 'Test link' will not be added into the actual result. Thus I think the result div is redundant and can be removed. Given that the assertion failure never hits on release builds, you probably want to clarify that the test passes only if it does not hit assertions. > Also, I believe this will fix bug 61008 as well. That's nice to know. Ideally we can add a test for the bug 61008 after this patch was landed.
(In reply to comment #6) > (In reply to comment #5) > > > > LayoutTests/fast/events/drag-link.html:34 > > > > + > > > > + document.getElementById("result").innerHTML = "PASS"; > > > > > > This test always passes? > > This test results into an assert if the patch is not applied, and if the drag-drop fails the 'Test link' will not be added into the actual result. Thus I think the result div is redundant and can be removed. > > Given that the assertion failure never hits on release builds, you probably want to clarify that the test passes only if it does not hit assertions. > Yes thats true. I will add a comment in the html file.
Created attachment 113136 [details] Patch after incorporating review comments. Patch updated after review comments.
Comment on attachment 113136 [details] Patch after incorporating review comments. View in context: https://bugs.webkit.org/attachment.cgi?id=113136&action=review > LayoutTests/fast/events/drag-link.html:26 > +} It'll be still nice to print PASS here so that we know for sure dragElement ran.
Created attachment 113140 [details] Updated patch
Comment on attachment 113140 [details] Updated patch Clearing flags on attachment: 113140 Committed r99085: <http://trac.webkit.org/changeset/99085>
All reviewed patches have been landed. Closing bug.