Bug 26699 - [Win] HTML5 Drag and drop, dragend is not fired when pressing Esc
: [Win] HTML5 Drag and drop, dragend is not fired when pressing Esc
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: PC Windows 7
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-06-24 17:36 PST by
Modified: 2009-07-02 18:07 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-24 17:36:56 PST
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 From 2009-06-24 17:38:13 PST -------
Created an attachment (id=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 From 2009-06-24 17:39:45 PST -------
The TC exposes another related issue. If I press Esc over the current page I get a spurious dragstart event.
------- Comment #3 From 2009-06-25 14:16:18 PST -------
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 From 2009-06-30 19:21:55 PST -------
Created an attachment (id=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 From 2009-07-01 06:16:26 PST -------
(From update of attachment 32104 [details])
> 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 From 2009-07-01 11:57:23 PST -------
> 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 From 2009-07-01 12:48:47 PST -------
Created an attachment (id=32133) [details]
Fixed Adam's comments
------- Comment #8 From 2009-07-01 13:38:47 PST -------
(From update of attachment 32133 [details])
r=me
------- Comment #9 From 2009-07-02 18:07:24 PST -------
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