Bug 26699 - [Win] HTML5 Drag and drop, dragend is not fired when pressing Esc
Summary: [Win] HTML5 Drag and drop, dragend is not fired when pressing Esc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-24 17:36 PDT by Erik Arvidsson
Modified: 2009-07-02 18:07 PDT (History)
4 users (show)

See Also:


Attachments
Test case (1.93 KB, text/html)
2009-06-24 17:38 PDT, Erik Arvidsson
no flags Details
Patch (4.38 KB, patch)
2009-06-30 19:21 PDT, Erik Arvidsson
aroben: review-
Details | Formatted Diff | Diff
Fixed Adam's comments (4.69 KB, patch)
2009-07-01 12:48 PDT, Erik Arvidsson
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2009-06-24 17:36:56 PDT
A page that has HTML5 drag and drop support and listens to the dragend event will not get a callback if the user presses the escape key while dragging and the cursor is outside the current window.

This makes cleaning up from drag operations very hard.

This happens on Safari 4 and Chrome on Windows 7. It works fine for Safari 4 on Mac.

TC coming.
Comment 1 Erik Arvidsson 2009-06-24 17:38:13 PDT
Created attachment 31818 [details]
Test case

Try dragging one of the images. Drag it outside the current browser window and press the escape key. Note that we do not get a dragend event in this case.
Comment 2 Erik Arvidsson 2009-06-24 17:39:45 PDT
The TC exposes another related issue. If I press Esc over the current page I get a spurious dragstart event.
Comment 3 Erik Arvidsson 2009-06-25 14:16:18 PDT
One more bug in the same spirit.

1. Drag an image out of the window.
2. Press Esc (but don't let go of the mouse button)
3. Move back over the window and the cancelled drag operation is resurrected with a new dragstart :'(
Comment 4 Erik Arvidsson 2009-06-30 19:21:55 PDT
Created attachment 32104 [details]
Patch

Calls dragSourceEndedAt when fEscapePressed is true in WebDropSource::QueryContinueDrag so we can fire the dragend event correctly.

When we end the drag and drop we set m_mouseDownMayStartDrag to false so that the mouse move after the dnd has ended does not start a new dnd session.
Comment 5 Adam Roben (:aroben) 2009-07-01 06:16:26 PDT
Comment on attachment 32104 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 45412)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> +2009-06-30  Erik Arvidsson  <arv@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixes issue where escape did not cancel the drag and drop operation on windows.

You should add the URL and title of this bug to your ChangeLogs.

> Property changes on: WebCore/manual-tests/drag-escape.html
> ___________________________________________________________________
> Added: svn:executable
>    + *

Please remove the executable bit. It would also be good to set svn:eol-style to native.

> 
> Index: WebCore/page/EventHandler.cpp
> ===================================================================
> --- WebCore/page/EventHandler.cpp	(revision 45393)
> +++ WebCore/page/EventHandler.cpp	(working copy)
> @@ -2102,6 +2102,7 @@ void EventHandler::dragSourceEndedAt(con
>      }
>      freeClipboard();
>      dragState().m_dragSrc = 0;
> +    m_mouseDownMayStartDrag = false;
>  }

It's probably worth adding a comment here about why this is needed (i.e., to handle situations where the drag is cancelled while the mouse button is still down).

>  STDMETHODIMP WebDropSource::QueryContinueDrag(BOOL fEscapePressed, DWORD grfKeyState)
>  {
> -    if(fEscapePressed)
> -        return DRAGDROP_S_CANCEL;
> -
> -    if(!(grfKeyState & (MK_LBUTTON|MK_RBUTTON))) {
> -        m_dropped = true;
> +    if(fEscapePressed || !(grfKeyState & (MK_LBUTTON|MK_RBUTTON))) {

Please add a space after "if" while you're modifying this line.

> +        m_dropped = !fEscapePressed;
>          if (Page* page = m_webView->page())
>              if (Frame* frame = page->mainFrame()) 
>                  //FIXME: We need to figure out how to find out what actually happened in the drag <rdar://problem/5015961>
>                  frame->eventHandler()->dragSourceEndedAt(generateMouseEvent(m_webView.get(), false), DragOperationCopy);

It doesn't seem right to pass DragOperationCopy if Escape was pressed.

> -        return DRAGDROP_S_DROP;
> +		return fEscapePressed? DRAGDROP_S_CANCEL : DRAGDROP_S_DROP;

Tabs! Please use 4-space indents.

Thanks for looking at this!
Comment 6 Eric Seidel (no email) 2009-07-01 11:57:23 PDT
> Property changes on: WebCore/manual-tests/drag-escape.html
> ___________________________________________________________________
> Added: svn:executable
>    + *

Please remove the executable bit. It would also be good to set svn:eol-style to
native.

We need some sort of script to remove these for you.  Windows SVN adds them *all the time*.  Windows SVN also gets the mime types of added images wrong too.

I've filed https://bugs.webkit.org/show_bug.cgi?id=26895 to track creation of such a script.
Comment 7 Erik Arvidsson 2009-07-01 12:48:47 PDT
Created attachment 32133 [details]
Fixed Adam's comments
Comment 8 Adam Roben (:aroben) 2009-07-01 13:38:47 PDT
Comment on attachment 32133 [details]
Fixed Adam's comments

r=me
Comment 9 Eric Seidel (no email) 2009-07-02 18:07:24 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/manual-tests/drag-escape.html
	M	WebCore/page/EventHandler.cpp
	M	WebKit/win/ChangeLog
	M	WebKit/win/WebDropSource.cpp
Committed r45515
	M	WebKit/win/ChangeLog
	M	WebKit/win/WebDropSource.cpp
	M	WebCore/ChangeLog
	M	WebCore/page/EventHandler.cpp
	A	WebCore/manual-tests/drag-escape.html
r45515 = e9a38090ac268a71d0631db21ea0212e575d7316 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/45515