Bug 79173 – dragover's default action should prevent drop for file drags

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Daniel Cheng]

Created attachment 128098 [details]
Test case

Per the spec, both dragenter and dragover need to be cancelled for a drop to take place.

[Alexey Proskuryakov]

See also: bug 66469.

[Daniel Cheng]

*** Bug 99791 has been marked as a duplicate of this bug. ***

[Daniel Cheng]

I'm not sure when this regressed, but apparently not cancelling dragover still results in drops now as well. Ick.

[Daniel Cheng]

Created attachment 169558 [details]
Patch

I have an initial patch. I need to refine it further though; according to the spec, drop events should still happen on things like <input> and <textarea> without requiring the dragover event to be cancelled.

[Build Bot]

Comment on attachment 169558 [details]
Patch

Attachment 169558 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14457372

New failing tests:
http/tests/security/clipboard/clipboard-file-access.html
editing/pasteboard/drag-drop-url-with-style.html
fast/events/moving-text-should-fire-drop-and-dragend-events.html
fast/events/moving-text-should-fire-drop-and-dragend-events-2.html

[WebKit Review Bot]

Comment on attachment 169558 [details]
Patch

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

New failing tests:
fast/events/moving-text-should-fire-drop-and-dragend-events.html
platform/chromium/plugins/drag-events.html
http/tests/security/clipboard/clipboard-file-access.html
fast/events/moving-text-should-fire-drop-and-dragend-events-2.html
editing/pasteboard/drag-drop-url-with-style.html
fast/dom/shadow/drop-event-in-shadow.html
fast/events/input-element-display-none-in-dragleave-crash.html

[Daniel Cheng]

Created attachment 169761 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169761 [details]
Patch

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

New failing tests:
fast/dom/shadow/drop-event-in-shadow.html

[Daniel Cheng]

Created attachment 169788 [details]
Patch

[Tony Chang]

Comment on attachment 169788 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        action of the dragover event. Otherwise, if the cursor is over a text field, editing host,
> +        or an editable element, we also consider the drag to be accepted.

Can you include information about browser compat here? E.g., This makes us match the spec, IE and Opera, but differs from Gecko.  You might also want to include the spec text that describes this behavior (maybe also link to the msdn article about behavior in IE?).

> Source/WebCore/page/DragController.h:124
> +        bool m_documentIsHandlingDrag;

Should we initialize this in the constructor?  I guess it'll always be set by dragEnteredOrUpdated before we read it in performDrag.

> LayoutTests/fast/events/only-valid-drop-targets-receive-drop-expected.txt:8
> +Dropping here should NOT result in a drop event.
> +Dropping here should result in a drop event.
> +Starting drag...
> +Drop not received
> +Starting drag...
> +Drop received

Nit: It would be nice if the text output was clear about which div is which.  E.g., 
Dropping on drop1 should NOT...
Dropping on drop2 should result...
Starting drag to drop1...
Drop not received on drop1.
Starting drag to drop2...
Drop received on drop2.

> LayoutTests/fast/events/only-valid-drop-targets-receive-drop.html:64
> +

Nit: Extra blank line.

[Daniel Cheng]

Upon further investigation, this behavior is specific to file drags. The layout test I added actually worked without the patch.

Currently, WebKit dispatches a drop event without requiring dragenter/dragover for file drags.

Other browsers:
IE: requires dragenter/dragover to be cancelled
Firefox: requires dragenter/dragover to be cancelled
Opera: requires dragenter/dragover to be cancelled

Given that our behavior is in the minority, I'm going to go ahead and land this patch after updating the layout test and addressing the review comments. I personally dislike it as it makes builtin drop handling inconsistent (<textarea> and <input type="text"> don't require dragenter/dragover cancellation to override default drop behavior), but talking with Tony, we're going to prioritize cross-browser compatibility.

[Daniel Cheng]

Created attachment 170709 [details]
More test cases

[Daniel Cheng]

Comment on attachment 169788 [details]
Patch

I'm going to be updating this patch for the following items:
1. Reflect the fact that the layout test I added is only for file drags.
2. Fix the fact that simply cancelling dragover is enough to get a drop event (per the spec, unless you're the body element, you need to cancel both the dragenter/dragover event). This brings us in line with the spec and IE behavior.

[Daniel Cheng]

Created attachment 171032 [details]
Patch

Updated patch. I need to finish testing this on another machine since DRT won't run on my Mac anymore...

[Daniel Cheng]

(In reply to comment #10)
> (From update of attachment 169788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169788&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        action of the dragover event. Otherwise, if the cursor is over a text field, editing host,
> > +        or an editable element, we also consider the drag to be accepted.
> 
> Can you include information about browser compat here? E.g., This makes us match the spec, IE and Opera, but differs from Gecko.  You might also want to include the spec text that describes this behavior (maybe also link to the msdn article about behavior in IE?).
> 

Done.

> > Source/WebCore/page/DragController.h:124
> > +        bool m_documentIsHandlingDrag;
> 
> Should we initialize this in the constructor?  I guess it'll always be set by dragEnteredOrUpdated before we read it in performDrag.
> 

Done. It should always be initialized before we read it... but just to be safe, I initialized it in the ctor.

> > LayoutTests/fast/events/only-valid-drop-targets-receive-drop-expected.txt:8
> > +Dropping here should NOT result in a drop event.
> > +Dropping here should result in a drop event.
> > +Starting drag...
> > +Drop not received
> > +Starting drag...
> > +Drop received
> 
> Nit: It would be nice if the text output was clear about which div is which.  E.g., 
> Dropping on drop1 should NOT...
> Dropping on drop2 should result...
> Starting drag to drop1...
> Drop not received on drop1.
> Starting drag to drop2...
> Drop received on drop2.
> 

Done (partially). I can't think of a good way to do this that works with manual testing, so it only says "dropX" if a drop was received.

> > LayoutTests/fast/events/only-valid-drop-targets-receive-drop.html:64
> > +
> 
> Nit: Extra blank line.

Done.

[Daniel Cheng]

Created attachment 171051 [details]
Patch

[WebKit Review Bot]

Comment on attachment 171051 [details]
Patch

Clearing flags on attachment: 171051

Committed r132716: <http://trac.webkit.org/changeset/132716>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

Hello, fast/events/only-valid-drop-targets-receive-file-drop.html, fails on the Apple-Windows port.

Here's the diff:

 Starting drag...
 Drop received in drop target 1
 Starting drag...
-Drop not received
+FAIL: navigation expected
 
Do have any hunches about what it could be? 
Thanks

[Tony Chang]

(In reply to comment #19)
> Hello, fast/events/only-valid-drop-targets-receive-file-drop.html, fails on the Apple-Windows port.
> 
> Here's the diff:
> 
>  Starting drag...
>  Drop received in drop target 1
>  Starting drag...
> -Drop not received
> +FAIL: navigation expected
> 
> Do have any hunches about what it could be? 
> Thanks

Win DRT is already skipping many drag&drop tests.  It's not clear to me if this failure is new or if Win DRT is missing some testing functionality.

LayoutTests/platform/win/TestExpectations has a comment that points to rdar://5621244 that may provide a reason?