Bug 23695 - Data store should be readable in dragstart/copy/cut events
Summary: Data store should be readable in dragstart/copy/cut events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Daniel Cheng
URL: http://code.eligrey.com/tests/all/dat...
Keywords:
: 109865 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-02-02 13:51 PST by Eli Grey (:sephr)
Modified: 2013-03-22 21:51 PDT (History)
21 users (show)

See Also:


Attachments
Crash dump for http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer (30.48 KB, text/plain)
2009-10-08 23:07 PDT, Daniel Bates
no flags Details
Reduced testcase working in WebKit (796 bytes, text/html)
2009-10-09 02:20 PDT, webkit
no flags Details
Patch (19.99 KB, patch)
2013-03-05 14:52 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (49.37 KB, patch)
2013-03-21 18:48 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (49.36 KB, patch)
2013-03-22 10:45 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (33.65 KB, patch)
2013-03-22 11:46 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (33.65 KB, patch)
2013-03-22 11:50 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (33.89 KB, patch)
2013-03-22 12:34 PDT, Daniel Cheng
tony: review+
tony: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eli Grey (:sephr) 2009-02-02 13:51:30 PST
event.dataTransfer.getData should be usable from a dragstart event in the same domain where the drag initiated. Instead, WebkKit is silently failing. This severely limits the ability to customize the content being dragged, as it would require knowing the content being dragged to customize it.
The URL is practical example of customizing dragged content by adding citations to the source page in various formats for different drop targets (ie. textarea, word processing software, ect) The demo will work correctly in Firefox 3.1.
Comment 1 Eli Grey (:sephr) 2009-03-17 18:29:36 PDT
This is a very important feature of HTML 5 that should be fixed in my opinion and without it, it is impossible to make custom data types for drop operations. event.dataTransfer.setData is useless if you can't get the data for modification. 

For example, it should be possible to set an image ondrag for images where the site knows the metadata of certain images to getData("text/plain") and use this data to do setData("application/json+exif", customizedData) to include said metadata in a custom data type.
Comment 2 Eli Grey (:sephr) 2009-04-27 13:41:07 PDT
I can't find a tracking bug for drag and drop, so I'll just make it blocking GTK drag and drop implementation because it technically does.
Comment 3 Erik Arvidsson 2009-07-21 20:57:48 PDT
This does not block GTK DnD.

I do agree that it is important.
Comment 4 webkit 2009-10-08 04:37:15 PDT
This bug now crashses recent WebKit builds.

See: http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer
Tested on OS X 10.5/6
Comment 5 Daniel Bates 2009-10-08 23:07:28 PDT
Created attachment 40932 [details]
Crash dump for http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer

Following up on the comment made by webkit@leemcdermott.co.uk, this is a crash dump when dragging the box marked "Text" to the drop target on the page: http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer

We may want to spin this crasher off into another bug.
Comment 6 Daniel Bates 2009-10-08 23:39:11 PDT
Confirmed that crash on site http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer occurs in revisions > 47839.
Comment 7 Daniel Bates 2009-10-08 23:44:19 PDT
I misspoke, the crash definitely does not occur in revision 47839, and starts in nightly build revision 47880. So, the crash could have appeared in some revision 47839 < r <= 47880.

(In reply to comment #6)
> Confirmed that crash on site
> http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer occurs
> in revisions > 47839.
Comment 8 webkit 2009-10-09 02:20:43 PDT
Created attachment 40938 [details]
Reduced testcase working in WebKit
Comment 9 webkit 2009-10-09 02:23:28 PDT
Comment on attachment 40938 [details]
Reduced testcase working in WebKit

Ok, I reduced the code as much as I could and managed to get same-domain drag and drop *working* in recent WebKit builds (sans crash). Am I missing something here?!

Testcase should alert 'one', 'two', 'three' if successful.
Comment 10 Daniel Bates 2009-10-09 16:39:16 PDT
Filed new bug #30266 regarding crasher.
Comment 11 Daniel Bates 2009-10-09 16:39:51 PDT
Comment on attachment 40932 [details]
Crash dump for http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer

Obsolete. See bug #30266.
Comment 12 Eli Grey (:sephr) 2012-09-04 19:12:30 PDT
(In reply to comment #9)

Yes, you're missing getData() in dragstart.
Comment 13 Ryosuke Niwa 2013-02-03 13:57:37 PST
I see "one", "two", "three" when I drag & drop in the attached test case on the nightly build of WebKit? Does that mean this bug has been fixed?
Comment 14 Eli Grey (:sephr) 2013-02-03 14:32:36 PST
(In reply to comment #13)
Does that mean this bug has been fixed?

No, the reduced testcase doesn't test for dataTransfer.getData() in a dragstart event. I'm not sure if the URL works as intended anymore for illustrating the issue so there will have to be a new testcase written.

This bug has nothing to do with dnd crashes discussed above, just getData() not returning any data in dragstart events.
Comment 15 Ryosuke Niwa 2013-02-03 18:25:00 PST
(In reply to comment #14)
> (In reply to comment #13)
> Does that mean this bug has been fixed?

It appears so.

> No, the reduced testcase doesn't test for dataTransfer.getData() in a dragstart event. I'm not sure if the URL works as intended anymore for illustrating the issue so there will have to be a new testcase written.

?? Open https://bug-23695-attachments.webkit.org/attachment.cgi?id=40938.

> This bug has nothing to do with dnd crashes discussed above, just getData() not returning any data in dragstart events.

I'm well aware of that.
Comment 16 Eli Grey (:sephr) 2013-02-19 06:58:31 PST
I just found an old testcase for this issue I made a few years ago that still runs properly.
Comment 17 Eli Grey (:sephr) 2013-02-19 07:00:41 PST
(In reply to comment #16)
> ?? Open https://bug-23695-attachments.webkit.org/attachment.cgi?id=40938.

I was referring to the URL I assigned to the issue, not the reduced testcase URL. Anyways, http://code.eligrey.com/tests/all/dataTransfer.getData.html should properly illustrate the issue. Try dragging any data on that page.
Comment 18 Daniel Cheng 2013-02-19 12:54:31 PST
*** Bug 109865 has been marked as a duplicate of this bug. ***
Comment 19 Daniel Cheng 2013-03-05 14:52:02 PST
Created attachment 191573 [details]
Patch

Experimental patch to find EWS failures. Updates the access policy to be a permissions bitmask, and temporarily mangles the names that didn't change to make sure all instances are updated correctly.
Comment 20 Early Warning System Bot 2013-03-05 16:29:41 PST
Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17036068
Comment 21 Early Warning System Bot 2013-03-05 16:35:22 PST
Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17015038
Comment 22 Build Bot 2013-03-05 16:55:48 PST
Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17009200
Comment 23 EFL EWS Bot 2013-03-05 22:59:00 PST
Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17042169
Comment 24 Andruid Kerne 2013-03-06 00:54:21 PST
The name of this bug distracts from part of the problem.

The data should not just be readable in dragstart. It must also be writable!

Firefox does (properly) enable this.
Comment 25 Build Bot 2013-03-06 02:29:14 PST
Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17049301
Comment 26 Build Bot 2013-03-06 07:01:28 PST
Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17047219
Comment 27 Daniel Cheng 2013-03-21 18:48:06 PDT
Created attachment 194415 [details]
Patch
Comment 28 Tony Chang 2013-03-22 10:25:49 PDT
Comment on attachment 194415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194415&action=review

The code change for the bug fix is actually pretty small.  I recomment splitting this into two changes.  First, a refactor change that renames ClipboardWritable to ClipboardReadableAndWritable, adds the can*() methods and switches to using them.  A second change with just the bug fix (should be pretty small at that point).

> Source/WebCore/ChangeLog:18
> +        implementations to check permissions.
> +
> +        Tests: editing/pasteboard/can-read-in-copy-and-cut-events.html

You should also mention that this change allows reading the data store during cut and copy events.

> Source/WebCore/ChangeLog:38
> +        (WebCore::Editor::tryDHTMLCopy):
> +        (WebCore::Editor::tryDHTMLCut):

Please highlight the behavior changes in this part of the ChangeLog.  E.g., the cut/copy bug fix is here.

> Source/WebCore/ChangeLog:48
> +        * page/EventHandler.cpp:
> +        (WebCore::EventHandler::handleDrag):

And this is the drag fix.

> Source/WebCore/dom/Clipboard.cpp:54
> +    ASSERT(m_policy != None || policy == ClipboardNumb);
> +    switch (policy) {
> +    case ClipboardReadable:
> +        m_policy = static_cast<AccessPolicy>(ReadableTypes | ReadableData);
> +        break;

Having m_policy be a different type doesn't seem to be a huge benefit and is a bit confusing.  Why not keep m_policy as a ClipboardAccessPolicy and use the helper methods (canReadTypes(), canReadData(), etc) for checking the different enum values?

> Source/WebCore/dom/Clipboard.cpp:65
> +    default:

No default needed and without it, adding a new value to ClipboardAccessPolicy will trigger a compiler error.
Comment 29 Tony Chang 2013-03-22 10:26:37 PDT
(In reply to comment #28)
> The code change for the bug fix is actually pretty small.  I recomment ...

Err, recommend.
Comment 30 Daniel Cheng 2013-03-22 10:44:51 PDT
(In reply to comment #28)
> (From update of attachment 194415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194415&action=review
> 
> The code change for the bug fix is actually pretty small.  I recomment splitting this into two changes.  First, a refactor change that renames ClipboardWritable to ClipboardReadableAndWritable, adds the can*() methods and switches to using them.  A second change with just the bug fix (should be pretty small at that point).

I think it would be kind of strange to separate it into two changes. If I have a patch that renames ClipboardWritable -> ClipboardReadableAndWritable, that patch would introduce a logically incorrect enum name since the clipboard's not actually readable still without the bug fix...

> 
> > Source/WebCore/ChangeLog:18
> > +        implementations to check permissions.
> > +
> > +        Tests: editing/pasteboard/can-read-in-copy-and-cut-events.html
> 
> You should also mention that this change allows reading the data store during cut and copy events.

Done.

> 
> > Source/WebCore/ChangeLog:38
> > +        (WebCore::Editor::tryDHTMLCopy):
> > +        (WebCore::Editor::tryDHTMLCut):
> 
> Please highlight the behavior changes in this part of the ChangeLog.  E.g., the cut/copy bug fix is here.

Strictly speaking, in the current implementation, this is just a renaming change. The actual behavior fixes are in getData() and friends.

> 
> > Source/WebCore/ChangeLog:48
> > +        * page/EventHandler.cpp:
> > +        (WebCore::EventHandler::handleDrag):
> 
> And this is the drag fix.

Ditto.

> 
> > Source/WebCore/dom/Clipboard.cpp:54
> > +    ASSERT(m_policy != None || policy == ClipboardNumb);
> > +    switch (policy) {
> > +    case ClipboardReadable:
> > +        m_policy = static_cast<AccessPolicy>(ReadableTypes | ReadableData);
> > +        break;
> 
> Having m_policy be a different type doesn't seem to be a huge benefit and is a bit confusing.  Why not keep m_policy as a ClipboardAccessPolicy and use the helper methods (canReadTypes(), canReadData(), etc) for checking the different enum values?

The intent was to reduce the complexity and possible error on the part of external callers. If I just reused the same enum, I'd have to define a bitmask position for the writable bit and a caller could accidentally pass that in or compare against that directly. This could easily happen in some place like EventHandler::createDraggingClipboard or Editor::newGeneralClipboard.

Another alternative is to just add the helper functions and not change any of the existing enum definitions.
bool canReadData() const { return m_policy == ClipboardReadable || m_policy == ClipboardWritable; }

etc

The reason I chose the bitmask is I thought it was a better representation of what the Clipboard actually wanted to enforce internally.

> 
> > Source/WebCore/dom/Clipboard.cpp:65
> > +    default:
> 
> No default needed and without it, adding a new value to ClipboardAccessPolicy will trigger a compiler error.

Done.
Comment 31 Daniel Cheng 2013-03-22 10:45:29 PDT
Created attachment 194590 [details]
Patch
Comment 32 Daniel Cheng 2013-03-22 11:46:34 PDT
Created attachment 194606 [details]
Patch
Comment 33 Daniel Cheng 2013-03-22 11:50:56 PDT
Created attachment 194607 [details]
Patch

Fix inverted logic in ClipboardQt and ClipboardWin
Comment 34 Tony Chang 2013-03-22 12:13:29 PDT
Comment on attachment 194607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194607&action=review

> Source/WebCore/dom/Clipboard.cpp:182
> -    if (m_policy == ClipboardReadable || m_policy == ClipboardTypesReadable)
> -        m_dropEffect = effect;
> +    m_dropEffect = effect;

Is this bug fix related?  Can we do this in a separate patch?

> Source/WebCore/dom/Clipboard.h:121
> +        // Instead of using this member directly, prefer to use the can*() methods above.
>          ClipboardAccessPolicy m_policy;

In the future, we may want to make a class to wrap ClipboardAccessPolicy to make it impossible to use m_policy incorrectly.

> Source/WebCore/platform/blackberry/ClipboardBlackBerry.cpp:61
> -    if (policy() != ClipboardReadable)
> +    if (!canReadData())

I see, this is the getData bug fix: ClipboardTypesReadable and ClipboardWritable now allow getData.  Please mention this in the ChangeLog.
Comment 35 Daniel Cheng 2013-03-22 12:34:54 PDT
Created attachment 194618 [details]
Patch
Comment 36 Tony Chang 2013-03-22 12:37:24 PDT
Comment on attachment 194618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194618&action=review

> Source/WebCore/ChangeLog:33
> +        (WebCore::ClipboardBlackBerry::getData):

Please mention that the fix is here.
Comment 37 Daniel Cheng 2013-03-22 13:06:38 PDT
Committed r146644: <http://trac.webkit.org/changeset/146644>
Comment 38 Ryosuke Niwa 2013-03-22 21:51:59 PDT
The tests added in this patch are crashing on Qt:
http://build.webkit.org/results/Qt%20Linux%20Release/r146705%20(58724)/results.html