The default bug view has changed. See this FAQ.

Add autoscroll support

RESOLVED FIXED in seamonkey1.0alpha

Status

SeaMonkey
UI Design
--
enhancement
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com], Assigned: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com])

Tracking

(Blocks: 1 bug, {fixed1.8, relnote})

Trunk
seamonkey1.0alpha
x86
All
fixed1.8, relnote
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

162 bytes, image/gif
Details
9.91 KB, patch
db48x
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
13.48 KB, patch
db48x
: review+
neil@parkwaycc.co.uk
: superreview+
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.
Created attachment 192696 [details] [diff] [review]
patch

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 3

12 years ago
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)
Created attachment 192795 [details]
autoscroll icon
Created attachment 192798 [details] [diff] [review]
patch

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 6

12 years ago
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.
Attachment #192798 - Flags: review?(db48x)
Created attachment 192877 [details] [diff] [review]
patch
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 10

12 years ago
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-
Attachment #192877 - Flags: review?(db48x)
Created attachment 193320 [details] [diff] [review]
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.
Attachment #192877 - Attachment is obsolete: true
Attachment #193320 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #193320 - Flags: review?(db48x)

Comment 12

12 years ago
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

12 years ago
(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-

Comment 15

12 years ago
Created attachment 194386 [details]
better icon (checked in)
Attachment #192795 - Attachment is obsolete: true
Attachment #193320 - Flags: superreview?(neil.parkwaycc.co.uk)
Created attachment 194451 [details] [diff] [review]
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.
Attachment #193320 - Attachment is obsolete: true
Attachment #194451 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #194451 - Flags: review?(db48x)

Comment 17

12 years ago
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+
Created attachment 194844 [details] [diff] [review]
what I checked in
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?

Updated

12 years ago
Attachment #194451 - Flags: approval1.8b5? → approval1.8b5+
Created attachment 195212 [details] [diff] [review]
followup patch

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)
Created attachment 196392 [details] [diff] [review]
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.
Attachment #196392 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196392 - Flags: review?(db48x)

Comment 26

12 years ago
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)

Comment 27

12 years ago
(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 29

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 33

12 years ago
> 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. ***

Comment 35

12 years ago
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.

Updated

12 years ago
Attachment #196392 - Flags: approval1.8b5? → approval1.8b5+
Blocks: 309537
Blocks: 309538
Keywords: relnote
Keywords: fixed1.8

Updated

10 years ago
Blocks: 242621

Updated

10 years ago
Blocks: 384575

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.