Bug 79172 – Having a drop handler prevents navigation on drop even if event is not cancelled

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Cheng]

Download the attached test case, and open it. Attempt to navigate to another page by dragging and dropping a link or another HTML file. Nothing will happen, even though we didn't prevent the default drop event.

[Daniel Cheng]

Created attachment 128321 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 128321 [details]
Patch

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

> Source/WebCore/page/DragController.cpp:-215
> -        if (m_isHandlingDrag || preventedDefault) {

r- because this is not a Chromium specific change. If this bug exists on non-Chromium ports as well, then please rename the bug.

[Daniel Cheng]

Created attachment 128327 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 128327 [details]
Patch

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

> LayoutTests/fast/events/drop-handler-should-not-stop-navigate.html:18
> +    log('SUCCESS');

Please use 'PASS' instead to be consistent with other tests.

> LayoutTests/fast/events/drop-handler-should-not-stop-navigate.html:40
> +eventSender.mouseUp();

We should probably add log('FAIL') and a call to notifyDone() here so that the test will print 'FAIL' instead of timing out when the beforeunload is never fired.

[Alexey Proskuryakov]

What is the rationale for this change? Is there a spec that agrees with both IE and Firefox?

[Daniel Cheng]

(In reply to comment #5)
> What is the rationale for this change? Is there a spec that agrees with both IE and Firefox?

I believe it was an unintentional regression introduced from http://trac.webkit.org/changeset/105396. It also matches the behavior of Firefox and Opera, and all the other event handlers: merely setting an event handler should not be enough to suppress the default action unless you call preventDefault().

[WebKit Review Bot]

Comment on attachment 128327 [details]
Patch

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

New failing tests:
http/tests/security/clipboard/clipboard-file-access.html

[Ryosuke Niwa]

Has this patch been landed yet?

[Daniel Cheng]

Committed r112954: <http://trac.webkit.org/changeset/112954>

[János Badics]

Unfortunately this test fails on Qt.

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/drop-handler-should-not-stop-navigate-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/drop-handler-should-not-stop-navigate-actual.txt 
@@ -1,7 +1,5 @@
+CONSOLE MESSAGE: line 40: TypeError: 'undefined' is not a function (evaluating 'eventSender.beginDragWithFiles(['DRTFakeFile'])')
+FAIL: Timed out waiting for notifyDone to be called
 This tests that a drop handler's default action must be prevented in order to stop navigation. Otherwise, if event.preventDefault() is not called, navigation should occur. To test manually, simply drag and drop another link or HTML file on this page. If navigation occurs, then the test passed.
 
 Starting test
-Cancelling dragenter
-Cancelling dragover
-Not preventing default event on drop.
-PASS

Could you guys take a look at it please?

[Ryosuke Niwa]

(In reply to comment #10)
> Unfortunately this test fails on Qt.

Qt's DRT doesn't support drag & drop:
http://trac.webkit.org/browser/trunk/LayoutTests/platform/qt/Skipped#L398

[János Badics]

> Qt's DRT doesn't support drag & drop:
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/qt/Skipped#L398

Thank you for your quick reply.
Then I am going to skip it.

[Alexander Pavlov (apavlov)]

This change was reverted in http://trac.webkit.org/changeset/113004 as it had resulted in hitting an assert on debug bots (also noticed by the cr-linux EWS bot.)

[Daniel Cheng]

Created attachment 135379 [details]
Fix layout test and remove old assert

[Ryosuke Niwa]

Comment on attachment 135379 [details]
Fix layout test and remove old assert

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

> Source/WebCore/page/DragController.cpp:-425
> -    ASSERT(!m_isHandlingDrag);

Why is it okay to remove this assertion? What does it mean for m_isHandlingDrag to be true here?

[Daniel Cheng]

Comment on attachment 135379 [details]
Fix layout test and remove old assert

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

>> Source/WebCore/page/DragController.cpp:-425
>> -    ASSERT(!m_isHandlingDrag);
> 
> Why is it okay to remove this assertion? What does it mean for m_isHandlingDrag to be true here?

Before, it just meant that the presence of a drop handler prevented all default actions, like dragging a file to a file input control or default text editing operations. Now, we let these default actions run if the drop wasn't canceled, so we don't want this assert anymore.

[Tony Chang]

Comment on attachment 135379 [details]
Fix layout test and remove old assert

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

> Source/WebCore/ChangeLog:8
> +        Only early return if the drop handler prevents the default action.

You might want to mention that this is a regression and the change in which it regressed.

[Ryosuke Niwa]

(In reply to comment #16)
> (From update of attachment 135379 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135379&action=review
> 
> >> Source/WebCore/page/DragController.cpp:-425
> >> -    ASSERT(!m_isHandlingDrag);
> > 
> > Why is it okay to remove this assertion? What does it mean for m_isHandlingDrag to be true here?
> 
> Before, it just meant that the presence of a drop handler prevented all default actions, like dragging a file to a file input control or default text editing operations. Now, we let these default actions run if the drop wasn't canceled, so we don't want this assert anymore.

Please add that to the change log.

[Daniel Cheng]

Committed r113088: <http://trac.webkit.org/changeset/113088>

[Simon Fraser (smfr)]

This is causing a test failure on WK2 bots:
<http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r113190%20(6143)/fast/events/drop-handler-should-not-stop-navigate-pretty-diff.html>

 1CONSOLE MESSAGE: line 38: TypeError: 'undefined' is not a function (evaluating 'eventSender.beginDragWithFiles(['DRTFakeFile'])')
 2FAIL: Timed out waiting for notifyDone to be called

I presume that WebKitTestRunner's eventSender doesn't support eventSender.beginDragWithFiles