Bug 79172

Summary: Having a drop handler prevents navigation on drop even if event is not cancelled
Product: WebKit Reporter: Daniel Cheng <dcheng>
Component: PlatformAssignee: Daniel Cheng <dcheng>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, ap, dglazkov, enrica, jbadics, rniwa, simon.fraser, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 79171    
Attachments:
Description Flags
Patch
none
Patch
none
Fix layout test and remove old assert tony: review+

Description Daniel Cheng 2012-02-21 17:57:13 PST
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.
Comment 1 Daniel Cheng 2012-02-22 16:18:30 PST
Created attachment 128321 [details]
Patch
Comment 2 Ryosuke Niwa 2012-02-22 16:23:42 PST
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.
Comment 3 Daniel Cheng 2012-02-22 16:27:08 PST
Created attachment 128327 [details]
Patch
Comment 4 Ryosuke Niwa 2012-02-22 16:31:42 PST
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.
Comment 5 Alexey Proskuryakov 2012-02-22 16:40:09 PST
What is the rationale for this change? Is there a spec that agrees with both IE and Firefox?
Comment 6 Daniel Cheng 2012-02-22 16:54:25 PST
(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().
Comment 7 WebKit Review Bot 2012-02-22 18:07:52 PST
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
Comment 8 Ryosuke Niwa 2012-04-01 17:25:53 PDT
Has this patch been landed yet?
Comment 9 Daniel Cheng 2012-04-02 15:33:52 PDT
Committed r112954: <http://trac.webkit.org/changeset/112954>
Comment 10 János Badics 2012-04-03 01:44:01 PDT
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?
Comment 11 Ryosuke Niwa 2012-04-03 01:54:43 PDT
(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
Comment 12 János Badics 2012-04-03 02:02:22 PDT
> 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.
Comment 13 Alexander Pavlov (apavlov) 2012-04-03 02:59:42 PDT
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.)
Comment 14 Daniel Cheng 2012-04-03 11:49:20 PDT
Created attachment 135379 [details]
Fix layout test and remove old assert
Comment 15 Ryosuke Niwa 2012-04-03 11:52:20 PDT
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?
Comment 16 Daniel Cheng 2012-04-03 11:54:42 PDT
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.
Comment 17 Tony Chang 2012-04-03 11:55:13 PDT
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.
Comment 18 Ryosuke Niwa 2012-04-03 12:12:08 PDT
(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.
Comment 19 Daniel Cheng 2012-04-03 14:21:58 PDT
Committed r113088: <http://trac.webkit.org/changeset/113088>
Comment 20 Simon Fraser (smfr) 2012-04-04 09:17:05 PDT
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