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.

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

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.

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.

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...

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"

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?)

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.

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...

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.

sorry, missing some diffs.

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'm stumped.

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. ***

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.

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)

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 :-).

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.

*** 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

> 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.

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.

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... :)