When in edit mode (using QtWebKit), if I drag a live link with no text selected and then drop it somewhere else on the page, I hit an ASSERT in MoveSelectionCommand::doApply(). I sent a message to the mailing list about this (https://lists.webkit.org/pipermail/webkit-dev/2008-May/004044.html), but I'll copy it here for reference: I'm attempting to add editing support to QtWebKit (e.g. QWebPage::setEditable()), and it seems to work great for basic editing. But while playing around with it, I've noticed some bugs related to drag-and-drop that I'm having trouble chasing down and would appreciate a bit of guidance. The browser I'm testing with does not set anything special for editableLinkBehavior (so it's the default, which means that links are live in edit mode). The first problem that I'm running into is the following: - No text is selected - page is in edit mode - click and drag a link - drop the link somewhere else on the page -> hits ASSERT in MoveSelectionCommand::doApply() - ASSERT(selection.isRange()); So, as far as I can see, the problem is one of the following: A) we're using the wrong command in this situation (we shouldn't be using MoveSelectionCommand because there is no current selection) B) this command should handle the case where there's no selection instead of just asserting it C) the drag should not be allowed unless there is a selection (this one seems wrong to me -- if the editable link behavior is 'live', it seems they should behave just like normal links, which can be dragged without an active selection) D) When a user initiates a drag without anything selected, webkit should automatically expand the selection to the entire contents of the link E) something I haven't thought of.. Which one of the above is the right answer? As I'm relatively new to WebKit development and have very little experience with the editing internals, I would greatly appreciate some advice from people with more experience in this area. Also, if anybody can try to reproduce the behavior described above in any of the other ports (especially mac or windows), I'd be very interested to hear your results.
Created attachment 21438 [details] Patch with a potential fix for link drag and drop ASSERT This patch seems to make things behave more as I would expect them to, but I don't know if this is really the 'right' solution. It seemed like the least bad option to me, but I don't know this code intimately so there may be better options. Feedback appreciated.
Created attachment 21484 [details] An alternate patch that uses the enclosingAnchorElement() function from htmlediting.h instead of traversing the heirarchy manually
Comment on attachment 21484 [details] An alternate patch that uses the enclosingAnchorElement() function from htmlediting.h instead of traversing the heirarchy manually Any fix for this bug, whether it's valid or not, should be at the drag launch site, not the receiver. Eg. If the drag is starting inside a link, and the correct link mode is set, then the caret should be converted to a range containing that link.
Great, thanks for the direction. When you say 'the caret should be converted to a range', do you mean that the link should actually be selected when you start a drag (similar to what it does when you drag an image?). Just trying to clarify whether you're referring to the specific 'Range' class or whether you mean 'range' in a generic sense.
I can reproduce this on the Mac if I set the editable link behaviour to "Default" or "Always Live".
<rdar://problem/5984433>
Thanks for the info mark. Can I prod you for a bit more detail? Now we know that the mac version hits the ASSERT, but when you were not running a debug build (and therefore didn't hit the ASSERT), what was the behavior on mac when editable links were live? - Did the link get properly inserted into the drop location with the correct text and correct link URL? - Did the old link get deleted properly from the original location? - If the link was inserted properly in the drop location, was the new link selected after the drop?
Created attachment 21509 [details] Expand the selection to the enclosing anchor element at the drag launch per comment by Olliver Well, Here's a modified patch that expands the selection at the drag launch site as you suggested. I'm not convinced this is the ideal solution however. In this case, if you start a drag and then abort it (e.g. by pressing 'esc'), you'll end with the link selected when it wasn't selected before the drag. This doesn't seem quite right to me, but I guess it doesn't seem too bad either as it's the same behavior that exists for images.
Comment on attachment 21509 [details] Expand the selection to the enclosing anchor element at the drag launch per comment by Olliver The selection should not be modified unless EditableLinkBehavior is set to an appropriate setting, otherwise the logic looks fine. Minor style issue -- if (node) ... should not have braces as it is a single line block Otherwise this looks good.
I can change the logic to only expand the selection if we're in editing mode, however I was trying to match the behavior of dragging an image. When you start an image drag without any text selected, the image becomes automatically selected, even if you're not in edit mode. So before I update the patch, I just want to confirm that you want these behaviors to be different. Alternatively, I could change the image-dragging behavior to not expand selection if we're not in edit mode. (See prepareClipboardForImageDrag() in DragController.cpp for where the image selection is expanded)
Created attachment 21888 [details] updated patch which only expands selection when in edit mode This patch is brought up-to-date with current trunk and doesn't expand unless we're in edit mode. I didn't touch anything related to images (see my previous comment), so images will still become selected on a drag even when not in edit mode.
Created attachment 21889 [details] Updated coding style (remove braces on one-line if statement) Sorry, forgot your coding style comment on the previous patch. This one should be correct.
Comment on attachment 21889 [details] Updated coding style (remove braces on one-line if statement) Oliver reviewed the last couple patches, assigning to him for final review on this one.
Comment on attachment 21889 [details] Updated coding style (remove braces on one-line if statement) r=me, on this, i'm not sure we can make an automated test for it unforunately due to the drag mode change. Also, my tree is dirty so i'd appreciate it if someone else could land this.
We normally require a test no matter what -- a manual test if not an automated one. But since this was reviewed months ago I guess I will fudge the rules.
http://trac.webkit.org/changeset/37534