304563 - Add autoscroll support

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

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

I filed this because bug 22775 has too many comments.  I have a patch which
doesn't require modifying the page DOM.  To the user, it acts basically like the
"autoscroll" extension (which, I think, is what Firefox currently uses).

Don't comment in this bug about:
* Drivers (Intellimouse, Logitech, etc)
* How it should work (or how any other implementations work)
* How it should look
* Differences between autoscroll, panning, grabber scrolling, etc
* Firefox
* Your two cents

Don't attach screenshots.

Comment 1

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

Attachment: patch

1. It's opaque.  That's not changing until bug 70798 (maybe 1.9?).
2. It's rectangular.  See point #1.

Comment 2

[Daniel Brooks [:db48x]]

Comment on attachment 192696 [details] [diff] [review]
patch

>Index: mozilla/xpfe/browser/resources/content/navigator.xul
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.xul,v
>retrieving revision 1.431
>diff -u -p -9 -r1.431 navigator.xul
>--- mozilla/xpfe/browser/resources/content/navigator.xul	14 Jul 2005 04:48:56 -0000	1.431
>+++ mozilla/xpfe/browser/resources/content/navigator.xul	14 Aug 2005 23:19:20 -0000
>@@ -133,18 +133,22 @@
>   <popup id="sidebarPopup"/>
> 
>   <popupset id="bookmarksPopupset"/>
>   <tooltip  id="btTooltip"/>
>   <template id="bookmarksMenuTemplate"/>
>  
>   <!-- context menu -->
>   <popupset id="contentAreaContextSet"/>
> 
>+  <popup id="autoscroller" style="height: 30px; width:30px; margin: 0px; border:0px;" onmousedown="handleMouseUpDown" onpopuphidden="stopScrolling()">
>+    <image src="http://ctho.ath.cx/tmp/scroll.png"/>
>+  </popup>

This could be bad for the health of your webserver.

>+  function startScroll(event)
>+  {
>+    if (isScrolling || !document.getElementById("autoscroller"))
>+      return;
>+    document.popupNode = null;
>+    document.getElementById("autoscroller").showPopup(document.getElementById("autoscroller").parentNode, event.screenX-15, event.screenY-15, "popup", "topleft", "at_pointer");

Hrm, I don't particularly care for that. For one thing, since you're specifying
coordinates, the final two arguments are ignored. However, isn't there a value
you can put in the align parameter that will center the popup over the cursor?
If not, perhaps you could find out the actual width and height of the popup and
offset by half of that instead of hardcoding 15 pixels?

>+
>+  function handleMouseUpDown(event)
>+  {
>+    stopScrolling();
>+    document.getElementById("autoscroller").hidePopup();
>+  }
>+
>+  function stopScrolling()
>+  {
>+    if (isScrolling) {
>+      isScrolling = false;
>+      sX = 0; sY = 0; startX = 0; startY = 0; targdocument = null;
>+      window.removeEventListener('mousemove', window.handleMouseMove, false);
>+      window.removeEventListener('mousedown', window.handleMouseUpDown, false);
>+      window.removeEventListener('mouseup', window.handleMouseUpDown, false);
>+      // Put this back in if you want to crash Moz
>+      //    document.getElementById("autoscroller").hidePopup();

You have the same function call in handleMouseUpDown, which calls this
function. Perhaps it crashes if you call hidePopup() on a popup that isn't
visible (which means the second call would crash)

I'll also have to try it before I r+ it, of course.

Comment 3

[[e-mail]]

Comment on attachment 192696 [details] [diff] [review]
patch

>+  <popup id="autoscroller" style="height: 30px; width:30px; margin: 0px; border:0px;" onmousedown="handleMouseUpDown" onpopuphidden="stopScrolling()">
>+    <image src="http://ctho.ath.cx/tmp/scroll.png"/>
>+  </popup>
You forgot to remove the popup's padding. Which means changing the dimensions
to 28px if we're keeping that image... and the offset to 14px, which is best
done using margin: -14px; rather than in the JS.

I'm still wondering how best to fix the styling.
a) Have <popup id="autoscroller"><image/></popup> and put the styles in
chrome://communicator/skin/communicator.css that undo all the default popup
styles
b) Have <popup id="autoscroller"/> and put an XBL binding in
chrome://communicator/content/communicator.css for
chrome://communicator/content/autoscroll/autoscroll.xml which defines the
<image/> as content and a stylesheet of
chrome://communicator/skin/autoscroll/autoscroll.css
c) As b) but use <autoscroller/> instead
d) as b) or c) but put the styles in content instead
Note: b) .. d) requires duplicating the code in popup.xml

>-                    onclick="return contentAreaClick(event);"/>
>+                    onclick="return contentAreaClick(event);"
>+                    onmousedown="return contentAreaMouseDown(event);"/>
If you swap these over it makes that patch smaller :-P

>+  var isScrolling = false;
Strictly speaking, unnecessary as you can track the state using one of the
other variables.

>+  var startX;
>+  var startY;
>+  var sX;
>+  var sY;
currentX/Y?

>+  var targdocument;
Not a document any more. scrollingView perhaps?

>+  var scrollTimer;
I'm leaning towards autoScrollTimer.

>+  function contentAreaMouseDown(event)
>+  {
>+    if (!hrefForClickEvent(event) && event.button == 1 
>+        && (!pref || !pref.getBoolPref("middlemouse.contentLoadURL"))
>+        && (event.target != event.currentTarget)) {
>+      startScroll(event);
startScrolling perhaps?

>+      event.stopPropagation(); // make sure handleMouseUpDown doesn't kill the scroll
Unfortunately on Linux the mouse up still kills the scroll... maybe Windows
treats mouse up events differently when a popup is showing.

>+    window.addEventListener('mousemove', window.handleMouseMove, false);
>+    window.addEventListener('mouseup', window.handleMouseUpDown, false);
>+    window.addEventListener('mousedown', window.handleMouseUpDown, false);
The window. prefixes are unnecessary.

>+  function autoScrollLoop()
>+  {
>+    if (!isScrolling) {
>+      clearInterval(scrollTimer);
>+      return;
>+    }
I don't like this here. This should be in stopScrolling.

>+    var speed = 3;
const (or pref?!)?

>+    targdocument.scrollBy(x/speed, y/speed);
Spaces around /

>+  function handleMouseUpDown(event)
>+  {
>+    stopScrolling();
>+    document.getElementById("autoscroller").hidePopup();
>+  }
Hiding the popup should stop the scrolling, no need to do it twice.

>+      sX = 0; sY = 0; startX = 0; startY = 0; targdocument = null;
Don't need to reset the integers.

Comment 4

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

Attachment: autoscroll icon

Comment 5

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

Attachment: patch

Not sure why Linux wouldn't work.  I need more info.

Comment 6

[[e-mail]]

Comment on attachment 192798 [details] [diff] [review]
patch

> /* ::::: print preview toolbar ::::: */
> 
> toolbar[printpreview="true"] {
>   -moz-binding: url("chrome://communicator/content/printPreviewBindings.xml#printpreviewtoolbar");
> }
>+
>+/* :::::: autoscroll popup ::::: */
>+
>+#autoscroller { 
>+  height: 28px;
>+  width: 30px;
>+  border: 0px;
>+  margin: -15px;
>+  padding: 0px;
>+}
>+
>+#autoscroller-icon { 
>+  list-style-image:url("chrome://communicator/skin/icons/autoscroll.png");
>+}
These styles belong in the skin. Only if you're defining a new widget using XBL
should you put styles in content.
The image is actually 28x28 so the width should be 28 and the margin -14. Also
was it quite necessary to use a 24-bit png when a monochrome gif will do?
list-style-image inherits from the parent, so you can specify it on the popup
and the image will inherit it.

>+    if (!hrefForClickEvent(event) && event.button == 1 
Your tree isn't up-to-date.

>+    if (scrollingView || !document.getElementById("autoscroller"))
>+      return;
>+    document.popupNode = null;
>+    var scroller = document.getElementById("autoscroller");
Move the variable up so that you can reuse it.

>+    addEventListener('mousemove', window.handleMouseMove, false);
>+    addEventListener('mouseup', window.handleMouseUpDown, false);
>+    addEventListener('mousedown', window.handleMouseUpDown, false);
Still some window.s

>+    var speed = 4;
It's still var :-P

>+    stopScrolling();
>+    document.getElementById("autoscroller").hidePopup();
I don't think you need to call stopScrolling if you're hiding the popup.

>+      isScrolling = false;
No longer used :-P

Comment 7

[Daniel Brooks [:db48x]]

Just tested the patch, it works pretty well once I resolved the conflict (always
update before you diff.) The only gripe I have is that when the page moves it
leaves behind a trail because the spot that was covered up by the popup doesn't
get redrawn in sync with the rest of the page. I've seen that in other
situations though, so I guess it's a layout bug.

I don't think the speed needs to be a pref, just pick a resonable value. Four
seems to be a good choice.

As for autoscroll.png, it's only 450 bytes as is. A little bit of tweaking with
pngcrush got it down to 277, which is smaller than almost every other image in
that directory.

Comment 8

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

Attachment: patch

Comment 9

[Daniel Brooks [:db48x]]

bah. that patch doesn't apply either.

Comment 10

[[e-mail]]

Comment on attachment 192877 [details] [diff] [review]
patch

I started investigating the issue with the popup not working reliably under
Linux. It appears that the Linux behaviour, in that you get a mouse up but no
mouse click, is correct, but as yet I don't know why Windows is giving me
different behaviour. Fortunately it appears that IE's autoscroll ignores mouse
events over the popup widget itself.

>+  <popup id="autoscroller" onmousedown="handleMouseUpDown" onpopuphidden="stopScrolling()">
The onmousedown does nothing, because handleMouseUpDown doesn't have any ()s.
It doesn't matter, because the window gets the event anyway.

>+    <image id="autoscroller-icon"/>
You still haven't got rid of this unnecessary id. The image will inherit the
list-style-image style from the popup.

>+    var ceParams = {event: event, href: "", linkNode: null};
>+    hrefAndLinkNodeForClickEvent(ceParams);
Bitrotted...

>+    if (!ceParams.href && event.button == 1 
>+        && (!pref || !pref.getBoolPref("middlemouse.contentLoadURL"))
>+        && (event.target != event.currentTarget)) {
Please put these in order of simplicity i.e. start with event.button == 1

>+    addEventListener('mousemove', handleMouseMove, false);
>+    addEventListener('mouseup', handleMouseUpDown, false);
>+    addEventListener('mousedown', handleMouseUpDown, false);
>+      removeEventListener('mousemove', handleMouseMove, false);
>+      removeEventListener('mousedown', handleMouseUpDown, false);
>+      removeEventListener('mouseup', handleMouseUpDown, false);
I think using true might be more useful, as that would stop web pages from
blocking an ongoing scroll.

Comment 11

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

Attachment: patch

(In reply to comment #10)
> (From update of attachment 192877 [details] [diff] [review] [edit])
> I started investigating the issue with the popup not working reliably under
> Linux. It appears that the Linux behaviour, in that you get a mouse up but no

> mouse click, is correct, but as yet I don't know why Windows is giving me
> different behaviour. Fortunately it appears that IE's autoscroll ignores
mouse
> events over the popup widget itself.
So it doesn't work properly on Linux?  I can't debug that.

> 
> >+  <popup id="autoscroller" onmousedown="handleMouseUpDown"
onpopuphidden="stopScrolling()">
> The onmousedown does nothing, because handleMouseUpDown doesn't have any ()s.

> It doesn't matter, because the window gets the event anyway.
Fixed.

> >+	<image id="autoscroller-icon"/>
> You still haven't got rid of this unnecessary id. The image will inherit the
> list-style-image style from the popup.
Fixed (and improved).

> >+	var ceParams = {event: event, href: "", linkNode: null};
> >+	hrefAndLinkNodeForClickEvent(ceParams);
> Bitrotted...
How so?  I cvs upped anyway for the new diff and didn't have to change
anything... based on IanN's patch, I think he unbitrotted me with the
regression fix.

> >+	if (!ceParams.href && event.button == 1 
> >+	    && (!pref || !pref.getBoolPref("middlemouse.contentLoadURL"))
> >+	    && (event.target != event.currentTarget)) {
> Please put these in order of simplicity i.e. start with event.button == 1
Fixed.

> >+	addEventListener('mousemove', handleMouseMove, false);
> >	     <snip>
> I think using true might be more useful, as that would stop web pages from
> blocking an ongoing scroll.
Fixed.

Comment 12

[[e-mail]]

You're actually relying on a Windows widget bug.

When you mouse down, the widget code captures the mouse. This makes the mouse up
message go to the original window even though you've since opened a popup window
under the mouse. This confuses the ESM into generating a follow-up click.

Comment 13

[Ian Neal]

(In reply to comment #11)
> Created an attachment (id=193320) [edit]
> patch

> > >+	var ceParams = {event: event, href: "", linkNode: null};
> > >+	hrefAndLinkNodeForClickEvent(ceParams);
> > Bitrotted...
> How so?  I cvs upped anyway for the new diff and didn't have to change
> anything... based on IanN's patch, I think he unbitrotted me with the
> regression fix.
No I still bitrotted you.
hrefAndLinkNodeForClickEvent takes event as its argument and returns null if
href is null otherwise returns an array of href and linkNode.

Comment 14

[Daniel Brooks [:db48x]]

Comment on attachment 193320 [details] [diff] [review]
patch

yea, it doesn't work well at all on linux. We discussed on irc a way to do it
that should work, but I'm interested in the bug you mention - it would be
pretty funny if that bug makes the method I proposed not work on windows :).

Comment 15

[[e-mail]]

Attachment: better icon (checked in)

Comment 16

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

Attachment: patch (checked in)

Works on gtk2.	Didn't test gtk1 or Windows (I don't have access to those
platforms right now)... based on what Neil has told me, I would expect some
minor behavioral difference on gtk1.  Windows should work fine.

Comment 17

[[e-mail]]

Comment on attachment 194451 [details] [diff] [review]
patch (checked in)

>+    if (event.button == 1 && !hrefAndLinkNodeForClickEvent(event)
>+        && (!pref || !pref.getBoolPref("middlemouse.contentLoadURL"))
>+        && (event.target != event.currentTarget)) {
I think the target test should be moved from fourth to second.

>+      event.stopPropagation(); // make sure handleMouseUpDown doesn't kill the scroll
The comment or possibly the whole line is wrong.

>+    if (Math.abs(x) > speed || Math.abs(y) > speed)
>= here please.

>+  skin/classic/communicator/icons/autoscroll.png                        (communicator/icons/autoscroll.png)
>+  skin/modern/communicator/icons/autoscroll.png                    (communicator/icons/autoscroll.png)
s/.png/.gif/g

sr=me with the nits fixed.

Comment 18

[Daniel Brooks [:db48x]]

Comment on attachment 194451 [details] [diff] [review]
patch (checked in)

r=db48x

Comment 19

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

Attachment: what I checked in

Comment 20

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

http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1125848315&maxdate=1125848760&who=cst%25andrew.cmu.edu
What I actually checked in.  (I made the .diff after committing... so it was
empty.  oops.)

Remaining issues: mailnews, view source, domi, anything else?

Some followup features might be:
 * different icon for vertical/horizontal-only scroll
 * round/transparent popup
 * modifying the cursor and keeping it as you mouse around the page

Comment 21

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

Comment on attachment 194451 [details] [diff] [review]
patch (checked in)

SeaMonkey-only patch

Comment 22

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

Attachment: followup patch

Add autoscroll to mail, view source, move autoscroll code to a better file,
make minor improvements to autoscroll implementation.

Comment 23

[Boris Zbarsky [:bzbarsky]]

It looks like this caused bug 308503 on Windows at least.

Comment 24

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

Comment on attachment 195212 [details] [diff] [review]
followup patch

Obsoleting due to regressions not addressed here.

Comment 25

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

Attachment: autoscroll v2

This patch adds autoscroll to a bunch of other places, fixes the regression
described in bug 308503, moves the autoscroll code to a better place, and makes
a few improvements to the autoscroll code itself.

Comment 26

[:deanis74]

Comment on attachment 196392 [details] [diff] [review]
autoscroll v2

>+function startScrolling(event)
...
>+  if (event.originalTarget instanceof XULElement &&
>+      ((event.originalTarget.localName == "thumb")
>+        || (event.originalTarget.localName == "slider")
>+        || (event.originalTarget.localName == "scrollbarbutton")))
>+    return;

Why isn't this check also in isAutoScrollBlocker?

Comment 27

[[e-mail]]

(In reply to comment #26)
>(From update of attachment 196392 [details] [diff] [review])
>>+function startScrolling(event)
>...
>>+  if (event.originalTarget instanceof XULElement &&
>>+      ((event.originalTarget.localName == "thumb")
>>+        || (event.originalTarget.localName == "slider")
>>+        || (event.originalTarget.localName == "scrollbarbutton")))
>>+    return;
>Why isn't this check also in isAutoScrollBlocker?
I guess because View Source needs this check too?

Comment 28

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

+    if (event.originalTarget.ownerDocument.designMode == "on")
+      return true;

That should be "node." instead of "event.originalTarget.".

Comment 29

[[e-mail]]

Comment on attachment 196392 [details] [diff] [review]
autoscroll v2

>  * - [ Dependencies ] ---------------------------------------------------------
>  *  utilityOverlay.js:
>  *    - gatherTextUnder
>  */
Could add startScrolling here ;-)
>+        && !isAutoscrollBlocker(event.originalTarget)
>         && (!pref || !pref.getBoolPref("middlemouse.contentLoadURL"))) {
You added that pref check to isAutoscrollBlocker, so you can remove it here.

>+      if ((node instanceof HTMLInputElement
>+                       && (node.type == "text" || node.type == "password"))
>+                      || node instanceof HTMLTextAreaElement)
Write the text area check first, it will make the code more readable, also put
||s and &&s at the ends of lines, not the start.

sr=me with things fixed.

Comment 30

[Daniel Brooks [:db48x]]

Comment on attachment 196392 [details] [diff] [review]
autoscroll v2

I don't see anything else. r=db48x

Comment 31

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

Comment on attachment 196392 [details] [diff] [review]
autoscroll v2

SeaMonkey-only patch, includes a regression fix.

Comment 32

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

I'd be interested in followup bugs for icons that indicate direction, and
round/transparent icons (blocked by the transparent popup bug).

Comment 33

[Gérard Talbot]

> I'd be interested in followup bugs for icons that indicate direction, and
> round/transparent icons (blocked by the transparent popup bug).

I can do the 4 directions-cursors. Join bug 275174.

Comment 34

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

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

Comment 35

[Gérard Talbot]

I created the 8 direction-cursors for autoscroll. You can see/view/test how they
look at the bottom of this page:

http://www.gtalbot.org/DHTMLSection/Cursors.html

in the last 2 red-bordered tables. Hotspots, outlines, shapes, etc.. can all be
redone, redrawn if needed. Let me know where (in which bugfile) you want me to
upload these as attachments.