RESOLVED FIXED 38374
REGRESSION(r54368): Text drag-and-drop from input/textarea doesn't work if the text is like a URL
https://bugs.webkit.org/show_bug.cgi?id=38374
Summary REGRESSION(r54368): Text drag-and-drop from input/textarea doesn't work if th...
Kent Tamura
Reported 2010-04-29 22:12:56 PDT
http://crbug.com/42125 r54368 changed clipboard structure for text drag-and-drop from input/textarea. Before it, clipboard had text/html and text/plain. After it, clipboard has only text/plain. Chromium pasteboard code automatically adds a URL created from the text/plain datum. WebCore/page/DragController.cpp documentFragmentFromDragData() tries to insert the URL because of no text/html. It fails in a case of dropping another input/textarea because the URL datum has no title.
Attachments
Proposed patch (2.55 KB, patch)
2010-04-29 22:57 PDT, Kent Tamura
no flags
Proposed patch (rev.2) (11.98 KB, patch)
2010-05-01 06:19 PDT, Kent Tamura
no flags
Proposed patch (rev.3) (2.45 KB, patch)
2010-05-01 08:33 PDT, Kent Tamura
no flags
Proposed patch (rev.4) (5.04 KB, patch)
2010-05-01 10:19 PDT, Kent Tamura
levin: review+
levin: commit-queue-
Kent Tamura
Comment 1 2010-04-29 22:57:16 PDT
Created attachment 54782 [details] Proposed patch
Eric Seidel (no email)
Comment 2 2010-04-30 11:45:50 PDT
Comment on attachment 54782 [details] Proposed patch I'm having difficulty understanding if this is the right layer to hack. I think I would have written a small inline to handle this: + buffer.append('&'); + buffer.append('a'); + buffer.append('m'); + buffer.append('p'); + buffer.append(';'); or maybe Vector already will add multiple values from an T*?
Kent Tamura
Comment 3 2010-05-01 06:19:54 PDT
Created attachment 54851 [details] Proposed patch (rev.2)
Kent Tamura
Comment 4 2010-05-01 06:24:24 PDT
(In reply to comment #2) > (From update of attachment 54782 [details]) > I'm having difficulty understanding if this is the right layer to hack. Three are some other points we can add this hack: - setting data to pasteboard in Chromium code - populating pasteboard data in Chromium code - Retrieving data from DataObject in WebCore code I chose here because I'd like to make the behavior same as behavior prior to r54368. > I think I would have written a small inline to handle this: > + buffer.append('&'); > + buffer.append('a'); > + buffer.append('m'); > + buffer.append('p'); > + buffer.append(';'); > > or maybe Vector already will add multiple values from an T*? I improved the code.
Eric Seidel (no email)
Comment 5 2010-05-01 08:19:03 PDT
I think you may have included some extra code in your patch?
Kent Tamura
Comment 6 2010-05-01 08:28:08 PDT
(In reply to comment #5) > I think you may have included some extra code in your patch? Oops, what happened...
Kent Tamura
Comment 7 2010-05-01 08:33:22 PDT
Created attachment 54852 [details] Proposed patch (rev.3)
Eric Seidel (no email)
Comment 8 2010-05-01 08:47:49 PDT
Comment on attachment 54852 [details] Proposed patch (rev.3) That's nice. Do we know how Safari Mac works, and why this is not an issue for them?
Kent Tamura
Comment 9 2010-05-01 08:55:59 PDT
(In reply to comment #8) > (From update of attachment 54852 [details]) > That's nice. Do we know how Safari Mac works, and why this is not an issue for > them? Yes. I wrote it in the comment: // Chromium pasteboard code automatically adds URL for a text/plain datum // in order to add capability of dragging text to the address bar. I think Safari/Mac doesn't have such feature.
Eric Seidel (no email)
Comment 10 2010-05-01 09:10:33 PDT
Text from what source? From Chromium itself? Does the chromium code add a url for non-url data? It appears that Safari will only add the URL type for things which look like URLs. If you type http://apple.com in a random text field and then try to drag it to the Safari bar, it works. However "foo bar baz" does not. It seems this should be fixed with smarts on both the writer and reader side. The writer should only write the URL type when things look like URLs. The reader should be able to support plain text (not just urls) and should just do a Google search for the contents of the plain text (maybe only supporting it if the plain text is under some character limit). Then we'd correctly create the URL type only for things that look like URLs (thus we'd integrate with the rest of the OS better) and we'd support searching for stuff in Google (which safari already supports by the fact that the Google search bar accepts any text type as a drop).
Daniel Cheng
Comment 11 2010-05-01 09:19:09 PDT
> The writer should only write the URL type when things look like URLs. Known issue in NSPasteboard+Utils.mm. See https://crbug.com/42125 for a more detailed explanation. > The reader should be able to support plain text (not just urls) and should just do a Google search for the contents of the plain text (maybe only supporting it if the plain text is under some character limit). The reader does support plain text. Unfortunately, when dragging and dropping, the order documentFragmentFromDragData checks for content on the dragging pboard is HTML, then URL, then plain text. Since URL is always populated on Mac thanks to the bug mentioned in crbug.com/42125, it always tries to drop the URL--but since there's no title, nothing appears. We could fix the NSPasteboard+Utils.mm code, but the more general case of dragging and dropping plain-text URLs between text inputs would still be broken.
Eric Seidel (no email)
Comment 12 2010-05-01 09:24:25 PDT
I'm not sure I"m familiar enough with how plain text drags work between text fields in WebKit to comment. I'll have to look at documentFragmentFromDragData and see why it's involved. I'm still not sure how Safari works and why we'd need to/want to differ from their behavior, or need to add this hack. The hack itself looks fine, I'm just not yet convinced we want it?
Kent Tamura
Comment 13 2010-05-01 09:34:33 PDT
Hmm, I found a problem on Safari Nightly too. 1. type "http://www.google.com/" in an <input element> 2. select it 3. drag it and drop to another <input> or <textarea> Expected: 4. "http://www.google.com/" moves from the source <input> to the destination field Actual: 5. "http://www.google.com/" disappears and the destination fields is unchanged. This should be a regression on r54368.
Eric Seidel (no email)
Comment 14 2010-05-01 09:36:45 PDT
Comment on attachment 54852 [details] Proposed patch (rev.3) Sounds like the proper fix would fix both Chromium and Safari then instead of this chromium-only fix. :)
Eric Seidel (no email)
Comment 15 2010-05-01 09:39:08 PDT
Removing [chromium] since this is not chromium specific anymore. :)
Daniel Cheng
Comment 16 2010-05-01 09:39:41 PDT
> I'm still not sure how Safari works and why we'd > need to/want to differ from their behavior, or need to add this hack. Safari never populates URLs in the dragging pboard when dragging plain-text. I'm not entirely sure why this logic was added in Chrome. My personal theory is that it was to allow dragged and dropped plain-text URLs to trigger navigation, but I'm not sure. Edit: I didn't realize it affected Safari nightlies too, so I guess my first sentence is inaccurate.
Kent Tamura
Comment 17 2010-05-01 10:19:59 PDT
Created attachment 54854 [details] Proposed patch (rev.4)
Kent Tamura
Comment 18 2010-05-01 10:25:33 PDT
FYI. Mac's text -> URL code is _web_bestURL in WebKit/mac/Misc/WebNSPasteboardExtras.mm.
Enrica Casucci
Comment 19 2010-05-04 13:41:57 PDT
It looks good to me and I like that is not a platform specific. I'd like to see instructions in the test to run it manually.
David Levin
Comment 20 2010-05-04 14:01:55 PDT
Comment on attachment 54854 [details] Proposed patch (rev.4) This looked fine to me and mostly importantly looked fine to Enrica as well. Thanks Enrica and good point regarding the test! cq- due to the issue that Enrica mentioned which should be fixed before landing.
David Levin
Comment 21 2010-05-04 15:05:24 PDT
WebKit Review Bot
Comment 22 2010-05-04 15:34:03 PDT
http://trac.webkit.org/changeset/58780 might have broken GTK Linux 32-bit Release and Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/58779 http://trac.webkit.org/changeset/58780
David Levin
Comment 23 2010-05-04 15:53:30 PDT
(In reply to comment #22) > http://trac.webkit.org/changeset/58780 might have broken GTK Linux 32-bit > Release and Qt Linux Release > The following changes are on the blame list: > http://trac.webkit.org/changeset/58779 > http://trac.webkit.org/changeset/58780 gtk/qt don't seem to implement drag/drop support so I just added the test to their respective skipped lists.
Note You need to log in before you can comment on or make changes to this bug.