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.
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.
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.
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
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.
Confirmed that crash on site http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer occurs in revisions > 47839.
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.
Filed new bug #30266 regarding crasher.
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.
(In reply to comment #9) Yes, you're missing getData() in dragstart.
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?
(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.
(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.
I just found an old testcase for this issue I made a few years ago that still runs properly.
(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.
*** Bug 109865 has been marked as a duplicate of this bug. ***
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 on attachment 191573 [details] Patch Attachment 191573 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17036068
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 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 on attachment 191573 [details] Patch Attachment 191573 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17042169
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 on attachment 191573 [details] Patch Attachment 191573 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17049301
Comment on attachment 191573 [details] Patch Attachment 191573 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17047219
Created attachment 194415 [details] Patch
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.
(In reply to comment #28) > The code change for the bug fix is actually pretty small. I recomment ... Err, recommend.
(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.
Created attachment 194590 [details] Patch
Created attachment 194606 [details] Patch
Created attachment 194607 [details] Patch Fix inverted logic in ClipboardQt and ClipboardWin
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.
Created attachment 194618 [details] Patch
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.
Committed r146644: <http://trac.webkit.org/changeset/146644>
The tests added in this patch are crashing on Qt: http://build.webkit.org/results/Qt%20Linux%20Release/r146705%20(58724)/results.html