Bug 17267 – [GTK] Primary selection/clipboard support

• Component: WebKitGTK
• Status: RESOLVED DUPLICATE

[Alp Toker]

We already have clipboard support, but it doesn't work with the primary clipboard on X.

We need to support

  (a) selections for pasting from WebKit into other applications
  (b) middle-click for pasting from the primary clipboard into editable WebKit areas

On platforms without a primary clipboard, GTK+ automatically makes this a no-op, so we don't need to add any conditional/X-specific code to support this.

[Christian Dywan]

Created attachment 21872 [details]
Attempt at primary clipboard support

I do not know too well about WebCore's clipboard support, but at some point I really needed primary clipboard support.

So here is my attempt. It works for all I want.

[Oliver Hunt]

Comment on attachment 21872 [details]
Attempt at primary clipboard support

r-, the shouldBlah methods are callbacks made to the api to query whether something is allowed, not that is has already happened.

EditorClient methods are (by and large) just present to allow WebCore's behaviour to be controlled through the platforms API, so all these methods should just map through to your API implementation.

The WebKit/GTK API's default implementation for didChangeSelectedRange (or whatever, definitely not shouldChangeSelectedRange) would probably be something along these lines.

[Alp Toker]

The performance hit/memory fragmentation of converting HTML to text every time a large selection is changed would be too great, and would rule out the possibility of content negotiation (copy and paste as HTML, text, rich text, Pango markup etc.)

I think we need to avoid copying the inner text of the selection every time it changes, instead responding to a primary selection request when it happens (Qt does the former and I don't like it.)

[Alp Toker]

Created attachment 23321 [details]
Primary clipboard copy support

The behaviour of this patch is intended to match GtkTextView (which is slightly different to the selection support in Firefox and OpenOffice and fits in better with GTK+ applications).

[Alp Toker]

Comment on attachment 23321 [details]
Primary clipboard copy support

Landed in r36323. Still need paste support to close this bug.

[Sergio García-Cuevas González]

Created attachment 24698 [details]
Copy link locations to the primary selection

This patch makes WebCore::Pasteboard::writeURL use the primary selection as well as the clipboard selection.

[Alp Toker]

(In reply to comment #6)
> Created an attachment (id=24698) [edit]
> Copy link locations to the primary selection
> 
> This patch makes WebCore::Pasteboard::writeURL use the primary selection as
> well as the clipboard selection.
> 

Thanks, this is a reasonable feature to add. I'll review this one in a bit. There are a few interactions that need to be checked before getting this in.

[Jan Alonzo]

Comment on attachment 24698 [details]
Copy link locations to the primary selection

Alp to review..

[Alexander Butenko]

'Primary clipboard copy support' is already landed. i applied 'Copy link locations to the primary selection' patch but still cant paste nothing to editable areas in webkit.

[Sergio García-Cuevas González]

Created attachment 27129 [details]
Preliminary primary selection paste support

Here is my attempt to support middle-click pasting.  The target editable area must be selected, so it only works when the selection comes from an external application or when a clipboard manager is running (and then the selection comes from an external application, too).

[Sergio García-Cuevas González]

Comment on attachment 27129 [details]
Preliminary primary selection paste support

>Index: WebKit/gtk/ChangeLog
>===================================================================
>--- WebKit/gtk/ChangeLog	(revision 40327)
>+++ WebKit/gtk/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>[phone]  Sergio García-Cuevas <[e-mail]>
>+
>+        https://bugs.webkit.org/show_bug.cgi?id=17267
>+
>+        Implement primary selection support (pasting into editable regions).
>+
>+        * WebCoreSupport/EditorClientGtk.cpp:
>+        (WebKit::EditorClient::buttonPressed):
>+        (WebKit::EditorClient::EditorClient):
>+
> 2009-01-27  Brady Eidson  <[e-mail]>
> 
>         Reviewed by Dan Bernstein
>Index: WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp
>===================================================================
>--- WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp	(revision 40327)
>+++ WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp	(working copy)
>@@ -37,6 +37,23 @@ using namespace WebCore;
> 
> namespace WebKit {
> 
>+static void buttonPressed(GtkWidget* widget, GdkEventButton* event, EditorClient* client)
>+{
>+    if (event->button == 2 && event->type == GDK_BUTTON_PRESS) {
>+        Frame* frame = core(client->m_webView)->focusController()->focusedOrMainFrame();
>+        Editor* editor = frame->editor();
>+        if (editor->canPaste()) {
>+            GtkClipboard* clipboard = gtk_widget_get_clipboard(GTK_WIDGET(client->m_webView), GDK_SELECTION_PRIMARY);
>+            gchar* utf8 = gtk_clipboard_wait_for_text(clipboard);
>+            if (utf8) {
>+                String text = String::fromUTF8(utf8);
>+                g_free(utf8);
>+                editor->insertText(text, 0);
>+            }
>+        }
>+    }
>+}
>+
> static void imContextCommitted(GtkIMContext* context, const gchar* str, EditorClient* client)
> {
>     Frame* targetFrame = core(client->m_webView)->focusController()->focusedOrMainFrame();
>@@ -470,6 +490,7 @@ EditorClient::EditorClient(WebKitWebView
>     WebKitWebViewPrivate* priv = m_webView->priv;
>     g_signal_connect(priv->imContext, "commit", G_CALLBACK(imContextCommitted), this);
>     g_signal_connect(priv->imContext, "preedit-changed", G_CALLBACK(imContextPreeditChanged), this);
>+    g_signal_connect_after(m_webView, "button-press-event", G_CALLBACK(buttonPressed), this);
> }
> 
> EditorClient::~EditorClient()

[Sergio García-Cuevas González]

(In reply to comment #11)

I misplaced the braces in my latest patch.  Then I tried to correct it in an obviously wrong way.  I think I should have some rest.

[Gustavo Noronha (kov)]

Comment on attachment 24698 [details]
Copy link locations to the primary selection

> +        WARNING: NO TEST CASES ADDED OR CHANGED

This warning should be removed. There is no behavior change for webcore; we could probably add GTK+ unit tests for this, though, in WebKit/gtk.

> +GtkClipboard* PasteboardHelperGtk::getPrimary(Frame* frame) const {
> +    WebKitWebView* webView = webkit_web_frame_get_web_view(kit(frame));
> +    return gtk_widget_get_clipboard(GTK_WIDGET (webView),
> +                                    GDK_SELECTION_PRIMARY);
> +}
> +

I believe the only problem with this patch is the incorrect placement of braces here. Whoever lands, please fix this, and rs=me on a second commit to fix the other braces in this file.

I wonder if we should review the PasteBoard files placements as a whole, sometime.

[Jan Alonzo]

Comment on attachment 24698 [details]
Copy link locations to the primary selection

Landed in r42855. Style fixes landed in r42866.

Clearing review flag.

[Gustavo Noronha (kov)]

Comment on attachment 27129 [details]
Preliminary primary selection paste support

> +static void buttonPressed(GtkWidget* widget, GdkEventButton* event, EditorClient* client)
> +{
> +    if (event->button == 2 && event->type == GDK_BUTTON_PRESS)
> +    {
> +        Frame* frame = core(client->m_webView)->focusController()->focusedOrMainFrame();
> +        Editor* editor = frame->editor();
> +        if (editor->canPaste())
> +        {

The patch looks right, but the if braces are bad. They should look like this:

if (a) {
    blahblah();
}

Also, we should return early, where possible, so I would change that if to:

if (!editor->canPaste())
    return;

I thought about suggesting making only 1 early-return if, but I believe we may want to use this callback for other features in the future, so it makes sense this way. r=me with those changes.

[Gustavo Noronha (kov)]

Comment on attachment 27129 [details]
Preliminary primary selection paste support

Actually, this does not seem to work. The entries do not generate a button-press-event for the view.

[Gustavo Noronha (kov)]

*** This bug has been marked as a duplicate of 25685 ***