RESOLVED FIXED 79172
Having a drop handler prevents navigation on drop even if event is not cancelled
https://bugs.webkit.org/show_bug.cgi?id=79172
Summary Having a drop handler prevents navigation on drop even if event is not cancelled
Daniel Cheng
Reported 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.
Attachments
Patch (4.82 KB, patch)
2012-02-22 16:18 PST, Daniel Cheng
no flags
Patch (4.80 KB, patch)
2012-02-22 16:27 PST, Daniel Cheng
no flags
Fix layout test and remove old assert (6.88 KB, patch)
2012-04-03 11:49 PDT, Daniel Cheng
tony: review+
Daniel Cheng
Comment 1 2012-02-22 16:18:30 PST
Ryosuke Niwa
Comment 2 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.
Daniel Cheng
Comment 3 2012-02-22 16:27:08 PST
Ryosuke Niwa
Comment 4 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.
Alexey Proskuryakov
Comment 5 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?
Daniel Cheng
Comment 6 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().
WebKit Review Bot
Comment 7 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
Ryosuke Niwa
Comment 8 2012-04-01 17:25:53 PDT
Has this patch been landed yet?
Daniel Cheng
Comment 9 2012-04-02 15:33:52 PDT
János Badics
Comment 10 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?
Ryosuke Niwa
Comment 11 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
János Badics
Comment 12 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.
Alexander Pavlov (apavlov)
Comment 13 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.)
Daniel Cheng
Comment 14 2012-04-03 11:49:20 PDT
Created attachment 135379 [details] Fix layout test and remove old assert
Ryosuke Niwa
Comment 15 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?
Daniel Cheng
Comment 16 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.
Tony Chang
Comment 17 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.
Ryosuke Niwa
Comment 18 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.
Daniel Cheng
Comment 19 2012-04-03 14:21:58 PDT
Simon Fraser (smfr)
Comment 20 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
Note You need to log in before you can comment on or make changes to this bug.