Bug 70277 – MoveSelectionCommand::doApply hits an assertion when dragging text without a range selection

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

[Daniel Cheng]

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.

[Devdatta Deshpande]

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.

[Ryosuke Niwa]

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).

[Devdatta Deshpande]

Created attachment 113130 [details]
Patch after incorporating review comments.

Thanks for the review comments.
Please find attached an updated patch.

[Ryosuke Niwa]

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?

[Devdatta Deshpande]

(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.

[Ryosuke Niwa]

(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.

[Devdatta Deshpande]

(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.

[Devdatta Deshpande]

Created attachment 113136 [details]
Patch after incorporating review comments.

Patch updated after review comments.

[Ryosuke Niwa]

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.

[Devdatta Deshpande]

Created attachment 113140 [details]
Updated patch

[WebKit Review Bot]

Comment on attachment 113140 [details]
Updated patch

Clearing flags on attachment: 113140

Committed r99085: <http://trac.webkit.org/changeset/99085>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.