Bug 23695 – Data store should be readable in dragstart/copy/cut events

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Eli Grey (:sephr)]

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.

[Eli Grey (:sephr)]

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.

[Eli Grey (:sephr)]

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.

[Erik Arvidsson]

This does not block GTK DnD.

I do agree that it is important.

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

[Daniel Bates]

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 [e-mail], 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.

[Daniel Bates]

Confirmed that crash on site http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer occurs in revisions > 47839.

[Daniel Bates]

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.

Created attachment 40938 [details]
Reduced testcase working in WebKit

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.

[Daniel Bates]

Filed new bug #30266 regarding crasher.

[Daniel Bates]

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.

[Eli Grey (:sephr)]

(In reply to comment #9)

Yes, you're missing getData() in dragstart.

[Ryosuke Niwa]

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?

[Eli Grey (:sephr)]

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

[Ryosuke Niwa]

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

[Eli Grey (:sephr)]

I just found an old testcase for this issue I made a few years ago that still runs properly.

[Eli Grey (:sephr)]

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

[Daniel Cheng]

*** Bug 109865 has been marked as a duplicate of this bug. ***

[Daniel Cheng]

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.

[Early Warning System Bot]

Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17036068

[Early Warning System Bot]

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

[Build Bot]

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

[EFL EWS Bot]

Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17042169

[Andruid Kerne]

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.

[Build Bot]

Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17049301

[Build Bot]

Comment on attachment 191573 [details]
Patch

Attachment 191573 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17047219

[Daniel Cheng]

Created attachment 194415 [details]
Patch

[Tony Chang]

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.

[Tony Chang]

(In reply to comment #28)
> The code change for the bug fix is actually pretty small.  I recomment ...

Err, recommend.

[Daniel Cheng]

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

[Daniel Cheng]

Created attachment 194590 [details]
Patch

[Daniel Cheng]

Created attachment 194606 [details]
Patch

[Daniel Cheng]

Created attachment 194607 [details]
Patch

Fix inverted logic in ClipboardQt and ClipboardWin

[Tony Chang]

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.

[Daniel Cheng]

Created attachment 194618 [details]
Patch

[Tony Chang]

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.

[Daniel Cheng]

Committed r146644: <http://trac.webkit.org/changeset/146644>

[Ryosuke Niwa]

The tests added in this patch are crashing on Qt:
http://build.webkit.org/results/Qt%20Linux%20Release/r146705%20(58724)/results.html