Bug 70277 - MoveSelectionCommand::doApply hits an assertion when dragging text without a range selection
Summary: MoveSelectionCommand::doApply hits an assertion when dragging text without a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 71460
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-17 15:40 PDT by Daniel Cheng
Modified: 2011-11-03 04:55 PDT (History)
7 users (show)

See Also:


Attachments
Test case (368 bytes, text/html)
2011-10-17 15:40 PDT, Daniel Cheng
no flags Details
Proposed patch (3.85 KB, patch)
2011-10-31 04:09 PDT, Devdatta Deshpande
rniwa: review-
Details | Formatted Diff | Diff
Patch after incorporating review comments. (4.07 KB, patch)
2011-10-31 23:15 PDT, Devdatta Deshpande
no flags Details | Formatted Diff | Diff
Patch after incorporating review comments. (3.99 KB, patch)
2011-11-01 00:30 PDT, Devdatta Deshpande
no flags Details | Formatted Diff | Diff
Updated patch (4.12 KB, patch)
2011-11-01 00:51 PDT, Devdatta Deshpande
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Cheng 2011-10-17 15:40:45 PDT
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.
Comment 1 Devdatta Deshpande 2011-10-31 04:09:30 PDT
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 2 Ryosuke Niwa 2011-10-31 06:45:01 PDT
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).
Comment 3 Devdatta Deshpande 2011-10-31 23:15:30 PDT
Created attachment 113130 [details]
Patch after incorporating review comments.

Thanks for the review comments.
Please find attached an updated patch.
Comment 4 Ryosuke Niwa 2011-10-31 23:42:25 PDT
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?
Comment 5 Devdatta Deshpande 2011-10-31 23:50:26 PDT
(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.
Comment 6 Ryosuke Niwa 2011-11-01 00:04:46 PDT
(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.
Comment 7 Devdatta Deshpande 2011-11-01 00:12:41 PDT
(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.
Comment 8 Devdatta Deshpande 2011-11-01 00:30:40 PDT
Created attachment 113136 [details]
Patch after incorporating review comments.

Patch updated after review comments.
Comment 9 Ryosuke Niwa 2011-11-01 00:39:59 PDT
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.
Comment 10 Devdatta Deshpande 2011-11-01 00:51:08 PDT
Created attachment 113140 [details]
Updated patch
Comment 11 WebKit Review Bot 2011-11-02 11:43:44 PDT
Comment on attachment 113140 [details]
Updated patch

Clearing flags on attachment: 113140

Committed r99085: <http://trac.webkit.org/changeset/99085>
Comment 12 WebKit Review Bot 2011-11-02 11:43:48 PDT
All reviewed patches have been landed.  Closing bug.