160200 - clipboard.autocopy does not work on Windows

Status: NEW

(Core :: DOM: Selection, defect)

Description

[bob]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1b) Gecko/20020721
BuildID:    2002072104

This is from http://www.mozilla.org/unix/customizing.html
"// Uncomment this to turn off Unix-style autocopy
// (or set it to true to turn it on on non-Unix platforms):
//user_pref("clipboard.autocopy", false);"


Reproducible: Always
Steps to Reproduce:
1. add user_pref("clipboard.autocopy", false); to user.js
2. exit mozilla (quick launch too )
3. ummmmm open mozilla
4. highlight text and paste

Actual Results:  highlighted text is not copied to clipboard  

Expected Results:  text is copied to clipboard 

i know it is one of the hidden prefs but i'd to see it working in Windowz ( or
maybe it is just XP; sorry no access to other Winz w/ Mozilla ).

Comment 1

[bob]

--> 1. add user_pref("clipboard.autocopy", false); to user.js <--
it should be TRUE not false ( i just c&p from top :)

Comment 2

[R.K.Aa.]

I found that pref too a while back, in all.js, and caught hope - but it doesn't
work on WinXP here either. COnfirming.

No idea if autocopy even is supposed to work on Windows. Default is "false". The
switch may be *nix specific. Would be great to have on Windows of course.

Over to selection for comment.

Comment 3

[Kim Scarborough]

It doesn't work under Windows 2000 either (1.1 final). Too bad, it's a nice idea.

Comment 4

[Vince Valenti]

I have confirmed this to be a bug on Mozilla 1.2.1 on Windows XP as well.

Comment 5

[Neil Fraser]

Doesn't work on Windows 98 either.  Mozilla 1.3b.  Used about:config to make the
change, confirmed that prefs.js was changed properly, but after restart Mozilla
didn't autocopy to the clipboard.

Shame, I'm in the process of a large copy/paste job which can't be scripted;
this option would have saved me considerable time.

Comment 6

[John]

Verified that it also does not work on NT4.0 SP6.0a using mozilla 1.3b

Comment 7

[phil]

I'm having related problems (at least I think they're related) using 1.3 under
Linux.

With clipboard.autocopy set to false, /all/ copy-to-clipboard operations fail,
even right-click-on-selected-text and choosing "Copy".  Nothing from the browser
ever makes it into the clipboard.

Leaving it true causes a really, really annoying fight between mozilla and any
tool which scans clipboard text for special actions (e.g., KDE's klipper).
Simply selecting the URL text in the urlbar -- even without actually
right-clicking and choosing "copy" -- will copy the URL into the clipboard.
Which then triggers a popup or some other message from the special-actions
tool.  I was really expecting that the autocopy would only effect text in the
normal browser window, not the urlbar.

Comment 8

[u64127]

This bug also exists in
Windows 2000 5.00.2195 Service Pack 3
Mozilla 1.4 RC1

I have the Optimoz mouse gestures plugin installed that may cause some conflict.

Comment 9

[phil]

Hmmm... I had the radial context / mouse gestures plugin from mozdev installed
at the time I wrote comment 7.  Since then I've had to reinstall mozilla for
unrelated reasons.  I've never gotten around to reinstalling that (very useful)
plugin, and I no longer experience the fights over the clipboard text.

Comment 10

[Dave Evans]

This is a bug in W2K SP4 in Thunderbird 0.1 Milestone as well

Comment 11

[Asa Dotzler [:asa]]

*** Bug 219966 has been marked as a duplicate of this bug. ***

Comment 12

[phil]

I wrote comment #7 and then comment #9.  Using 1.5, also with no weird plugins,
behavior remained good.  Under 1.6, the applications are fighting over the
clipboard again.

Setting this to false makes them not fight, but still kills all clipboard
copying, everywhere, even manual Edit -> Copy and right-click -> Copy.  :-(

Comment 13

[OstGote!]

Please change the summary to the fact that all Windows versions are affected.

"clipboard.autocopy does not work on Windows"

Comment 14

[David Landgren]

Does not work with XP SP1 and Mozilla 1.7b. If this was fixed I think my surfing
life would be complete.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 258860 has been marked as a duplicate of this bug. ***

Comment 16

[vw]

Everyone who works on unix as well as on windows will appreciate a fix.

Comment 17

[Steve Russell (RaiseMachine)]

Will there ever be a Windows fix for this? 
Or is it that this will never work on Win32 and is just for *nix? Just knowing either way would be helpful...

Reporter/someone with Summary change access needs to change this to Windows (All)

Comment 18

[Jesse Ruderman]

If this worked, I'd be able to test some "Linux-only" bugs on Windows.

Comment 19

[Bob Clary [:bc] (inactive)]

bryner, it doesn't seem obvious to me why this doesn't work on windows.

Comment 20

[had]

clipboard.autocopy also doesn't seem to work on Mac OS X. I really miss this feature, on unix machines I use it every day, and I always have to remember it doesn't work on my mac... :(

Comment 21

[Jeremy Baron]

(In reply to comment #20)
> clipboard.autocopy also doesn't seem to work on Mac OS X.
There is a seperate bug filed for this on OS X: Bug 392161.

Related bug 265868, will probably occur on Windows as well if this is fixed. (when this pref is enabled)

Comment 22

[l_paradox_l]

this is STILL not working in 3.6?

Comment 23

[skmail]

still not working on xp

Comment 24

[bburns]

Just another person hoping this feature will be enabled on Windows. 

Maybe they don't want people messing about with about:config, or adding another checkbox to the settings dialog. 

It's a pity though - this would be really useful to a lot of people who aren't aware such a thing exists. 
Especially considering the amount of copying and pasting people do from websites. Well, I do a lot. 

It could be another selling point against IE and Chrome.

Comment 25

[kernp25]

Attachment: enable-auto-copy-on-all-platforms.patch

This patch will enable auto copy on all platforms.

Comment 26

[kernp25]

can some1 review my patch?
please

Comment 27

[kernp25]

the problem is also that if u enable clipboard.autocopy pref on windows (and leave it on), then it will add useless selection listener (it creates memory leaks)

maybe if u dont want enable this feature in windows then hide this pref?

Comment 28

[kernp25]

the problem because why its not working is there is no selection clipboard in windows
there is only global clipboard

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8434121 [details] [diff] [review]
enable-auto-copy-on-all-platforms.patch

Review of attachment 8434121 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch! But I'm not a reviewer for this component. Maybe Masayuki can help here.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8434121 [details] [diff] [review]
enable-auto-copy-on-all-platforms.patch

This patch may destroy global clipbord data unexpectedly. I guess that some users who don't understand the pref well have already changed the pref true. Then, such users would file INVALID or WONTFIX bugs.

I'd like to suggest that we should add new bool pref like "clipboard.autocopy.allow_to_use_global_clipboard".

Then, this patch can be safer and simpler.

First, you should add two static members to nsAutoCopyListener. One is |bool sSelectionClipboardSupported|. It must not be changed dynamically if the platform changes to support selection clipboard. Therefore, the result must be able to be stored as a simple bool pref. You should cache the result of nsIClipboard::SupportsSelectionClipboard() at the constructor of nsAutoCopy. I.e., you need to move the constructor implementation from nsSelection.h to nsAutoCopyListener.h to nsSelection.cpp.

Next, the other is |bool sUseGlobalClipboard|. This should be used by Preferences::AddBoolVarCache() (http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/Preferences.h#270) and it should store the new pref's value. You should initialize this in the constructor too.

Then, nsAutoCopyListener::NotifySelectionChanged() would be like this:

> if (sUseGlobalClipboard) {
>   return nsCopySupport::HTMLCopy(... nsIClipboard::kGlobalClipboard);
> }
> 
> if (!sSelectionClipboardSupported) {
>   return NS_OK;
> }
> 
> return nsCopySupport::HTMLCopy(... nsIClipboard::kSelectionClipboard);

I.e., the new pref allows Linux and UNIX users to use same behavior as the other platforms.

I'd like to ask bz about this idea. How do you think, bz? (IIRC, bz commented around autocopy.)

Comment 31

[kernp25]

Or maybe make auto copy intelligent/smart?

Look at my addon:
https://addons.mozilla.org/de/firefox/addon/quick-copy-2/

This does only copy text if you hold down the mouse for 0.8 secs.
When its copied, the selection will be collapsed.
So, the user can easily see that the text is copied.

Comment 32

[kernp25]

but
I like your idea also :)

Comment 33

[kernp25]

Here is my code from Quick Copy 2 addon:

const selectionListener = {
  copyTimer: null,
  notifySelectionChanged: function(doc, sel, reason) {
  
    if (reason & Ci.nsISelectionListener.MOUSEDOWN_REASON) {
      if (sel.isCollapsed)
        return;
        
      if (!/\S+/.test(sel.toString()))
        return; 
  
      this._win = doc.defaultView;
      
      this.cancelCopyTimer();
      this.copyTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
      this.copyTimer.initWithCallback(this, 800,
                                      Ci.nsITimer.TYPE_ONE_SHOT);
    } 
    else if (reason == Ci.nsISelectionListener.MOUSEUP_REASON) {
      this.cancelCopyTimer();
    }
  },
  
  cancelCopyTimer: function() {
    if (this.copyTimer) {
      this.copyTimer.cancel();
      this.copyTimer = null;
    }
  },
  
  notify: function(aTimer) {
    if (aTimer == this.copyTimer) {
      this.copyTimer = null;
      let sel = this._win.getSelection();
      if (!sel) return;
      let toCopy = sel.toString().trim();
      if (toCopy) {
        let clipboardHelper = Cc["@mozilla.org/widget/clipboardhelper;1"].
                              getService(Ci.nsIClipboardHelper);
        clipboardHelper.copyString(toCopy, this._win.document);
        sel.collapse(sel.focusNode, sel.focusOffset);
      }  
    }
  }
}

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to kernp25 from comment #31)
> Or maybe make auto copy intelligent/smart?
> 
> Look at my addon:
> https://addons.mozilla.org/de/firefox/addon/quick-copy-2/
> 
> This does only copy text if you hold down the mouse for 0.8 secs.
> When its copied, the selection will be collapsed.
> So, the user can easily see that the text is copied.

Such intelligent behavior is a little bit hacky and may make creating automating tests difficult. I strongly recommend that we should not do that for this. If it were worthwhile to do that, such feature should be enabled in default settings. Probably, if you do that in Core, you will struggle with a lot of regression bugs like me :-(

And I forgot to refer our coding rules.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

Please do not forget to put {} to every block even if its line is only one.

Comment 35

[kernp25]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> Comment on attachment 8434121 [details] [diff] [review]
> enable-auto-copy-on-all-platforms.patch
> 
> This patch may destroy global clipbord data unexpectedly. I guess that some
> users who don't understand the pref well have already changed the pref true.
> Then, such users would file INVALID or WONTFIX bugs.

I think we can avoid this by adding a new setting to the setting window.
e.g. settings > advanced > General tab > Accessibility or maybe add it to Browsing?
https://support.mozilla.org/en-US/kb/advanced-settings-browsing-network-updates-encryption#w_general-tab

Because now this feature only can be changed if you going to about:config
So, if the user don't want this feature, he can easily turn this feature off in the firefox settings dialog.

I think also if the users dont want this feature,
then we add this clipboard.autocopy pref to the settings of firefox

Comment 36

[kernp25]

> if (sUseGlobalClipboard) {
>   return nsCopySupport::HTMLCopy(... nsIClipboard::kGlobalClipboard);
> }
> 
> if (!sSelectionClipboardSupported) {
>   return NS_OK;
> }
> 
> 

But what if sUseGlobalClipboard is false and sSelectionClipboardSupported is also false here
then the selection listener is useless here i think.
Maybe we check this before we adding the selection listener?

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to kernp25 from comment #35)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> > Comment on attachment 8434121 [details] [diff] [review]
> > enable-auto-copy-on-all-platforms.patch
> > 
> > This patch may destroy global clipbord data unexpectedly. I guess that some
> > users who don't understand the pref well have already changed the pref true.
> > Then, such users would file INVALID or WONTFIX bugs.
> 
> I think we can avoid this by adding a new setting to the setting window.
> e.g. settings > advanced > General tab > Accessibility or maybe add it to
> Browsing?
> https://support.mozilla.org/en-US/kb/advanced-settings-browsing-network-
> updates-encryption#w_general-tab
> 
> Because now this feature only can be changed if you going to about:config
> So, if the user don't want this feature, he can easily turn this feature off
> in the firefox settings dialog.


Like this feature should not be added to Preference dialog. It's enough to exist only in about:config. The default settings should honor the standard behavior in each platform as far as possible. Anyway, it should be a bug of Firefox, not Core.

(In reply to kernp25 from comment #36)
> > if (sUseGlobalClipboard) {
> >   return nsCopySupport::HTMLCopy(... nsIClipboard::kGlobalClipboard);
> > }
> > 
> > if (!sSelectionClipboardSupported) {
> >   return NS_OK;
> > }
> > 
> > 
> 
> But what if sUseGlobalClipboard is false and sSelectionClipboardSupported is
> also false here
> then the selection listener is useless here i think.
> Maybe we check this before we adding the selection listener?

Yes, it's ideal behavior. But if you want to change the behavior as ideal, we need to hack around here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#341
And also you need to remove the selection listener *when* the pref is disabled. Additionally, you need to add the selection listener *when* the pref is disabled. If you want to do it, I don't disagree. But I think that it's excessive implementation because these prefs must not be changed many times. Therefore, I believe that it's enough to check current settings only in selection listener. This only detects disabling the prefs.

Comment 38

[kernp25]

But i really think if you are not going to support other platforms, then we must really remove this 'useless' pref.(Why this pref was ever added?)
I found this: 
https://bugzilla.mozilla.org/show_bug.cgi?id=41127

> The "autocopy" feature is expected only on UNIX systems. But a pref will
> allow UNIX users to turn it off and Windows/Mac users to turn it on if they
> want it.
LOL!!!

This doesn't work in Windows/Mac anyway!!!

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#341

My suggestion would be instead of checking the pref, check if the system supports the selection clipboard and then add the selection listener.

Replace this:
>   // Check to see if the autocopy pref is enabled
>   //   and add the autocopy listener if it is
>   if (Preferences::GetBool("clipboard.autocopy")) {

with:
>   // get clipboard
>   nsCOMPtr<nsIClipboard> clipboard(do_GetService(kCClipboardCID, &rv));
>   NS_ENSURE_SUCCESS(rv, rv);
>
>   // check whether the system supports the selection clipboard or not.
>   bool selectionSupported;
>   rv = clipboard->SupportsSelectionClipboard(&selectionSupported);
>   NS_ENSURE_SUCCESS(rv, rv);
>
>   // if the system supports it then add our autocopy listener
>   if (selectionSupported) {

But i really think if you are not going to implement this feature for other platforms you must remove this pref.
Because this pref is useless (doesn't do anything in windows/mac).

And if a user wants this functionality, he can already install an addon for this.
There are already addons that adds this functionality.

https://addons.mozilla.org/en-US/firefox/search/?q=auto+copy&platform=windows&appver=28.0

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Wait, I don't understand why you want to remove the pref.

It should stay there for Linux/UNIX users turning it off.

And you're trying to implement this on all platforms, isn't it?

But the important issue of your patch is, if a user has already changed "clipboard.autocopy" to true and he/she forgot it, then, landing your current patch causes destroying clipboard data of such user when he/she just selects text. My suggestion is for preventing such critical dataloss. You are really trying to add a *new* feature and *new* feature should be enabled by new pref because any users don't accept the new feature explicitly *yet*.

Comment 40

[kernp25]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> Comment on attachment 8434121 [details] [diff] [review]
> enable-auto-copy-on-all-platforms.patch
> 
> This patch may destroy global clipbord data unexpectedly. I guess that some
> users who don't understand the pref well have already changed the pref true.
> Then, such users would file INVALID or WONTFIX bugs.
> 
> I'd like to suggest that we should add new bool pref like
> "clipboard.autocopy.allow_to_use_global_clipboard".
> 
> Then, this patch can be safer and simpler.

Isn't this useless?
I'm asking because if a windows users has set this clipboard.autocopy to true
but setting the clipboard.autocopy.allow_to_use_global_clipboard to false then the auto copy won't work
because in windows there is only global clipboard.

Maybe we only add this sSelectionClipboardSupported?

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to kernp25 from comment #40)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> > Comment on attachment 8434121 [details] [diff] [review]
> > enable-auto-copy-on-all-platforms.patch
> > 
> > This patch may destroy global clipbord data unexpectedly. I guess that some
> > users who don't understand the pref well have already changed the pref true.
> > Then, such users would file INVALID or WONTFIX bugs.
> > 
> > I'd like to suggest that we should add new bool pref like
> > "clipboard.autocopy.allow_to_use_global_clipboard".
> > 
> > Then, this patch can be safer and simpler.
> 
> Isn't this useless?
> I'm asking because if a windows users has set this clipboard.autocopy to true
> but setting the clipboard.autocopy.allow_to_use_global_clipboard to false
> then the auto copy won't work
> because in windows there is only global clipboard.

Yes, it is what is my intention.

> Maybe we only add this sSelectionClipboardSupported?

As I said in my previous comment, your new feature uses global clipboard. I.e., not emulating Linux/UNIX behavior perfectly. In other words, your new feature is similar to the auto copy of Linux/UNIX but actually different. Therefore, even if users have already set clipboard.autocopy true, it does *not* mean that they accept your new feature. Adding the new pref works as warning message by its name. I believe that this is important.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And perhaps, "clipboard.autocopy.allow_to_use_global_clipboard" should be "clipboard.autocopy.use_global_clipboard" because it changes the behavior on Linux/UNIX too.

Comment 43

[kernp25]

What is bz saying about this?

Comment 44

[kernp25]

Is this pref clipboard.autocopy.use_global_clipboard really needed?

If i going to about:config i will get a warning dialog if the general.warnOnAboutConfig pref is set to true.

So, if the user is clicking the "I'll be careful, I promise!" button then i think the user knows what he is doing.

Comment 45

[kernp25]

Or maybe we change the name of the clipboard.autocopy to maybe clipboard.autocopyonselection?

Comment 46

[kernp25]

So, the user can see what this pref is doing.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to kernp25 from comment #44)
> Is this pref clipboard.autocopy.use_global_clipboard really needed?

I believe so. See comment 41.

> If i going to about:config i will get a warning dialog if the
> general.warnOnAboutConfig pref is set to true.
> 
> So, if the user is clicking the "I'll be careful, I promise!" button then i
> think the user knows what he is doing.

It doesn't make sense because your change will come after changing the pref. Some users may keep the changed prefs due to no effect. However, your patch will destroy clipboard data. Some of them may hate the behavior and probably be surprised since suddenly the regression starts. I guess that most users of them doesn't suspect themselves because the behavior change comes from Gecko, not changing prefs by themselves.

(In reply to kernp25 from comment #45)
> Or maybe we change the name of the clipboard.autocopy to maybe
> clipboard.autocopyonselection?

I also don't like the pref name. However, if we change it, some Linux/UNIX users who are using it for turning off the auto copy is re-enabled. I.e., we need migration code for this. However, runtime migration wastes both running cost and binary size.

(In reply to kernp25 from comment #46)
> So, the user can see what this pref is doing.

I hope that MozillaZine will explain the pref meaning ;-)

Comment 48

[Boris Zbarsky [:bzbarsky]]

> How do you think, bz?

I agree that having the pref suddenly start doing something when it didn't use to is pretty weird.

If I were designing this from scratch, I'd have a pref with three values: disable, enable, system default.

Given the existence of profiles with this pref already set, I think the simplest way to not break people who have the pref set already (by either making it suddenly do stuff or renaming it) is to have two prefs.  The old one, if set false means "disable".  Otherwise it means check the new one.  The new one could be called clipboard.autocopy.force-enable or whatever and if true would enable, else not.  

That would not require runtime migration bits, at least.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, bz. It sounds good to me.

kernp25: Either of approach is okay. Could you attach a new patch and request review to me?

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8434121 [details] [diff] [review]
enable-auto-copy-on-all-platforms.patch

When you attach a new patch, please request review again.

Comment 51

[kernp25]

Attachment: bug-160200-fix.patch

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, looks like that it might conflict with the patches for bug 1261299. I think that you should create the patch after the patches are landed.