Bug 30291 - When effectAllowed = "uninitialized", incorrect dropEffect returned
Summary: When effectAllowed = "uninitialized", incorrect dropEffect returned
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
: 26700 (view as bug list)
Depends on: 31326
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-11 18:15 PDT by Daniel Bates
Modified: 2009-11-12 11:22 PST (History)
4 users (show)

See Also:


Attachments
Layout test (7.14 KB, patch)
2009-10-25 15:16 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Self contained test (11.82 KB, text/html)
2009-10-25 15:20 PDT, Daniel Bates
no flags Details
Patch (4.66 KB, patch)
2009-11-10 12:05 PST, Daniel Bates
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2009-10-11 18:15:32 PDT
Following up from bug #30107, when effectAllowed = "uninitialized", the returned dropEffect is incorrect when the specified dropEffect is "copy", "move" or "link".

Currently, the resulting drop effect is determined by the followings mapping when effectAllowed == "uninitialized":

dropEffect:        Actual drop effect:    Expected drop effect:
"none"     -----------> "none"            "none"
"copy"     -----------> "none"            "copy"
"move"     -----------> "none"            "move"
"link"     -----------> "none"            "link"
"dummy"    -----------> "none"            "none"

As per section 7.9.2 of the HTML 5 specification <http://dev.w3.org/html5/spec/Overview.html#the-dragevent-and-datatransfer-interfaces>, when effectAllowed = "uninitialized" the resulting dropEffect should be the user-specified dropEffect (i.e. "copy", "move", "link") and "none" for any other case.
Comment 1 Daniel Bates 2009-10-25 15:16:45 PDT
Created attachment 41831 [details]
Layout test

DRT-compatible layout test. Can also be manually run.
Comment 2 Daniel Bates 2009-10-25 15:20:51 PDT
Created attachment 41832 [details]
Self contained test

For convenience, here is a self-contained version of the test included in the patch Layout test.
Comment 3 Jens Alfke 2009-11-06 13:45:59 PST
*** Bug 26700 has been marked as a duplicate of this bug. ***
Comment 4 Daniel Bates 2009-11-10 12:05:48 PST
Created attachment 42885 [details]
Patch

No test case is included because we have an existing test case (fast/events/drag-and-drop.html) from bug #24731. We just need to rebase the results of that test case.
Comment 5 Darin Adler 2009-11-10 15:10:41 PST
Comment on attachment 42885 [details]
Patch

r=me

But this test could be improved with one tiny change.

This line:

    shouldEvaluateTo('event.dataTransfer.dropEffect', 'dropEffectElem.options[dropEffectElem.selectedIndex].value');

Should be changed to:

    shouldBe('event.dataTransfer.dropEffect', dropEffectElem.options[dropEffectElem.selectedIndex].value);

This would then make the test output easy to read because you could see the actual result strings instead of the expression over and over again.

I'm really surprised by the shouldEvaluateTo function. I don't know who added it and when, and I think I should look into this a bit more. When the second argument is a string, it's the same as shouldBe, so I'm kind of mystified why we would need it in that case.
Comment 6 Daniel Bates 2009-11-10 17:01:09 PST
Filed a new bug #31326 with regards to cleaning up the output, which was just r+'ed. I was able to get the prettier output by using shouldBeEqualToString as opposed to shouldBe. 

From looking at js-test-pre.js, shouldBe causes an exception to be thrown using the line you provided because of line 102 <http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/resources/js-test-pre.js#L102>, in which eval(_b) is called. And if _b is a string, say "none", then eval("none") will be interpreted as script code, instead of returning a string.

I will rebase the test with the passing of bug #31326 before I land this patch.

Thanks for your suggestion. This definitely makes the test much more beautiful.

(In reply to comment #5)
> (From update of attachment 42885 [details])
> r=me
> 
> But this test could be improved with one tiny change.
> 
> This line:
> 
>     shouldEvaluateTo('event.dataTransfer.dropEffect',
> 'dropEffectElem.options[dropEffectElem.selectedIndex].value');
> 
> Should be changed to:
> 
>     shouldBe('event.dataTransfer.dropEffect',
> dropEffectElem.options[dropEffectElem.selectedIndex].value);
> 
> This would then make the test output easy to read because you could see the
> actual result strings instead of the expression over and over again.
> 
> I'm really surprised by the shouldEvaluateTo function. I don't know who added
> it and when, and I think I should look into this a bit more. When the second
> argument is a string, it's the same as shouldBe, so I'm kind of mystified why
> we would need it in that case.
Comment 7 Daniel Bates 2009-11-12 11:22:20 PST
Committed r50888: <http://trac.webkit.org/changeset/50888>