Bug 79173

See also: bug 66469.

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

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

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.

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

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

Created attachment 169761 [details] Patch

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

Created attachment 169788 [details] Patch

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.

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.

Created attachment 170709 [details] More test cases

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.

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...

(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.

Created attachment 171051 [details] Patch

Comment on attachment 171051 [details] Patch Clearing flags on attachment: 171051 Committed r132716: <http://trac.webkit.org/changeset/132716>

All reviewed patches have been landed. Closing bug.

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

(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?