Autoscroll indicator should show possible scrolling directions

RESOLVED FIXED

Status

SeaMonkey
UI Design
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})

Trunk
x86
Windows XP
fixed1.8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

The indicator that appears when you start autoscrolling should show what
scrolling directions are possible.  In a normal page, for example, there should
only be north-south arrows.  A wide but short page should result in east-west
arrows.  Attachment 184934 [details] [diff] on bug 296080 has some code that could be useful. 
Given how we set the icon, though, we'd have to set an attribute on the
autoscroller which gets matched by the CSS.
Status: NEW → ASSIGNED
Created attachment 196948 [details] [diff] [review]
patch
Attachment #196948 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196948 - Flags: review?(db48x)
Created attachment 196949 [details]
north-south icon

This needs to be optimized.  I just cut some arrows out of the existing icon,
and the file got much bigger.
Created attachment 196950 [details]
east-west icon

Same deal here.

Comment 4

12 years ago
Created attachment 196987 [details]
north-south icon (optimized gif)

Comment 5

12 years ago
Created attachment 196988 [details]
east-west icon (optimized gif)

Comment 6

12 years ago
256 colours is overkill, but I can actually optimize them down to 4 colours ;-)

Comment 7

12 years ago
Comment on attachment 196948 [details] [diff] [review]
patch

>+  skin/classic/communicator/icons/autoscrollNS.gif                        (communicator/icons/autoscrollNS.gif)
>+  skin/classic/communicator/icons/autoscrollEW.gif                        (communicator/icons/autoscrollEW.gif)
Should be autoscroll-ns or -ew. (Or possibly ns- or ew-autoscroll, compare
ns-resize cursor).

>+#autoscroller[scrolldir="NSEW"] {
>   background-image: url("chrome://communicator/skin/icons/autoscroll.gif");
> }
>+#autoscroller[scrolldir="NS"] {
>+  background-image: url("chrome://communicator/skin/icons/autoscrollNS.gif");
>+}
>+#autoscroller[scrolldir="EW"] {
>+  background-image: url("chrome://communicator/skin/icons/autoscrollEW.gif");
>+}
Please choose one rule to be default i.e. not using an attribute selector.

>+  var scrollingDir = "";
>+  if (gScrollingView.scrollMaxY > 0) {
>+    scrollingDir += "NS";
>+  }
>+  if (gScrollingView.scrollMaxX > 0) {
>+    scrollingDir += "EW";
>+  }
>+  if (!scrollingDir)
>+    scrollingDir = "NSEW";
You should probably do nothing (abort the scroll) if the frame isn't
scrollable. If you make ns the default then maybe you could write it like this:
if (gScrollingView.scrollMaxX > 0)
  gAutoScrollPopup.setAttribute("scrolldir", gScrollingView.scrollMaxY > 0 ?
"nsew" : "ew")
else if (gScrollingView.scrollMaxY > 0)
  gAutoScrollPopup.removeAttribute("scrolldir"); // default
else
  return; // don't scroll
Attachment #196948 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-

Updated

12 years ago
Component: General → XP Apps: GUI Features

Updated

12 years ago
Blocks: 280331
Whiteboard: [cst:r?[
Attachment #196948 - Attachment is obsolete: true
Attachment #196948 - Flags: review?(db48x)
Attachment #196987 - Attachment filename: autoscrollNS2.gif → autoscroll-ns.gif
Attachment #196988 - Attachment filename: autoscrollEW2.gif → autoscroll-ew.gif
Created attachment 199628 [details] [diff] [review]
patch v2

I left the "NSEW" attribute value for multi-directional scrolling.
Attachment #199628 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #199628 - Flags: review?(db48x)

Comment 9

12 years ago
Comment on attachment 199628 [details] [diff] [review]
patch v2

Remind me to get you those fully optimized images (137/148 bytes).

> #autoscroller { 
>   height: 28px;
>   width: 28px;
>   border: 0px;
>   margin: -14px;
>   padding: 0px;
>+}
>+#autoscroller {
Unnecessary addition (applies to both themes).

>+    gScrollingView = null; // make sure we don't think we're scrolling
Wouldn't "abort scrolling" suffice?
Attachment #199628 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+

Updated

12 years ago
Attachment #199628 - Flags: review?(db48x) → review+
Comment on attachment 199628 [details] [diff] [review]
patch v2

SeaMonkey-only patch
Attachment #199628 - Flags: approval1.8rc1?
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [cst:r?[ → [cst: a?] [cst: fixed-trunk]
Created attachment 200312 [details] [diff] [review]
what I checked in on trunk

Updated

12 years ago
Attachment #199628 - Flags: approval1.8rc1? → approval1.8rc1+
Keywords: fixed1.8
Whiteboard: [cst: a?] [cst: fixed-trunk]

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.