WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(4.80 KB, patch)
2012-02-22 16:27 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Fix layout test and remove old assert
(6.88 KB, patch)
2012-04-03 11:49 PDT
,
Daniel Cheng
tony
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Daniel Cheng
Comment 1
2012-02-22 16:18:30 PST
Created
attachment 128321
[details]
Patch
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
Created
attachment 128327
[details]
Patch
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
Committed
r112954
: <
http://trac.webkit.org/changeset/112954
>
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
Committed
r113088
: <
http://trac.webkit.org/changeset/113088
>
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.
Top of Page
Format For Printing
XML
Clone This Bug