Closed Bug 304563 Opened 19 years ago Closed 19 years ago

Add autoscroll support

Categories

(SeaMonkey :: UI Design, enhancement)

x86
All
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey1.0alpha

People

(Reporter: csthomas, Assigned: csthomas)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.8, relnote)

Attachments

(3 files, 7 obsolete files)

162 bytes, image/gif
Details
9.91 KB, patch
db48x
: review+
Details | Diff | Splinter Review
13.48 KB, patch
db48x
: review+
Details | Diff | Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
1. It's opaque.  That's not changing until bug 70798 (maybe 1.9?).
2. It's rectangular.  See point #1.
Attachment #192696 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #192696 - Flags: review?(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 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.
Attachment #192696 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attachment #192696 - Attachment is obsolete: true
Attachment #192696 - Flags: review?(db48x)
Attached patch patch (obsolete) — Splinter Review
Not sure why Linux wouldn't work.  I need more info.
Attachment #192798 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #192798 - Flags: review?(db48x)
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
Attachment #192798 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #192798 - Attachment is obsolete: true
Attachment #192877 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #192877 - Flags: review?(db48x)
bah. that patch doesn't apply either.
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.
Attachment #192877 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attached patch patch (obsolete) — Splinter Review
(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.
Attachment #192877 - Attachment is obsolete: true
Attachment #193320 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #193320 - Flags: review?(db48x)
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.
(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 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 :).
Attachment #193320 - Flags: review?(db48x) → review-
Attachment #192795 - Attachment is obsolete: true
Attachment #193320 - Flags: superreview?(neil.parkwaycc.co.uk)
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.
Attachment #193320 - Attachment is obsolete: true
Attachment #194451 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #194451 - Flags: review?(db48x)
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.
Attachment #194451 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Status: NEW → ASSIGNED
Target Milestone: --- → Seamonkey1.0alpha
Comment on attachment 194451 [details] [diff] [review]
patch (checked in)

r=db48x
Attachment #194451 - Flags: review?(db48x) → review+
Attachment #194844 - Attachment is obsolete: true
Attachment #194386 - Attachment description: better icon → better icon (checked in)
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
QA Contact: stephen.donner
QA Contact: stephen.donner → nobody
Comment on attachment 194451 [details] [diff] [review]
patch (checked in)

SeaMonkey-only patch
Attachment #194451 - Flags: approval1.8b5?
Attachment #194451 - Flags: approval1.8b5? → approval1.8b5+
Attached patch followup patch (obsolete) — Splinter Review
Add autoscroll to mail, view source, move autoscroll code to a better file,
make minor improvements to autoscroll implementation.
Attachment #195212 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #195212 - Flags: review?(db48x)
Whiteboard: [cst: r?sr?]
It looks like this caused bug 308503 on Windows at least.
Depends on: 308503
Comment on attachment 195212 [details] [diff] [review]
followup patch

Obsoleting due to regressions not addressed here.
Attachment #195212 - Attachment is obsolete: true
Whiteboard: [cst: r?sr?] → [cst: fix regression]
Whiteboard: [cst: fix regression]
Attachment #194451 - Attachment description: patch → patch (checked in)
Attached patch autoscroll v2Splinter Review
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.
Attachment #196392 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196392 - Flags: review?(db48x)
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?
Attachment #195212 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #195212 - Flags: review?(db48x)
(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?
+    if (event.originalTarget.ownerDocument.designMode == "on")
+      return true;

That should be "node." instead of "event.originalTarget.".
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.
Attachment #196392 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Comment on attachment 196392 [details] [diff] [review]
autoscroll v2

I don't see anything else. r=db48x
Attachment #196392 - Flags: review?(db48x) → review+
Comment on attachment 196392 [details] [diff] [review]
autoscroll v2

SeaMonkey-only patch, includes a regression fix.
Attachment #196392 - Flags: approval1.8b5?
I'd be interested in followup bugs for icons that indicate direction, and
round/transparent icons (blocked by the transparent popup bug).
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
> 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.
*** Bug 22775 has been marked as a duplicate of this bug. ***
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.
Attachment #196392 - Flags: approval1.8b5? → approval1.8b5+
Blocks: 242621
Blocks: 384575
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.