| Summary: | Assertion failed if text field is cleared while drag&drop | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Naoki Takano <honten> | ||||||||||||||
| Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | ap, dglazkov, enrica, kaustubh.ra, mrobinson, rniwa, tony, webkit.review.bot | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 93853 [details]
Test html file.
Created attachment 111912 [details]
Patch
This was issue with GtkUtilities to calculate screenPoints, gtk2 API was being used for translating window co-ordinates.
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. 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. (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. (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. (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. 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. 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. 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. Comment on attachment 113625 [details]
Patch
r- due to missing change log entry.
Created attachment 113627 [details]
Updated Patch
Added ChangeLog explanation for adding test case.
Created attachment 113628 [details]
Updated Patch
Thanks Ryosuke for quick review :) (In reply to comment #14) > Thanks Ryosuke for quick review :) Thanks a lot for adding the test! 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 (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 ? I don't know. Maybe Tony would know? 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. (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? Created attachment 114000 [details]
Patch
Updated test case.
Ryosuke, can you review once? Thanks. Comment on attachment 114000 [details] Patch Clearing flags on attachment: 114000 Committed r100009: <http://trac.webkit.org/changeset/100009> All reviewed patches have been landed. Closing bug. |
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