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.
Created attachment 54782 [details] Proposed patch
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*?
Created attachment 54851 [details] Proposed patch (rev.2)
(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.
I think you may have included some extra code in your patch?
(In reply to comment #5) > I think you may have included some extra code in your patch? Oops, what happened...
Created attachment 54852 [details] Proposed patch (rev.3)
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?
(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.
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).
> 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.
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?
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.
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. :)
Removing [chromium] since this is not chromium specific anymore. :)
> 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.
Created attachment 54854 [details] Proposed patch (rev.4)
FYI. Mac's text -> URL code is _web_bestURL in WebKit/mac/Misc/WebNSPasteboardExtras.mm.
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.
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.
Committed as http://trac.webkit.org/changeset/58780
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
(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.