Closed Bug 174049 (smoothscroll) Opened 17 years ago Closed 17 years ago

implement smooth scrolling

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla1.3alpha

People

(Reporter: neil, Assigned: roc)

References

Details

(Whiteboard: [fix])

Attachments

(3 files, 9 obsolete files)

3.87 KB, patch
Details | Diff | Splinter Review
221 bytes, text/html
Details
27.22 KB, patch
kinmoz
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
Phoenix should animate the document body when the user scrolls it.
this patch works when the user presses the up/down pg up/pg down keys but not
with the scroll bar.  I looked at the xul code and it seems that
nsIScrollMediator does not track the event, so I'm not sure how to
differentiate between clicking on the arrow or the non-thumb area and dragging
the thumb.  smooth scrolling should not happen when the user drags the thumb.  


I'm sure it's pretty simple to change the xul to send the appropriate
aUpdateFlags so that smooth scrolling happens at the right time, but the xul
code makes my head hurt, and I don't use the scroll bar so it's not a big deal
to me.
I'm happy to #ifdef MOZ_PHOENIX this out if that's useful to anyone.
Status: NEW → ASSIGNED
*** Bug 33966 has been marked as a duplicate of this bug. ***
Keywords: patch, review
cc: roc, I here you are the module owner on this.
i'm confused.

should this not in fact be a duplicate of bug 33966, since all check-ins to the
mozilla branch contribute to phoenix?
We have a decent patch here, so this is the bug we should be tracking.
Some comments on the patch:

Views aren't ref counted. Don't make the AddRef/Release public. You don't need
to change the nsISupports-related code at all.

+  PRUint32 NumOfElapsedScrollFrames;
+  nscoord CurrentCoordinateIncrement;

These member variables should start with "m".

You have some places where you're just reformatting code. Don't. Especially
where you're just introducing unnecessary parentheses.

+  if (destinationY < mOffsetY && destinationY != mOffsetY)
+    mDirection = SCROLL_DIRECTION_UP;
+  if (destinationX > mOffsetX && destinationX != mOffsetX)
+    mDirection = SCROLL_DIRECTION_RIGHT;

It's possible for both of these to be true; are you sure it's OK to just forget
about SCROLL_DIRECTION_UP? Doesn't look good to me. I think you should remember
them both.

+  nsIDeviceContext  *dev;

I know you're just copying the old code, but if you're going to mess with it,
change this to an nsCOMPtr, thanks.

+ nsScrollPortView::IsSmoothScrollingEnabled() 

You should really cache the pref somewhere. This gets called a lot.

+    case SCROLL_DIRECTION_LEFT:
+      if (mOffsetX < mDestinationX)

Shouldn't these be <=?

I don't understand why you need to mess with mLineHeight.

There doesn't seem to be any code that prevents disaster if the view is
destroyed while you're still doing the incremental scroll. It looks like the
timer will fire and try to scroll a non-existent view. Or does dropping the only
reference to the nsITimer automatically cancel it?

+     destY = mOffsetY - CurrentCoordinateIncrement;;

+     destY = mOffsetY + CurrentCoordinateIncrement;;

Useless extra ;.

How fast is this on a complex page? My impression of Windows is that it doesn't
paint the uncovered area during a scroll animation until the animation has
finished. But this does. Can we paint the uncovered area fast enough? What
happens on a slow machine?

Sometimes our scrolling code just has to repaint the whole window whenever we
scroll, e.g., if the window has a "background-attachment: fixed". Would it make
sense to cancel the incremental scrolling in this case? The code in
nsScrollPortView::Scroll detects this. Be careful because whether we have to
repaint the whole window or not can depend on where you are in the document, so
you might find during incremental scrolling that you suddenly go from
bitblitting to full repainting.

If nsScrollPortView::EndScrollingIfDestinationIsReached() cancels the timer
because it hit MAX_FRAMES, how do you make sure that the window is actually
scrolled the rest of the way to the destination?

I'd be slightly happier if all the smooth-scrolling member variables were in an
object of their own pointed to by nsScrollPortView, so the space overhead is
only there while smooth scrolling is happening.

If we can get this patch together then I would be in favour of checking it in
before 1.2final, since it should be pretty clear than nothing changes while the
pref is off.
roc, thank you for your comments.  I've addressed most of the items you
mentioned.  these are the outstanding items:

"Views aren't ref counted. Don't make the AddRef/Release public. You don't need

to change the nsISupports-related code at all."

doesn't the nsITimer need access to these?  I agree that it sucks to make these
public, but I could not get the timer to work without it.  can you point me to
some code that uses a static timer maybe?

"I know you're just copying the old code, but if you're going to mess with it,
change this to an nsCOMPtr, thanks."

I know it's not clear from the diff, but I didn't copy code, I renamed the
entire ScrollTo() and privatized it.  in doing that, I moved the body down to
match the order in the .h file.  if you want a prettier diff with these methods
out of order, let me know.  outside of a prettier diff, I don't think I'm the
one to clean up the internals of this old method.

"I don't understand why you need to mess with mLineHeight."

increasing the mLineHeight adds to the usability of the smooth scrolling.  I
removed it, because it's problematic setting it when the pref changes, but I
think it should be re-added before this becomes final.

"How fast is this on a complex page?"  

it runs fine on my pII 200 scrolling around slashdot's front page (vertical and
horizontal).  I'd appreciate more feedback from others though (especially on
windows and mac)

"I'd be slightly happier if all the smooth-scrolling member variables were in
an
object of their own"

agreed.  If it's really important I can factor out the smooth scrolling once
the code is more complete, as it's just simpler for me to work with one class.

There is still the outstanding issue of the xul scroll bars not sending the
correct aUpdateFlags for ScrollTo().  I'm not sure if that issue is this bug,
or another one.  in any case, I'd appreciate a pointer to how to get started
with that.
Attachment #102642 - Attachment is obsolete: true
for those interested in trying this patch, change ScrollByLines() to this:

NS_IMETHODIMP nsScrollPortView::ScrollByLines(PRInt32 aNumLinesX, PRInt32
aNumLinesY)
{
    nscoord dx = mLineHeight*aNumLinesX*SMOOTH_LINE_MULTIPLIER;
    nscoord dy = mLineHeight*aNumLinesY*SMOOTH_LINE_MULTIPLIER;

    ScrollTo(mOffsetX + dx, mOffsetY + dy, 0);

	return NS_OK;
} 

this a really hacky way to increase the scrolling distance when you use your
kb's arrow keys.
> doesn't the nsITimer need access to these?

I don't see why it should. Can you explain what errors you got without changing
Addref/Release?

> I know it's not clear from the diff, but I didn't copy code, I renamed the
> entire ScrollTo() and privatized it.

Yeah, OK, don't worry about it.

> increasing the mLineHeight adds to the usability of the smooth scrolling.

I think I have to try to patch to understand this.

> it's just simpler for me to work with one class.

You don't need a class. Just make a struct with the smooth-scroll variables in
it. You don't need to put methods in the struct.

I'm kind of amazed that this performs well on complex pages, especially on an
old fossil like a 200MHz PII, but I'll take your word for it (and try it for
myself :-) ).

+// the usual scroll cases
+#define SCROLL_DIRECTION_UP    0
+#define SCROLL_DIRECTION_DOWN  1
+#define SCROLL_DIRECTION_LEFT  2
+#define SCROLL_DIRECTION_RIGHT 3
+
+// it's possible to scroll diagonally
+#define SCROLL_DIRECTION_DOWN_AND_LEFT  4
+#define SCROLL_DIRECTION_DOWN_AND_RIGHT 5
+#define SCROLL_DIRECTION_UP_AND_LEFT    6
+#define SCROLL_DIRECTION_UP_AND_RIGHT   7

This really is quite ugly. There must be a better way to do this. How about
making mDirection into two separate variables each of which ranges from -1 to 1?
Then you can replace a lot of your big ifs and switches with straight-line code,
or nearly so.

I still don't see how, if the max frames is reached, you guarantee that we
scroll to the final destination.
"Can you explain what errors you got without changing Addref/Release?"

it compiles fine, but I get a freeze on startup in nsXULWindow.cpp line 935: 
NS_ASSERTION(windowElement, "no xul:window");  I'm not sure if this is my fault
or not, but adding addref/release allows me to run.


"I still don't see how, if the max frames is reached, you guarantee that we
scroll to the final destination."  

I don't.  the scroll will stop wherever it happens to be at that point.  this
may not be needed, but I wanted to stop a huge bogus scroll in case somehow the
code doesn't catch that the destination has already been reached (or whatever).
 I can remove the var and the check if it adds un-needed confusion.


I'll move the members into a struct and see what I can do about cleaning up the
mDirection and then I'll post another patch.
> I get a freeze on startup in nsXULWindow.cpp line 935

Weird. I have no idea why that would be related to the Addref/Release on
nsScrollPortView.

There seems to be some strange usage of aUpdateFlags in various callers of
ScrollTo. I think you should change 'aUpdateFlags==1' to 'aUpdateFlags !=
NS_VMREFRESH_NO_SYNC'. And if you do one of those scroll operations, shouldn't
you kill the timer?

I think you can actually get rid of mDirection altogether. Instead of using it,
just compare the current scroll position to the destination scroll position and
move in the direction of the destination scroll position.
so what do you want me to do about addref/release?  is there someone else I can
ask about that assertion?

> I think you should change 'aUpdateFlags==1' to 'aUpdateFlags !=
> NS_VMREFRESH_NO_SYNC'

yeah, but as you noted there is some strangeness in the callers' use.  if I use
NS_VMREFRESH_NO_SYNC then dragging the scroll bar thumb is totally wacky.  I
think we need to fix the callers, but in the meantime, using '==1' keeps the
scrollbar happy and the keyboard scrolls smoothly.

> And if you do one of those scroll operations, shouldn't
> you kill the timer?

D'OH!  I am killing it on near-concurrent scrolls but not for non-smooth
scrolls.  thank you.

I'm not sure I see the need to move the data members into a struct.  since I
have to do some cleanup on the timer anyway it's going to need to be
instantiated as a null, so I don't see any space savings.  right?

regarding mDirection, you're right I don't strictly need it. but I think having
it makes the code easier to maintain.  getting rid of it means that I would have
to recalculate the direction in EndScrollingIfDestinationIsReached() and
IncrementalScroll().  

also, it might be cleaner if I used 2 seperate mDirections as you noted.  but I
don't see a win there either, since the destination checks (in 2 places) need to
use discrete operators anyway.  I'm sure it could be done in a more fancy way
with less code, but I'm not able see the benefit.  since I'm no c++ wizard, if
you have a more concrete suggestion, I can bang on it to make it work.

I'll wait to hear from you before I post another patch.
ok, the addref/release problem is gone.  I've reverted to having them 'return
1;' and I no longer get an assertion.  

either I was wrong about what in this patch was causing it, or someone else
fixed a misassumption on this code.  either way, it now works fine without the
modifications I made.

I also fixed a possible errant scroll timer.
Attachment #102867 - Attachment is obsolete: true
I tried it out. Very nice. A couple of issues I noticed:
-- When the pref is off, the scroll-by-line multiplier is still in effect.
-- When you hold down the arrow keys (or pageup/pagedown), it scrolls very
slowly (but smoothly!). This is because the timer keeps being interrupted, I guess.
> When the pref is off, the scroll-by-line multiplier is still in effect

this is easy to fix, but the larger issue is that the callers are all over the
place.  to my mind, clicking the scroll ball arrows should be the same as
pressing the down arrow, but such is not the case.  it looks like
nsIScrollbarMediator builds its own target coordinates and ignores ScrollByLine().  

I'd really appceciate some help in the xul area of scrolling.

> When you hold down the arrow keys (or pageup/pagedown), it scrolls very
slowly

this was intentional, but I see how it's not quite optimal.  if you've ever used
IE on a big page, you'd understand why queueing the keypresses is incredibly
annoying (and bogs down the whole machine).  

I'll see if I can come up with some magic to detect that the key is being held
down and keep speed high in that case.

do I hear an a=roc coming?  :)
I'm working on the patch a bit.

Another thing I noticed is that when you go back to a page that you'd scrolled
down, there's a nice smooth scroll from the top to the point you were last at.
That's not desirable.

Your physics logic in GetNextCoordinateIncrement seems wrong. It essentially
returns a velocity, but in your formula s(t) is the position, not the velocity.
Having said that, the visual results seem to be acceptable.
I actually sort of like the 'back' scrolling, although it would be better if it
was much faster.  but I'll defer to you on this.

> Your physics logic in GetNextCoordinateIncrement seems wrong.

hmm. I guess broadly, you could think of the return value as 'the distance to
scroll over a period of time', and can therefore be thought of as velocity.  but
narrowly (the way I think of it) it returns 'the position relative to the
previous position as a function of the elapsed time', therefore it's just a new
position.

if you want to change the semantics of the method or the comments to reduce
confusion, that's fine with me.  

other than that, agreed: the algorithm is the right one, visually.

I'm looking forward to seeing your work!
Did you consider having the scrolling start slow, speed up, and then slow down
again before reaching the target? That might look even better, especially for
short scrolls. I'll try it.
one thing I did consider was reversing the whole thing, i.e., decelerate from an
initially high value rather than accelerate from an initially low value.  this
would have the added benefit of not needing any additional magic when you hold
the key down.

also I considered having a method that returns an array of increments rather
than calling multiple times.  this could potentially make the destination checks
simpler.  I need to learn more about the xpcom arrays before attempting this,
though.

all in all, it's hard to argue that this patch is not usable enough as is...
Attached patch Work in progress (obsolete) — Splinter Review
This is just a checkpoint of where I'm at. The patch mostly works. It avoids
most of the problems we've talked about. It doesn't use any artificial
line-height boosting. Scrolling by key-repeat seems to work pretty well. It
needs more documentation, some of the comments are just lies at the point. The
major missing code is the code to make XUL scrollbar buttons do an
NS_VMREFRESH_SMOOTHSCROLL.
roc, this rules.  a couple issues:

- the patch does not apply to nsViewManager.h  I had to patch manually.
- I think that the line-height increase is pretty important, even if it's not as
dramatic as I had it.  otherwise pecking by line is much slower than with the
pref off.

but otherwise, this is awesome.

I built this on windows.  if anyone wants a build to play with, let me know.
this is actually pretty crashy.  I'm crashing at http://theonion.com and
http://fark.com  

possibly iframes?  or animated gifs?  dunno, I don't have a debug at the moment.
Any hope of this making 1.2 (even turned off)?  I would *love* to get this
enabled in Phoenix by default.  Let me know if I can do anything to help with
the patch.

hyatt, 
can you get the xul scroll bar to call
ScrollByLines(x,y,NS_VMREFRESH_SMOOTHSCROLL);?
alias "smoothscroll"
Alias: smoothscroll
Just thought I'd provide a couple of crude windows benchmarks (1.7GHz athlon,
win2k):

CPU load while "smooth-scrolling" slashdot.org

Opera 6 : <5%
IE5     : 50%-99%

neil>> I'd very much like it if you could mail me a Windows binary (A
mozilla.exe from you is all I need to get this working, right?)
Attached patch new patch (obsolete) — Splinter Review
This patch makes scrollbars set a "smooth" attribute on themselves which gets
checked by nsGfxScrollFrame, so clicking on the scrollbar buttons or on the
scrollbar trough causes a smooth scroll. Seems to work pretty well.

I've also tweaked the smooth scroll parameters so that the smooth scroll
happens in (nominally) 100ms. This reduces the smoothness effect, especially
for lines, but it stops line-by-line scrolling from feeling sluggish. This is a
more conservative approach than multiplying the scroll-by-line amount.

The only thing this patch doesn't handle is scrolling handled by scrolling
mediators (i.e., trees and listboxes, which create and destroy content during
scrolling). I imagine that would be quite a bit more work. We can live without
it for now. Maybe hyatt feels like taking that on :-).

I like this patch a lot and I think we're about ready to land it, but I don't
want this to land in 1.2. 1.3alpha will open pretty soon and we can land it
there.
Attachment #103464 - Attachment is obsolete: true
Attachment #103701 - Attachment is obsolete: true
That approach has a problem that the scroll thumb jumps to the new position
immediately and then jumps back and does the smooth thing. Hmm, this could be
quite hard to fix...
Attached patch new fix (obsolete) — Splinter Review
OK. This patch fixes the thumb jumping by forcing an update of the 'curpos'
attribute to the view's true value after each potentially-smooth scroll
operation. To get this to work properly I had change the reentry strategy in
nsGfxScrollFrame.
Attachment #104988 - Attachment is obsolete: true
Attached patch better patch (obsolete) — Splinter Review
sorry, missing some diffs.
Attachment #104992 - Attachment is obsolete: true
when I apply this patch, it patches and builds fine, but I am once again getting
an assertion at nsXULWindow.cpp line 935.

any ideas?
no idea. "Works for me"

you did a clobber build?
did a clobber, did a new tarball, still no go.  

unpatched tarball builds and runs fine, btw.
What's your compiler and environment?
neil@waylon mozilla $ uname -a
Linux waylon.rackle.com 2.4.19-gentoo-r9 #1 SMP Wed Oct 2 11:21:58 PDT
2002 i686 AuthenticAMD
neil@waylon mozilla $ gcc --version
2.95.3
I suggest you break the patch down into smaller pieces and try to identify the
exact line or lines that cause the assertion to trigger.
roc, what's your compiler and environment?
[roc@ocallahan roc]$ gcc -v
Reading specs from /usr/lib/gcc-lib/i586-mandrake-linux-gnu/2.96/specs
gcc version 2.96 20000731 (Mandrake Linux 8.2 2.96-0.76mdk)
[roc@ocallahan roc]$ uname -a
Linux ocallahan.org 2.4.18-6mdk #1 Fri Mar 15 02:59:08 CET 2002 i686 unknown
[roc@ocallahan roc]$ rpm -q glibc
glibc-2.2.4-25.1mdk
My tree has quite a lot of patches in it, though.
*** Bug 178994 has been marked as a duplicate of this bug. ***
Severity: normal → enhancement
Would the solution to this problem be applied to Mozilla as well as Phoenix?
yes, although the default pref setting may be different.
Any chance that we'll see this implemented in either Phoenix or Mozilla soon? 
It seems like its ready to go and is a vital part of making things appear
'smooth' in Phoenix/Mozilla.
Taking bug. Updated, unbitrotted patch to follow.
Assignee: neil → roc+moz
Status: ASSIGNED → NEW
Component: General → Layout: View Rendering
Product: Phoenix → Browser
Hardware: PC → All
Target Milestone: --- → mozilla1.3alpha
Version: unspecified → Trunk
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #105007 - Attachment is obsolete: true
Comment on attachment 111963 [details] [diff] [review]
Updated patch

dunno who's reviewing XUL changes these days ... kin, if you want to hand the
XUL changes to someone else, let me know (and let me know who to)
Attachment #111963 - Flags: superreview?(kin)
Attachment #111963 - Flags: review?(kin)
this works great with mozilla proper, but it doesn't work with phoenix.  (export
MOZ_PHOENIX=1 and mk_add_options MOZ_PHOENIX=1).  I'm building a debug phoenix
to see what the problem is, but I suspect it's that it's the old assert I was
getting before.  

roc, do you ever build phoenix?  do you get this assertion?

also, there's a problem with scrolling with the keyboard:  if you hold the key
down to scroll a couple of lines, once you release the key, scrolling continues
way beyond where you intended to stop.
I haven't ever built Phoenix. I hope you can find the problem :-).

I see the delay between releasing the arrow key and scrolling stopping is higher
than it should be, but it doesn't feel too bad to me. I don't know what's
causing it. Given that it's pref'ed off, I think we should land what we have for
1.3alpha, if possible, and then tweak it during 1.3beta.
> I think we should land what we have for 1.3alpha, if possible, and then
> tweak it during 1.3beta.

Uh... 1.3 Alpha came out last month.
Er, yeah. Whatever :-).
Attachment #111963 - Flags: superreview?(kin)
Attachment #111963 - Flags: review?(kin)
Attachment #112021 - Flags: superreview?(kin)
Attachment #112021 - Flags: review?(kin)
Attachment #112021 - Flags: superreview?(kin)
Attachment #112021 - Flags: review?(kin)
Attached patch new patchSplinter Review
Sorry. That patch had one hunk from some other work I was doing, which caused
scrolling views not to be clipped properly. Very very bad. This is the right
one.
Attachment #112021 - Attachment is obsolete: true
Attachment #112026 - Flags: superreview?(kin)
Attachment #112026 - Flags: review?(kin)
*** Bug 33966 has been marked as a duplicate of this bug. ***
note:  "home" and "end" do not scroll smoothly with this patch.  looking at the
code...
dumb question: why would you want home and end to scroll smoothly?
Looks like this is basically done. Are we going to see this in an official 
release sometime soon?
This code is basically ready except for the Phoenix problem that Neil was
worried about. I have just built Phoenix to check out this problem. Once I've
resolved it I'll try to get this landed.
A while ago I did something similar for gtk+ (see
http://mail.gnome.org/archives/gtk-devel-list/2003-January/msg00083.html). Based
on that I have two comments:

     (a) To me it looks like you are cancelling and restarting the animation 
         every time a new scroll event arrives. This means that if the events
         are arriving in quick succession, you will cancel and restart the
         animation in quick succession; in other words, the contribution of
         each scroll event will be small if the time between successive events
         is small. The speed of the scrolling will actually be constant
         no matter how fast you roll the wheel.

         In the gtk+ patch I maintain a "goal value" and keep track of how
         much time has passed since the last scroll event. There is a constant
         "time to complete" (the time after any given scroll event within which
         the scrolling must be complete. Set to 0.08s in the current version of
         the gtk+ patch).

         Based on that information the animation callback computes the
         appropriate frame to show. The difference to this patch is that
         when a new scroll event arrives, the gtk+ patch adds to the goal
         value and resets the "elapsed time" to 0. This means that a series of 
          
         scroll events in quick succession will lead to a high goal value with
         constant "time to complete" value, so that the speed of the scrolling
         will be proportional to the speed with which you are rolling the
         wheel.

     (b) You are using a timer to drive the animation. In the gtk+ patch I
         used an idle handler. Using an idle handler means a smoother effect
         at the cost of more CPU usage. I don't know if Mozilla has the
         concept of an idle handler.

In my opinion (a) is a real problem, and (b) is at least debatable.

I should note that I haven't tried the patch, nor do I know anything
about the mozilla source code, so it is possible that I have just
misread the patch.
I'm doing exactly what you suggest. The "goal value" is (mDestinationX,
mDestinationY), and the "elapsed time" is mFrameIndex.

Good point about the timer vs the idle loop. I assume the idea with the idle
loop is that you paint as many frames as you can, driving system usage to 100%.
We could effectively do that here by setting SMOOTH_SCROLL_MSECS_PER_FRAME to,
say, 1, in which case we'd just be painting as often as we can. (We already
handle the case when painting takes longer than SMOOTH_SCROLL_MSECS_PER_FRAME,
because that can easily happen on complex pages.) After I land the patch we can
tweak these parameters.
I built Phoenix with this patch. It works perfectly. We're good to go.
Should the patch work on Mozilla too? Building mozilla with attachment 112026 [details] [diff] [review] on
Linux, build terminated:

nsScrollPortView.cpp: In member function `virtual nsresult
   nsScrollPortView::ScrollToImpl(int, int, unsigned int)':
nsScrollPortView.cpp:596: `ClampScrollValues' undeclared (first use this
   function)
Yes, it should build. I can't explain your error since ClampScrollValues is
clearly defined in the patch.
Robert: I'll admit that I was originally a bit skeptical about smooth scrolling,
but Soeren's link in comment #62 changed my mind -- and I now look forward to
being able to scroll and read text at the same time :).

Do you think the patch is soon ready to land, or are R.K.Aa's compiler
difficulties postponing that?
I'll land it once it's reviewed.
I have tried the patch now, and compared to my gtk+ patch I think this one has a
slightly distracting or unpredictable feel. My guess is that this is because
of the variable-velocity thing, but I am not sure.

The gtk+ patch does do any velocity changes. I just moves linearly towards the 
current goal value always.

(I do agree that the patch doesn't have the "point (a)" problem from my previous 
comment).
We can fine-tune the policy after this lands. It's going to be off by default.
Soeren Sandmann wrote:
> The gtk+ patch does do any velocity changes. 

I'm guessing you meant "doesn't", there?
Right.
Comment on attachment 112026 [details] [diff] [review]
new patch

The patch looks fine to me.

==== This change concerned me at first, because it looked like it would be
active
even when the pref was off, but then I realized that ALWAYS_BLIT was a scroll
property, not an update flag. There are several other places that do this same
thing in other files, do we want to clean them up in the same sweep?


-  scrollable->ScrollTo(aX,aY, NS_SCROLL_PROPERTY_ALWAYS_BLIT);
+  scrollable->ScrollTo(aX, aY, aFlags);


==== Hmmm, I noticed a comptr was being used for an nsIScrollbarFrame around
some   code you touched.


-  if (!aVisible)
+  if (!aVisible) {
      content->SetAttr(kNameSpaceID_None, nsXULAtoms::collapsed,
NS_LITERAL_STRING("true"), PR_TRUE);
-  else
+  } else {
      content->UnsetAttr(kNameSpaceID_None, nsXULAtoms::collapsed, PR_TRUE);
+  }

   nsCOMPtr<nsIScrollbarFrame> scrollbar(do_QueryInterface(aScrollbar));
   if (scrollbar) {


I'm curious how this affects performance in the editor, if at all, since we
call ScrollIntoView() during every edit action.

I'm all for landing this with the pref off, but I'd like to make sure any
impact on editor is kept in mind before it gets turned on.

sr=kin@netscape.com
Attachment #112026 - Flags: superreview?(kin)
Attachment #112026 - Flags: superreview+
Attachment #112026 - Flags: review?(kin)
Attachment #112026 - Flags: review+
> NS_SCROLL_PROPERTY_ALWAYS_BLIT

These flags are always ignored. (Since a bug fix of mine, long ago.)

> Hmmm, I noticed a comptr was being used for an nsIScrollbarFrame around
> some code you touched.

Oh yeah! Bad.

> I'm curious how this affects performance in the editor

Good question. We'll see.


Checked in. Thanks Neil.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
So, what's the pref to activate this?
general.smoothScroll according to the patch source.
Verified fixed on Linux, with pref.  Woo, I rather like this.  Good work.

Does this get final verification when enabled by default on all platforms,
or does its default enabling constitute a different bug?
can we please not make this enabled by default? this is among the top things I
hate about msie.
I think we'll make a decision about whether or not to enable it by default
around the time of 1.4beta, after we've collected some feedback. We could make
different decisions for different products, too (the reason Neil put the pref in
in the first place).

biesi: I'm not very convinced by the "I don't like this" school of UI design
:-). At the very least we should take a poll. I think we should probably add an
nsILookAndFeel interface to access the platform's smooth scrolling prefs and
disable this if it's disabled on the platform, presumably that would take care
of things for people like you.
>to access the platform's smooth scrolling prefs

hm, I don't think such a thing exists on linux


also... it would probably be a good thing to add UI for this preference.... I'm
sure I'm not alone in not liking this :) And if we ship with it disabled, all
the more reason for having UI, so that users can find it.

a poll is probably a good idea, I guess.
maybe I'm missing something, but i can't get this working on build 2003032308. 
i added pref("general.smoothScroll", true); to the main prefs and
user_pref("general.smoothScroll", true); to my user.js file.  nothing happens
It wasn't in 2003032308.  Try today's build.
biesi: for the pref ui, see attachment 102643 [details] [diff] [review] for phoenix and attachment 95522 [details] [diff] [review]
for mozilla.

they may be out-of-date, but I'm sure with a few minutes of work they would apply.

since they are different patches and different apps may want to do different
things, perhaps there should be one bug per app to get them landed/not landed?
I just filed bug 198964 about setting the default pref.

I don't think we should have UI for this.
I agree with Robert, this thing does not need an UI pref, at least not in
Phoenix. It should be on by default, and those who don't like it (a.k.a.
power-users) know how to turn it off.
speaking for Mozilla (not Phoenix), I think there should definitely be a UI
option for smooth scroll, no doubt about it.  you can't expect everyone to be
savvy about the user.js file.  there is a huge bucketfull of users who will
expect to see options like this in the Preferences dialog.
Since smooth scrolling is supposed to emulate IE, we should go all the way and
add the pref, just as IE does.
Did this cause Bug 199024?
Tested on 2003032408 Win32.  Works, but not great.  I would describe MSIE's
behavior as "smooth".  Right now, we're just "less jerky".  This implementation
will be improved right?
I agree with comment #90. On Windows, it's not really very smooth, especially if
you keep the arrow keys pressed. It's better on Linux, althogh not really
comparable to MSIE or the smoothness that Mozilla exhibits when scrolling by
dragging the scrollbar.
Are you holding the arrow keys down? That may be a bit jerky. Maybe I should try
a constant-velocity approach.
When viewing a page and scrolling right/left, Mozilla will occasionally either
lock up (the window stops refreshing) or it will simply die (the window disappears).

I'm seeing this on Linux 2.4.18 on an ATI Radeon VE with X 4.3.0 plus a fairly
recent DRI from CVS.

Other than that and the actually-not-too-smooth scrolling, this patch seems to
work well. When I compared it to IE on a P3-500 running Win2k, they were both a
little jittery scrolling maximized windows at 1280x1024. 

Would it be possible to give users some interface for manipulating the variables
(speed, etc.) ? It might make it easier for the non-coders to experiment with
different settings.

Thanks to the makers of this patch!
roc, one thing that I started to implement (but never got working correctly) is
to catch when the user holds down the keys (by detecting timer deletes/resets),
and if so, don't reset the initial velocity for the next scroll.  this approach
may not work very well for the velocity formula you used, but I think something
along these lines would be *way* better than a simple const velocity scrolling,
which I think makes it too hard for the user to follow the document, especially
on lcd displays which are slow to update.

another approach which may make the problem moot is to scroll many more lines at
a time so that the user does not have to hold down the key at all.  if the user
is casually scanning (even a long) document, making the scrolling distance, say,
8 lines is very readable, plus the user has to scroll 1/8th as much.  this is
the way I did it in my original patch, for this very reason.  I think folks
looking for parity with IE will also be happier with multi-line scrolls.
Scroll button scrolling on Windows seems better than before.  I'm not sure I've
it's my imagination or not, but it's just a *little* better.

Is (not autoscroll) smooth scrolling with the scroll wheel a separate bug yet?
WFM, fresh CVS build on WinME.

Actually I like this "jerky-smooth" scrolling much better than the "silk-smooth"
scrolling of IE (if I have to use IE on a computer, smooth scrolling is one of
the first things I switch off). Therefore, it was a surprise to me that I like
the new smooth mode of Mozilla.

It seems it will be hard to please everyone...
Please promise me this is never going to be on by default. I feel dirty just
knowing this is in.
That last comment is a bit unhelpful.

The point is that we are up against Microsoft and Internet Explorer, and it has
smooth scrolling on by default.  There is no question that it helps the
readability of text, and I think that anything we can do to ease the transition
is a step forward.
Agree with comment #98.  When reading a long article with mostly text and no
paragraph spacing, it's much easier to keep track of where I am when smooth
scroll is on.

I have an idea for the developers.  Why not start with smooth scroll off, and
then listen for excessive mouse wheel events or cursor key ups/downs.  After X
occurrences, pop up a dialog asking "Would you like to enable smooth scroll?"
with the answers Yes/No, and don't ask again but maybe tell them where to find
the option in preferences.
This behaviour makes me feel (literally) nauseous. Please don't turn it on.
why would people who scroll fast want to _enable_ smooth scrolling? it rather
seems that they would want it disabled (so that scrolling is faster)
Since Mozilla is really for testing purposes, let's leave it on by default and
see how people react. Please make sure the UI goes in for the pref.
biesi: this patch never slows down scrolling. If you're scrolling fast for
whatever reason, we'll keep scrolling at that speed, we'll just drop scroll
animation frames if the CPU can't keep up.

People who want to scroll fast and whose CPU can keep up may benefit from this
patch, because the extra scroll animation frames may establish more visual
continuity.

I can't think of any way to make this "discoverable" if it's turned off by
default. Certainly it's not going to be turned on by default anywhere until the
remaining bugs are fixed.
verifying that the implementation (other bugs not withstanding) is in.
Status: RESOLVED → VERIFIED
After I've enabled this, Mozilla (build 2003032611, Win32) sometimes stops
responding during scrolling. Only ctrl-alt-del remains. I once got an error
about 'low memory' during one such freeze-up. Could smooth scrolling be causing
some kind of memory leak?
smoothwheel as an extension (targets the same functionality as smoothscroll).
it was done before smoothscroll was released (and i also wasn't aware of it),
but untill i set up the mozdev account, smoothscroll was released. has a
somewhat different approach to the scroll timing. not as complete as
smoothscroll's scope. very short javascript implementation.

homepage:
http://smoothwheel.mozdev.org

thread on mozillazine:
http://www.mozillazine.org/forums/viewtopic.php?t=8096

have fun
avih.
Hmmm. Neither my debian mozilla-snapshot from 2 days after the checkin or my cvs
build from yesterday have the general.SmoothScroll setting. Nor is it to be
found in any of the {unix,all}.js files? Most I do something else special to try
this out?
It is general.smoothScroll, lowercase s

and it is not expected to be part of any .js file, you must add it manually
When I scroll my main in the preview pane using spacebar=pagedown I get an
"Advance to next unread message" box after every screenfull. This should only
happen when I reach the end of the message. This behaviour went away when I
turned smoothScroll off.
Dave, that sounds like a seperate bug from this (although probably caused by
this one).

would you file a new bug for this issue?
CC: dcroal

Dave: Unless you add yourself to the CC list when you add comments, you won't
receive any of the replies ;). (such as the reply in comment #110)
I added new bug 200045 for my MailNews smoothscroll problem.
Thanks.
For those people who have performance problems due this feature:
Add the following line to your prefs.js (~/.phoenix/default/*/prefs.js) file:
-- snip --
user_pref("viewmanager.do_doublebuffering", false);
-- snip --
This may - depending on the gfx card&&driver being used - speed-up smooth
scrolling a lot (on my dumb m64 framebuffer it's a difference between day and
night... :)
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.