HTML5 Drag and drop, effectAllowed and dragEffect broken

• Component: DOM
• Status: RESOLVED DUPLICATE

[Erik Arvidsson]

Setting effectAllowed has no effect on the allowed drop effect and the effect allowed is hard coded to copyLink.

This happens on Safari 4 on Windows but works fine on Mac.

[Erik Arvidsson]

Created attachment 31819 [details]
Test case

Change the effectAllowed to link and dropEffect to move (for example). Now you should not be able to drop the image but dropping works as if the effectAllowed was copyLink (deducted by trying different cases).

[Jens Alfke]

The settings of effectAllowed and dropEffect are, in general, not in accordance with the HTML5 spec, and in some cases just broken. The issue Erik reported is one instance of this — there's no code in WebCore itself that restricts the destination-set dropEffect to what the source-set effectAllowed allows.

[Jens Alfke]

Created attachment 38494 [details]
patch

This patch brings the settings of effectAllowed and dropEffect into compliance with the HTML5 spec.
Includes a test case that checks the values of both properties on entry to every standard drag-and-drop event, including the negotiation of a final dropEffect from the source's and destination's requirements.

[Eric Seidel (no email)]

Comment on attachment 38494 [details]
patch

I've looked once, and will look again.  There are several behavior changes all at once here, which will require more careful study.

[Jens Alfke]

Thanks for reviewing, Eric. Any progress? I have a Chromium DnD patch too, that's waiting on this one.
I'll be glad to come over and walk you through the code in person, if that would make the review easier.

[Jens Alfke]

Bug 24731 appears to be a dup (subset) of this one. Someone's just submitted a patch for it, which is probably wasted effort, unfortunately. Can someone please review my patch soon to avoid more duplication?

[Eric Seidel (no email)]

Comment on attachment 38494 [details]
patch

There is a lot changing in this patch, which makes it difficult/scary to review. :(

This is sad:
 45         m_effectAllowed = "uninitialized";
Not your fault, just sad that that's how HTML5 is currently spec'd.

Sadder still:
 105     if (m_effectAllowed.isNull() || m_effectAllowed == "uninitialized")

Does the spec explain why?
 1510                 // Temporarily set dropEffect to 'none' while calling dropleave handler (as per HTML5 spec)

Your test could have been an js-test and would have saved you having to duplicate all the shouldBe/assert logic:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
Even if you didn't convert it to a full js test, you could import the js test standard js files (like js-test-pre.js) to get all the fun stuff.

Setting these to 0 is not really needed:
 var dragStartCalled=0, dragCalled=0, dragEndCalled=0;
 41 var dragEnterCalled=0, dragOverCalled=0, dragLeaveCalled=0, dropCalled=0;
Also I think officially we put spaces around =

You do this a few times:
  119     var dragTarget = document.getElementById(dstId);
 120     x = dragTarget.offsetLeft + dragTarget.offsetWidth / 2;
 121     y = dragTarget.offsetTop + dragTarget.offsetHeight / 2;
 122     eventSender.mouseMoveTo(x, y);

I've written functions in other drag tests which move the mouse to the center of an element.  Seems you want to do the same here.

Why is this needed?
 132     layoutTestController.waitUntilDone();

Not needed if you import the js-test-pre.js stuff or make this a real js-test:
 133     layoutTestController.dumpAsText();

Not needed:
 171     layoutTestController.notifyDone();

r-, mostly for testing nits.  This would be easier to review if fewer functional changes were being made at once.

[Jens Alfke]

>Does the spec explain why?
> 1510                 // Temporarily set dropEffect to 'none'

No; I assume it's just because That's How MSIE Does It.

>Setting these to 0 is not really needed:

I do that so the test will (mostly) work in a regular browser if you manually drag the image into the box. Very useful during development.

>I've written functions in other drag tests which move the mouse to the center
>of an element.  Seems you want to do the same here.

Twice was below my threshold of DRY, but sure, I'll factor it out.

>Why is this needed?
> 132     layoutTestController.waitUntilDone();

The fake dragging stuff apparently can't be done at the time the document is being parsed (I tried it: it crashes.) All the drag-related tests I saw run the test from an onload handler, so I copied what they did.

New patch coming up RSN...

[Jens Alfke]

Created attachment 39230 [details]
Patch with updated test case

I've made changes to the test case, such as using js-test-pre.js.

[Eric Seidel (no email)]

(In reply to comment #8)
> The fake dragging stuff apparently can't be done at the time the document is
> being parsed (I tried it: it crashes.) All the drag-related tests I saw run the
> test from an onload handler, so I copied what they did.

Really?  That sounds like a bug.  Either in DRT or WebCore.  Not one you need to fix, but it would certainly help if you could report it. :)

[Eric Seidel (no email)]

Comment on attachment 39230 [details]
Patch with updated test case

Sigh.  The game of submitting patches when not a contributer and unknown to the project is one of making them easy to r+.   This patch is large and changes a bunch of poorly tested code, which makes reviewing harder... but I think I've stared at enough times to be ready to pull the trigger.  :(


However staring at your test again I'm finding it somewhat hard to read.  And this nit (which would be a non-issue if you were a committer), concerns me:
179 Property changes on: LayoutTests/fast/events/drag-dropeffect.html
2180 ___________________________________________________________________
3181 Name: svn:eol-style
4182   + LF

Your spacing choices in the test don't match my recollection for "webkit style".  Generally I've seen folks put spaces around operators :" as : " for instance.   and "+test as "  + test.

Functions technically are supposed to have { on a new line according to wk style too.  Sadly we don't have a jslint yet (nor is our js styling nearly as strict as our c++ styling or Google's JS styling).

Please file a bug about the DRT issues mentioned above.  Ideally mentioning it in your LayoutTest ChangeLog.

Staring at it, I think I would have probably abstracted the various event handlers, since they could share a bunch of assert code and call counts and preventDefault.  Given the above, I think your and my abstraction thresholds are a bit different. :)

Anyway, I'm gonna r+ this and pray it doesn't break anything.  I think this will have to be landed by hand unless you post another patch w/o the line ending setting.

[Eric Seidel (no email)]

Comment on attachment 39230 [details]
Patch with updated test case

Also we generally don't commit commented out code:
 152       //assert(dragCalled > 0, "drag was called at least once");

I guess this is really an r-, or an r+ w/ mods.  I'm marking it r+ in case you have a committer next to you down in mtv who would like to work with you to land this directly.  Alternatively you can post another patch which can be r+/cq+'d.

[Jens Alfke]

FYI, I filed the DRT crash as bug 29101. It's an assertion failure in DragController.cpp; it seems to be caused by the image element being dragged not having loaded any image resource yet.

[Jens Alfke]

Well, I wouldn't say I'm _completely_ unknown to the project. I fixed a handful of nasty P1 bugs in WebKit while on loan to the Safari team in late 2004, and the Apple folks might remember me from such hits as PubSub.framework and SyndicationUI.framework. :)

Sorry about the size of the patch. It started out small, believe me, but it turned out that fixing the behaviors in question required a bunch of little changes in several places that weren't easily separable.

I'll get those nits fixed and put out a new patch. The svn-eol-style thing I can probably blame on Versions.app, which is a nice SVN GUI client but seems to set funny properties when it adds files.

[Jens Alfke]

Created attachment 39293 [details]
Patch with fixes to test-case nits

OK, I've addressed all the latest comments about the test case (including the svn:eol-style property.)

[Eric Seidel (no email)]

Comment on attachment 39293 [details]
Patch with fixes to test-case nits

Ok, lets all cross our fingers. :)  (Only because drag and drop is notoriously flakey/poorly tested.)

Thanks for the patch!

[WebKit Commit Bot]

Comment on attachment 39293 [details]
Patch with fixes to test-case nits

Rejecting patch 39293 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 39293 [details]
Patch with fixes to test-case nits

Sigh.  You're yet another victim of bug 28624.
media/video-source-error.html -> timed out
Let's try again.

[WebKit Commit Bot]

Comment on attachment 39293 [details]
Patch with fixes to test-case nits

Clearing flags on attachment: 39293

Committed r48229: <http://trac.webkit.org/changeset/48229>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

This broke Windows regression tests, presumably due to the lack of changes in WebKit/win to match those in WebKit/mac.  See bug 29129.

[WebKit Commit Bot]

Committed r48321: <http://trac.webkit.org/changeset/48321>

[Eric Seidel (no email)]

(In reply to comment #22)
> Committed r48321: <http://trac.webkit.org/changeset/48321>

This was the rollout as discussed in bug 29129.

[Jens Alfke]

Got r48229 checked out and built on an XP box. Ran Safari with this WebKit and initially just tried dragging between two textfields ... which immediately crashes in WebDragClient::willPerformDragSourceAction ... just as it has since at least July, when bug 27332 was filed (not by me.)

It seems a little disingenuous to be immediately backing out a patch because it caused Windows test regressions, when in TWO MONTHS no one seems to have investigated the problem that you can't drag anything at all in Safari. You're treating the test shell more seriously than the real application.

[Eric Seidel (no email)]

(In reply to comment #24)
> Got r48229 checked out and built on an XP box. Ran Safari with this WebKit and
> initially just tried dragging between two textfields ... which immediately
> crashes in WebDragClient::willPerformDragSourceAction ... just as it has since
> at least July, when bug 27332 was filed (not by me.)
> 
> It seems a little disingenuous to be immediately backing out a patch because it
> caused Windows test regressions, when in TWO MONTHS no one seems to have
> investigated the problem that you can't drag anything at all in Safari. You're
> treating the test shell more seriously than the real application.

I rolled out the change due to new failures being introduced as a result of the patch.  I didn't investigate the particular failures more than reading bug 29129.

Rolling out regressions is even more normal @ Google than it is @ WebKit. :)  The patch was in the tree for 49 hours before rollout.  It didn't appear anyone was working on fixing the regressions. 

Yup, it's definitely bad that Safari for Windows drag/drop is crashing.

I would be very happy to r+ (as I'm sure many others would) which did not regress existing Windows layout tests.

I would agree that we pay more attention to the test harness than the application.  The test harness is run after every commit, the application is not.  Ideally the test harness catches more than normal application usage would.  Obviously in the case of bug 27332 we need to fix both the framework and the test harness. :)

[Eric Seidel (no email)]

Comment on attachment 39230 [details]
Patch with updated test case

Clearing r+ on this obsolete patch (so that the to-be-committed list doesn't show this patch).

[Jens Alfke]

After this got beyond my depth (I have no Windows programming experience) I handed my patch off to Dan Bates, who was working on similar bugs and is a Windows programmer.

Dan reports that the issues in this bug report have all been addressed by now, except for the remaining bug 30291. So I'll close this as a dup of that.

*** This bug has been marked as a duplicate of bug 30291 ***