695032 - [GTK/X11] selecting text in scratchpad doesn't place it on the X primary selection

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

On the Linux/X11 desktop (definitely in GNOME apps, and I think in KDE as well), selecting text should make that text the X primary selection ("PRIMARY"), such that middle clicking in another app taking text input will paste that text.

(This is because X has a longstanding copy/paste model that's somewhat more like the mental model of dragging and dropping text than the Windows/Mac copy/paste model, except using select and middle-click.  The GNOME and KDE desktops have added a more Windows/Mac-like model with Copy and Paste operations in *addition* to that traditional model, but many X users use the traditional model.)

Steps to reproduce:
 1. Tools -> Web Developer -> Scratchpad
 2. select a word of the text in the window that opens
 3. open a terminal
 4. middle click in the terminal

Expected results:  The word that was selected gets pasted in the terminal (just like it does if you select a word of text in the browser or in view source or in some other app).

Actual results: either nothing is pasted or some other text is pasted

Comment 1

[Zack Weinberg (:zwol)]

There are a whole bunch of Gtk+ bugs that, collectively, add up to "I'm amazed you noticed this was broken, because it's already so broken as to be unusable."  Please see

https://bugzilla.gnome.org/show_bug.cgi?id=333514
https://bugzilla.gnome.org/show_bug.cgi?id=334060
https://bugzilla.gnome.org/show_bug.cgi?id=584236

... all of which have been hanging around not getting fixed since 2005-ish.  Of particular interest to fixing the bugs in *our* code, #333514 describes in detail how PRIMARY/middle-mouse is *supposed* to work.  I do not trust the linked fd.o clipboard spec to be correct, however; there were any number of people arguing quite strenuously for incorrect behaviors. :-(

Comment 2

[Mihai Sucan [:msucan]]

Zack: thanks for your comment and links, that was very helpful (I read 333514). I have only made a complete switch to Linux about 5 years ago and the Select/middle click behavior is something I am not used to. (I came from Windows.)

The place where we can fix this bug and 695035 is interesting to pick:

1) We can fix it in the SourceEditor component that wraps Orion. Selection events can be used to copy the text to PRIMARY. We can also hook into the middle click event and generate a paste into Orion.

2) We can look into Orion itself why this doesn't work. Orion makes a native selection, that should automatically be copied to PRIMARY (perhaps there's something prevent this from happening?). The middle click event is canceled by Orion. We can make it so the event is not canceled - to allow the paste to happen.

Comment 3

[Mihai Sucan [:msucan]]

Bug reported upstream:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=361471

Comment 4

[Zack Weinberg (:zwol)]

These days, middle-mouse paste and PRIMARY are kinda a hidden easter egg.  Users accustomed to Windows should be able to ignore them completely.  But for people like me, who have been using Unix on the desktop since the nineties, they're very convenient and it drives us nuts when they don't work precisely the way Xt made them work.  Which was largely accidental but happened to be quite ergonomic :)

Comment 5

[Mihai Sucan [:msucan]]

Results from upstream: this bug cannot be fixed within Orion.

We need to do this from the Source Editor integration code. I have a plan how this can be fixed.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch with included test.

Pushed this patch and the dependencies to the try server:

https://tbpl.mozilla.org/?tree=Try&rev=f9802b4377cf

Comment 7

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 580452 [details] [diff] [review]
proposed patch

 
   /**
+   *
+   * Orion Selection event handler for Linux users. This allows one to select
+   * text and have it copied into the X11 PRIMARY

looks like an extra * line in there. (nit)

     Services.prefs.getBoolPref(SourceEditor.PREFS.EXPAND_TAB);
+
+  this._onOrionSelection = this._onOrionSelection.bind(this);

I don't see a corresponding this._onOrionSelection = null or delete segment in destroy. Maybe it's not in this patch.

This looks good though.

Comment 8

[Zack Weinberg (:zwol)]

> +   * Orion Selection event handler for Linux users. This allows one to

Replace "Linux" with "X11" or "X Window System" throughout; the behavior we're trying to preserve here originates with MIT Athena, and has no historical connection to Linux.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] updated patch

Updated the patch. Thanks Rob for the review! Good catch!

I also made some minor changes to make sure the test passes under more diverse conditions.

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 580965 [details] [diff] [review]
[in-fx-team] updated patch

https://hg.mozilla.org/integration/fx-team/rev/7cf966da2e93

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 580965 [details] [diff] [review]
[in-fx-team] updated patch

(copy/paste fail in the previous comment)

correct link:
https://hg.mozilla.org/integration/fx-team/rev/5bd81a413dea

Comment 12

[Zack Weinberg (:zwol)]

Comment on attachment 580965 [details] [diff] [review]
[in-fx-team] updated patch

>+    if (Services.appinfo.OS == "Linux") {
>+      this._view.addEventListener("Selection", this._onOrionSelection);
>+    }

I shoulda actually read the patch before; *this is wrong*.  You need to detect X11, not Linux.  We have a fair number of users on systems that use X11 as their window system but are not Linux.

I'm afraid I don't know how to do this correctly.

>+    if (Services.appinfo.OS == "Linux") {
>+      this._view.removeEventListener("Selection", this._onOrionSelection);
>+    }

Similarly.

I'm r-ing to mark this as a serious concern, but you're welcome to fix as a follow-up patch.

Comment 13

[Mihai Sucan [:msucan]]

(In reply to Zack Weinberg (:zwol) from comment #12)
> Comment on attachment 580965 [details] [diff] [review]
> [in-fx-team] updated patch
> 
> >+    if (Services.appinfo.OS == "Linux") {
> >+      this._view.addEventListener("Selection", this._onOrionSelection);
> >+    }
> 
> I shoulda actually read the patch before; *this is wrong*.  You need to
> detect X11, not Linux.  We have a fair number of users on systems that use
> X11 as their window system but are not Linux.
> 
> I'm afraid I don't know how to do this correctly.
> 
> >+    if (Services.appinfo.OS == "Linux") {
> >+      this._view.removeEventListener("Selection", this._onOrionSelection);
> >+    }
> 
> Similarly.
> 
> I'm r-ing to mark this as a serious concern, but you're welcome to fix as a
> follow-up patch.

Sorry about this. Who can I ping on how I can detect X11 on the system?

Or should I not special case this? Can we use the selection primary on all systems?

I suggest we fix this in a follow up bug/patch.

Comment 14

[Zack Weinberg (:zwol)]

(In reply to Mihai Sucan [:msucan] from comment #13)
> 
> Sorry about this. Who can I ping on how I can detect X11 on the system?

The only person I can think of who isn't already cc:ed but might know is karlt.

> Or should I not special case this? Can we use the selection primary on all
> systems?

Dunno.

> I suggest we fix this in a follow up bug/patch.

Follow-up patch, yes, but I think this bug should not be closed until this is corrected.

Comment 15

[Zack Weinberg (:zwol)]

Doing a bit of my own homework: unless karlt has a better idea, I suggest the condition

   Services.appinfo.widgetToolkit.slice(0,3) === "gtk"

The slice() is to be future-proof against the addition of a gtk3 back end -- see bug 627699.  (We can worry about Gtk/Wayland later.)  As far as I am aware, the qt widget backend is not for use with KDE, so we don't need that in there.

See http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULRuntime.idl#88

Comment 16

[Karl Tomlinson (:karlt)]

Can you access CC[NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX + "http"].getService(CI.nsIHttpProtocolHandler)?

Its "platform" property is "X11".

Comment 17

[Rob Campbell [:rc] (:robcee)]

please file a bug to address Zack's concerns as a follow-up. This is in.

https://hg.mozilla.org/mozilla-central/rev/5bd81a413dea

Comment 18

[Rob Campbell [:rc] (:robcee)]

(In reply to Zack Weinberg (:zwol) from comment #14)
> (In reply to Mihai Sucan [:msucan] from comment #13)
> > 
> > Sorry about this. Who can I ping on how I can detect X11 on the system?
> 
> The only person I can think of who isn't already cc:ed but might know is
> karlt.
> 
> > Or should I not special case this? Can we use the selection primary on all
> > systems?
> 
> Dunno.
> 
> > I suggest we fix this in a follow up bug/patch.
> 
> Follow-up patch, yes, but I think this bug should not be closed until this
> is corrected.

I think you're being a tad overzealous about this. Linux is a supported platform. "Other variants of Unix" is really a fringe thing. While I want to support them too, getting this fix in for the majority of our *nix users is acceptable for a developer tool.