REGRESSION(r54368): Text drag-and-drop from input/textarea doesn't work if the text is like a URL

• Component: HTML Editing
• Status: RESOLVED FIXED

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 54782 [details]
Proposed patch

[Eric Seidel (no email)]

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]

Created attachment 54851 [details]
Proposed patch (rev.2)

[Kent Tamura]

(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)]

I think you may have included some extra code in your patch?

[Kent Tamura]

(In reply to comment #5)
> I think you may have included some extra code in your patch?

Oops, what happened...

[Kent Tamura]

Created attachment 54852 [details]
Proposed patch (rev.3)

[Eric Seidel (no email)]

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]

(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)]

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]

> 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)]

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]

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 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)]

Removing [chromium] since this is not chromium specific anymore. :)

[Daniel Cheng]

> 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]

Created attachment 54854 [details]
Proposed patch (rev.4)

[Kent Tamura]

FYI.
Mac's text -> URL code is _web_bestURL in WebKit/mac/Misc/WebNSPasteboardExtras.mm.

[Enrica Casucci]

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 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]

Committed as http://trac.webkit.org/changeset/58780

[WebKit Review Bot]

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]

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