Bug 61008 – Assertion failed if text field is cleared while drag&drop

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

[Naoki Takano]

if I select and drag text and a dom event fires (e.g., ondragenter) and removes the text I'm dragging, we hit the same assertion.

Attached the manual test file.

1, Run debug version
2, open drag.html
3, select "test" text in left text field and start dragging the selected text.
4, drop the test into the right text field.
5, Assertion failed as following,


        ASSERT(endingSelection().isNonOrphanedRange());
#0	0x0225bddd in WebCore::MoveSelectionCommand::doApply at
MoveSelectionCommand.cpp:42
#1	0x0222c62b in WebCore::EditCommand::apply at EditCommand.cpp:92
#2	0x0222c6db in WebCore::applyCommand at EditCommand.cpp:229
#3	0x023ce0e3 in WebCore::DragController::concludeEditDrag at
DragController.cpp:467
#4	0x023cf053 in WebCore::DragController::performDrag at DragController.cpp:206
#5	0x01a6aa35 in WebKit::WebViewImpl::dragTargetDrop at WebViewImpl.cpp:1937
#6	0x02f63aad in RenderView::OnDragTargetDrop at render_view.cc:3327

[Naoki Takano]

Created attachment 93853 [details]
Test html file.

[Kaustubh Atrawalkar]

Created attachment 111912 [details]
Patch

This was issue with GtkUtilities to calculate screenPoints, gtk2 API was being used for translating window co-ordinates.

[Enrica Casucci]

Comment on attachment 111912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111912&action=review

> Source/WebCore/platform/gtk/GtkUtilities.cpp:44
> +#endif

Are xInWindow and yInWindow in and out parameters? If not, I believe it is confusing to have them initialized in the GTK_API_VERSION_2 case.

[Martin Robinson]

I'm having trouble understanding this change. Is the call to gtk_window_translate_coordinates unnecessary? Are the coordinates correct without it. It's unusual that this would be required for GTK+ 2, but not something similar for GTK+ 3. Furthermore, I don't understand how this leads to an assertion failure.

I'm not saying the patch is wrong, I just don't grok it.

[Kaustubh Atrawalkar]

(In reply to comment #4)
> I'm having trouble understanding this change. Is the call to gtk_window_translate_coordinates unnecessary? Are the coordinates correct without it. It's unusual that this would be required for GTK+ 2, but not something similar for GTK+ 3.

AFAIK, In case of GTK+2, when the text is being dragged there is another small window getting created to hold the text being dragged. And for that the Widget we received in this call has to be translated into its toplevel widget co-ordinates which is not the GTK+3 case.

Furthermore, I don't understand how this leads to an assertion failure.
> 

The assert is thrown in this case as the endingSelection does not find it be in Range as the Position returned by dragData created from the point in convertWidgetPointToScreenPoint is not proper.

> I'm not saying the patch is wrong, I just don't grok it.

[Martin Robinson]

(In reply to comment #5)
> (In reply to comment #4)
> > I'm having trouble understanding this change. Is the call to gtk_window_translate_coordinates unnecessary? Are the coordinates correct without it. It's unusual that this would be required for GTK+ 2, but not something similar for GTK+ 3.
> 
> AFAIK, In case of GTK+2, when the text is being dragged there is another small window getting created to hold the text being dragged. And for that the Widget we received in this call has to be translated into its toplevel widget co-ordinates which is not the GTK+3 case.

This is also the case for GTK+ 3, AFAIK. The small window is the drag icon.

[Kaustubh Atrawalkar]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I'm having trouble understanding this change. Is the call to gtk_window_translate_coordinates unnecessary? Are the coordinates correct without it. It's unusual that this would be required for GTK+ 2, but not something similar for GTK+ 3.
> > 
> > AFAIK, In case of GTK+2, when the text is being dragged there is another small window getting created to hold the text being dragged. And for that the Widget we received in this call has to be translated into its toplevel widget co-ordinates which is not the GTK+3 case.
> 
> This is also the case for GTK+ 3, AFAIK. The small window is the drag icon.

During debug I was getting different points. So I looked around google to check and found this http://stackoverflow.com/questions/2088962/how-do-you-find-the-absolute-position-of-a-gtk-widget-in-a-window Where they suggested to use either of the APIs to find absolute position.

[Kaustubh Atrawalkar]

IMHO, the call gtk_widget_translate_coordinates seems unnecessary. We can remove that. I am not able to test it somehow as I have gtk3 but will try on another machine may be tomorrow. From the suggestion on stackoverflow.com though its not very full proof, and also from Gtk API reference it seems that we should be using either of API.

[Kaustubh Atrawalkar]

Created attachment 113625 [details]
Patch

This bug is duplicate of https://bugs.webkit.org/show_bug.cgi?id=70277 and has been fixed. Adding layout test case for text dragging.

[Ryosuke Niwa]

Comment on attachment 113625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113625&action=review

> LayoutTests/ChangeLog:7
> +

Please explain what kind of test you're adding and that the assertion failure has been fixed in another changeset (including the revision)

> LayoutTests/fast/events/drag-text-with-clear.html:10
> +    if (window.layoutTestController)
> +        layoutTestController.dumpAsText();

Please don't outdent the entire script content.

[Ryosuke Niwa]

Comment on attachment 113625 [details]
Patch

r- due to missing change log entry.

[Kaustubh Atrawalkar]

Created attachment 113627 [details]
Updated Patch

Added ChangeLog explanation for adding test case.

[Kaustubh Atrawalkar]

Created attachment 113628 [details]
Updated Patch

[Kaustubh Atrawalkar]

Thanks Ryosuke for quick review :)

[Ryosuke Niwa]

(In reply to comment #14)
> Thanks Ryosuke for quick review :)

Thanks a lot for adding the test!

[WebKit Review Bot]

Comment on attachment 113628 [details]
Updated Patch

Attachment 113628 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10313077

New failing tests:
fast/events/drag-text-with-clear.html

[Kaustubh Atrawalkar]

(In reply to comment #16)
> (From update of attachment 113628 [details])
> Attachment 113628 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10313077
> 
> New failing tests:
> fast/events/drag-text-with-clear.html

Hi Ryosuke any idea why this must be failing? Do I need to add platform specific things if any ?

[Ryosuke Niwa]

I don't know. Maybe Tony would know?

[Tony Chang]

I tried running this on Chromium Linux, the results include the text "test".  That seems correct to me (ie., the text is moved into the second input area).

What platform does this test work for you?  On Mac, you probably need to add a eventSender.leapForward call initiate a drag.

[Kaustubh Atrawalkar]

(In reply to comment #19)
> I tried running this on Chromium Linux, the results include the text "test".  That seems correct to me (ie., the text is moved into the second input area).
> 
> What platform does this test work for you?  On Mac, you probably need to add a eventSender.leapForward call initiate a drag.
I am working on GTK platform. Guess i might need to add platform specific result?

[Kaustubh Atrawalkar]

Created attachment 114000 [details]
Patch

Updated test case.

[Kaustubh Atrawalkar]

Ryosuke, can you review once? Thanks.

[WebKit Review Bot]

Comment on attachment 114000 [details]
Patch

Clearing flags on attachment: 114000

Committed r100009: <http://trac.webkit.org/changeset/100009>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.