Summary: | Drag and drop of links in edit mode violates assert in MoveSelectionCommand::doApply() | ||
---|---|---|---|
Product: | WebKit | Reporter: | Jonathon Jongsma (jonner) <jonathon> |
Component: | HTML Editing | Assignee: | Darin Adler <darin> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | justin.garcia, oliver, pierre-luc.beaudoin |
Priority: | P2 | Keywords: | InRadar |
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | Linux | ||
Bug Depends on: | |||
Bug Blocks: | 19386 | ||
Attachments: |
Description
Jonathon Jongsma (jonner)
2008-05-30 14:22:50 PDT
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". 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)
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. |