Bug 79172

Created attachment 128321 [details] Patch

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.

Created attachment 128327 [details] Patch

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.

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

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

Has this patch been landed yet?

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

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?

(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

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

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

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

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

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

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

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