310380 - middlemouse autoscroll default-on for UNIX/Linux (X) environment

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Hogarth]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050717 SeaMonkey/1.0a
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050717 SeaMonkey/1.0a

In the land of UNIX/Linux the middle mouse button is for pasting by default and
as such this feature is interfering and there's no easy workaround to set it
'right'.

Reproducible: Always

Steps to Reproduce:
1. Select text
2. Click to paste
3. Get autoscroll activated

Actual Results:  
Autoscroll was activated.

Expected Results:  
clipboard buffer should've been pasted

Comment 1

[Hogarth]

Attachment: Adds pref to allow toggling of autoscroll, with defaults based on OS.

I have patch thanks to CThos help. :)

It creates a pref called middlemouse.autoscroll and it sets it to false in UNIX
land and true otherwise. The all.js file is a tad messy in this respect so that
might ned careful eyeballing.

First patch to moz so please be gentle with the chainsaw. ;)

Comment 2

[[e-mail]]

Could you explain why you don't find the existing middlemouse prefs suffice? By
default on Linux autoscroll is disabled because contentLoadURL takes priority;
even if you disable that then paste and scroll prefs apply where appropriate.

Comment 3

[Hogarth]

(In reply to comment #2)
> Could you explain why you don't find the existing middlemouse prefs suffice? By
> default on Linux autoscroll is disabled because contentLoadURL takes priority;
> even if you disable that then paste and scroll prefs apply where appropriate.

My middlemouse prefs are as follows:

middlemouse.contentLoadURL: false
middlemouse.openNewWindow: true
middlemouse.paste: true
middlemouse.scrollbarPosition: true

and I still get the autoscroller. Even if I were to middle-click outside of a
paste field I do not want the autoscroller. This is why I've disabled
contentLoadURL. It was the height of annoyance and frustration to have the
browser suddenly go elsewhere (or in the latest case, suddenly lock the
functionality of the mouse and do 'weird' things to the display) because I
either missed the paste field or a link or accidentally clicked the middle mouse
button.

Right now, I have no way out what-so-ever. :/

Comment 4

[Bruno 'Aqualon' Escherl]

I don't want autoscroll and I don't want to enable contentLoadURL under Windows,
since it's even more distracting. So the only workaround at the moment is to
default the return value of isAutoscrollBlocker(node) in contentAreaClick.js to
true at the beginning of the function.

Since Firefox even has an UI button for that, it would be the least to allow
changing that behaviour over a preference.

Comment 5

[Frank Wein [:mcsmurf]]

I wonder if could not name this pref general.autoScroll like in Firefox? Why
create a newly named pref when there is already one which has the same purpose
and already appears in docs on the Internet.

Comment 6

[Hogarth]

(In reply to comment #5)
> I wonder if could not name this pref general.autoScroll like in Firefox? Why
> create a newly named pref when there is already one which has the same purpose
> and already appears in docs on the Internet.

I don't use Firefox so I did not know what existed there. If the reviewers would
prefer that name then I'll remake the patch with general.autoScroll rather then
middlemouse.autoscroll as the pref name. (although the latter makes more sense
to me then the former :)

Comment 7

[[e-mail]]

Comment on attachment 197780 [details] [diff] [review]
Adds pref to allow toggling of autoscroll, with defaults based on OS.

>+pref("middlemouse.autoscroll", true);
As mentioned this needs to be general.autoScroll and it also belongs in
browser-prefs.js instead.

>+  var prefService = Components.classes["@mozilla.org/preferences-service;1"];
>+  prefService = prefService.getService(Components.interfaces.nsIPrefService);
>+  var prefBranch = prefService.getBranch(null);
It looks like you copied an "inferior" version - the one in goPreferences is
superior.

>+  if (!prefBranch.getBoolPref("middlemouse.autoscroll"))
>+    return;
>+
>   if (gScrollingView || event.button != 1)
>     return;
Couldn't you combine these in one big condition?

Comment 8

[Hogarth]

(In reply to comment #7)
> (From update of attachment 197780 [details] [diff] [review] [edit])
> >+pref("middlemouse.autoscroll", true);
> As mentioned this needs to be general.autoScroll and it also belongs in
> browser-prefs.js instead.

No problem. Will do.

> >+  var prefService = Components.classes["@mozilla.org/preferences-service;1"];
> >+  prefService = prefService.getService(Components.interfaces.nsIPrefService);
> >+  var prefBranch = prefService.getBranch(null);
> It looks like you copied an "inferior" version - the one in goPreferences is

:) The inferior version does seem to be used in a lot of places. :)

> superior.

So, something like this?

var pref =
Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);

if (!pref.getBoolPref("general.autoScroll"))
    return;

Looking closer it seems both the old and new method appear to be used
interchangeably.

> >+  if (!prefBranch.getBoolPref("middlemouse.autoscroll"))
> >+    return;
> >+
> >   if (gScrollingView || event.button != 1)
> >     return;
> Couldn't you combine these in one big condition?

I thought of that as I was writing it but decided to do the above because:

a. It looks a lot cleaner
b. Conceptually they are checking for different things
c. Other people elsewhere in the same file appeared to have a similar idea so
   it appeared to be the way the style flowed.

I'm not fussed though so if the merging of the two ifs is required then I will
do so. Getting this in is more important to me (esp since I wasted all day
yesterday thinking there was a bug in some php code but all along it was a bug
in my old version of seamonkey (opera and lynx both work fine)).

I'll work up a patch once the ifedness gets cleared up (ie merge or no).

Comment 9

[Marko Macek]

I seem to have a bigger problem. This functionality is very annoying sometimes
(I keep pasting random pieces of text, for some reason).

Now in firefox 1.5 beta 2 the setting to disable this doesn't work.

Even enabling autoscroll which is a more useful feature for me doesn't help.

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #9)
> I seem to have a bigger problem. This functionality is very annoying sometimes
> (I keep pasting random pieces of text, for some reason).
> Now in firefox 1.5 beta 2 the setting to disable this doesn't work.
> Even enabling autoscroll which is a more useful feature for me doesn't help.

Firefox's autoscroll is entirely different from SeaMonkey's.  Please file a bug 
under the Firefox product.

Comment 11

[[e-mail]]

(In reply to comment #8)
>So, something like this?
> 
>var pref =
Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
Yes, but that long line needs to be split just before the second "." to line up
on the next line with the first "." above it.

>I'll work up a patch once the ifedness gets cleared up (ie merge or no).
OK, then don't bother merging the ifs.

Comment 12

[Hogarth]

Attachment: previous patch with requested modifications

This is the same patch as before but with the following (requested) changes:

* middlemouse.autoscroll changed to general.autoScroll
* used different method of grabbing pref
* set pref in browser-prefs.js instead

Comment 13

[[e-mail]]

Comment on attachment 199720 [details] [diff] [review]
previous patch with requested modifications

>+  var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                       .getService(Components.interfaces.nsIPrefBranch);
>+
>+  if (!pref.getBoolPref("general.autoScroll"))
>+    return;
>+
>   if (gScrollingView || event.button != 1)
>     return;
Although now that I look at it again I think I'd prefer if these two conditions
were exchanged.

Comment 14

[Hogarth]

(In reply to comment #13)
> (From update of attachment 199720 [details] [diff] [review] [edit])
> >+  var pref = Components.classes["@mozilla.org/preferences-service;1"]
> >+                       .getService(Components.interfaces.nsIPrefBranch);
> >+
> >+  if (!pref.getBoolPref("general.autoScroll"))
> >+    return;
> >+
> >   if (gScrollingView || event.button != 1)
> >     return;
> Although now that I look at it again I think I'd prefer if these two conditions
> were exchanged.

*nod* Looking at them, the latter might come up more often and be the faster
check. If you don't want to swap the two ifs around yourself I can generate a
new patch tonight.

Comment 15

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 199720 [details] [diff] [review]
previous patch with requested modifications

No need for a new patch version to flip the if order.  Just have whoever checks
it in make that change.

r=cst

Please also request approval1.8rc1 so we can get this for SeaMonkey 1.0.

Comment 16

[Hogarth]

(In reply to comment #15)
> (From update of attachment 199720 [details] [diff] [review] [edit])
> No need for a new patch version to flip the if order.  Just have whoever checks
> it in make that change.

Cool.

> Please also request approval1.8rc1 so we can get this for SeaMonkey 1.0.

Done. Thanks. :)

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Checked in on trunk.

Comment 18

[Elmar Ludwig]

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