Bug 19331 – Drag and drop of links in edit mode violates assert in MoveSelectionCommand::doApply()

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Jonathon Jongsma (jonner)]

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.

[Jonathon Jongsma (jonner)]

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.

[Jonathon Jongsma (jonner)]

Created attachment 21484 [details]
An alternate patch that uses the enclosingAnchorElement() function from htmlediting.h instead of traversing the heirarchy manually

[Oliver Hunt]

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.

[Jonathon Jongsma (jonner)]

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.

[Mark Rowe (bdash)]

I can reproduce this on the Mac if I set the editable link behaviour to "Default" or "Always Live".

[Mark Rowe (bdash)]

<rdar://problem/5984433>

[Jonathon Jongsma (jonner)]

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?

[Jonathon Jongsma (jonner)]

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.

[Oliver Hunt]

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.

[Jonathon Jongsma (jonner)]

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)

[Jonathon Jongsma (jonner)]

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.

[Jonathon Jongsma (jonner)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Oliver Hunt]

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.

[Darin Adler]

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.

[Darin Adler]

http://trac.webkit.org/changeset/37534