Bug 32356 – Implement smooth scrolling for multiple platforms

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Peter Kasting]

Title of this bug is vague because I'm not sure precisely how far I'll have to go.  At the very least I'd like a platform-neutral framework that different platforms plug different acceleration curves into, and a Windows implementation that Chromium could use (and probably Safari/Win too, which I assume would want the same behavior)

Some investigation of IE 8 and Fx 3.5 on Windows:

Points true of both, which we should copy:
* Clicking in the track, clicking on the scroll arrows, pgup/pgdn/space/up arrow/down arrow all smooth-scroll.
* Thumb dragging and shift-clicking do not smooth-scroll.
* Smooth-scrolling also smooth-scrolls the thumb.

IE-specific details:
* Animation curve may be nonlinear (initial acceleration, maybe closing acceleration)?  Hard to tell since it's so fast.
* Rate is calculated based on what type of scrolling is being done, not how far the thumb actually has to travel.  (Good, see below)
* Reversing direction in mid scroll completes the old scroll and then reverses.  (Slightly odd-feeling)

Firefox:
* Smooth scrolling cuts the net speed of scroll-with-scroll-arrows in half.  (Ugh)
* Animation curve may be linear, hard to tell (I didn't go look at the source code)
* When scrolling repeatedly (e.g. holding a button down), the scrolling is done in segments just like w/o smooth scrolling, each of which is scrolled smoothly over the time until the next update.  As a result, the last segment of a scroll, which may be shorter, is scrolled "slower" (often noticeably slow), which feels very weird.
* Reversing direction in mid scroll affects scrolling immediately.

Behavioral goals for our implementation:
* Determine desired steady-state scroll speed from scroll type (page vs. line), not distance to scroll (i.e. like IE, not Fx).
* Accelerate current scroll speed at a constant rate to reach desired speed.  Ramp time should be a small fraction of the standard time-per-scroll-increment, e.g. 0.25x.  The overall scroll will end up taking this amount of time "extra" (regardless of net distance scrolled) and will be half this amount of time "behind" the non-smooth scroll in steady-state.
* Ramp down linearly using same constant to stop at endpoint.
* Reversing direction in mid scroll should take effect immediately (i.e. like Fx, not IE), but precise algorithm for changing velocity is open to question; either reversing the acceleration or reversing the current velocity might be OK.

Implementation details:
* We'll need to add "desired position" and "desired velocity" variables to Scrollbar, as well as a Timer to do the updating.  We could theoretically use the existing timer if our smooth scroll update rate was a divisor of the autoscroll rates, but it seems better to not rely on that. 
* CSS animations look like they fire every 25 ms (= 40 FPS), perhaps we should use the same value for the smooth scroll updates.
* ScrollbarTheme is going to need ways to specify the smooth scrolling details (including "do we do it at all").  I don't know how much I need to parametrize because I don't know what the Mac smooth scrolling desired curve looks like.  Hyatt, feel free to fill me in...

[Peter Kasting]

More implementation detail:
* Programmatic scrolling (which we don't want to smooth-scroll) calls into setValue(), not scroll().  Both scroll() and setValue() call setCurrentPos().
* Middle-mouse-autoscroll (which we also don't want to smooth-scroll) also calls setValue().
* Wheel scroll (which we do want to smooth-scroll) also calls setValue().
* Dragging the thumb (which we don't want to smooth-scroll) calls the various mouseMoved()/moveThumb() functions.
* Scrolling by holding down keyboard keys (which we do want to smooth-scroll) calls scroll() repeatedly, never the timer-based functions called when a user holds down the mouse on a scrollbar part.

Not totally clear what should happen when multiple of these are in play at once (e.g. dragging the thumb + flicking the wheel).

Clearly, I'm going to need to parametrize setValue() so that callers can indicate if they want smooth scrolling.

[Peter Kasting]

(In reply to comment #1)
> Clearly, I'm going to need to parametrize setValue() so that callers can
> indicate if they want smooth scrolling.

Lies.  As an API name, "setValue()" is pretty clear.  It doesn't mean "scroll to value".  I should instead change wheel scrolls to go through a different function, probably scroll().

[Peter Kasting]

IE's acceleration curves for mouse wheel scrolling are more obvious than for other methods.  There they seem to have no deceleration and either a linear or superlinear acceleration.

There are a number of implementation bits I missed nothing above:
* We must account for timer lag lest smooth scrolling slow down net scroll speed on pages that don't scroll quickly.
* There are problems with cases like mouse wheel scroll, where we can get a rapid series of small scroll events.  In this case, for each event we'll micro-smooth-scroll the event (perhaps not smoothing at all if the distance is short compared to our initial smooth scrolling velocity) and then possibly do it again an extremely short time later for the next event, adding up to a different (less smooth) appearance than if we had done a single scroll of double the distance.  It's not obvious to me what the best solution is here.  Perhaps I need to have a far different acceleration curve for wheel scrolling.

[Peter Kasting]

Another data point: Neither Firefox nor IE smooth-scroll for document-sized scrolls (e.g. pressing home or end).

[Peter Kasting]

OK, I have a working patch I'll post soon.

More implementation notes:
* I dropped the idea of a deceleration at the end of the scroll.  It's tricky to figure out how to do this exactly right, and after thinking about it more I decided it probably wouldn't feel all that great anyway.  Added bonus: this cuts the net smooth-scrolling-imposed constant lag by half compared to my first design.
* For keyboard scrolls, because the key repeat rate might be higher than the autoscroll timer repeat rate (which is what I used to calculate the desired steady-state scroll velocity), we could get scroll increments queued up, resulting in scrolling for a while after releasing the key.  Because I don't know of a good way of checking the precise repeat rate (and didn't want to try to infer it with a heuristic), I inserted code that basically discards additional keyboard scrolls when we've already got some queued.  This has the effect of clamping the maximum keyboard scroll rate to match what happens when you hold the mouse down on parts of the scrollbar, which doesn't seem too bad to me.  Sadly, this meant plumbing a bool through basically every scroll-related function in WebCore, ugh; the alternative was extending the ScrollGranularity enum to hack this property in, which seemed like a bad superposition of discrete meanings.
* I decided to deal with the wheel scroll case by setting the desired steady-state velocity based on how far our desired scroll position is from our current one.  This makes wheel scrolls feel similar to other types of scrolls (see comment in the code for why), with the nice property that at both low and high speeds the scroll is smooth and not laggy.
* I don't know precisely what kinds of scroll algorithms different platforms will want, so for now I've only added a few knobs to the ScrollbarTheme.  We can of course add to these.

In my testing, Chromium built with this patch feels subjectively superior to both IE's and Firefox' smooth scrolling implementations.  Hoorj.

[Peter Kasting]

Created attachment 44720 [details]
patch v1

[WebKit Review Bot]

Attachment 44720 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/Scrollbar.cpp:232:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1

[Peter Kasting]

Created attachment 44721 [details]
patch v1.1

I wasn't even touching that line of code... :(

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44721 [details] without any errors.

Comment on attachment 44721 [details]
patch v1.1

It seems odd that smooth scrolling is implemented in Scrollbar, as it’s not a scrollbar feature, but rather a feature of scrollable areas.

This implementation of the scroll animator and additional scrolling behavior in cross-platform code does not enable ports to have their own smooth scrolling behavior that matches their platforms’ native behavior. Therefore, I think there’s no point doing things this way.

[Peter Kasting]

(In reply to comment #10)
> (From update of attachment 44721 [details])
> It seems odd that smooth scrolling is implemented in Scrollbar, as it’s not a
> scrollbar feature, but rather a feature of scrollable areas.

The code in Scrollbar is not just a View observing how an area is scrolled.  It function as a Model responsible for handling scrolling, calculating how things should be updated and sending out those updates.  Note how the scrollbar itself, not just the web content, is smoothly updated.  I don't think it's feasible to put this code anywhere else.

> This implementation of the scroll animator and additional scrolling behavior in
> cross-platform code does not enable ports to have their own smooth scrolling
> behavior that matches their platforms’ native behavior.

It's impossible for me to design anything different without knowing the precise details of how each platform smooth scrolls.  I put some basic tweaking knobs in to start with.  We can change them if we need to.  But this should be most of what any platform needs.

I had discussed this briefly with hyatt on IRC before launching into things and was under the impression that this was the desired route to take -- as I said above, it's the only way I can even imagine writing it.  And the whole point is to put much of the machinery into cross-platform code; even if we could keep it solely in platform-specific files, every platform would have to completely reimplement it.  That isn't how we've done anything else around Scrollbar, where everything is cross-platform with some oracle functions in ScrollbarTheme, so why is it desirable here?

It is extremely discouraging to put a completely functional implementation up (not just a prototype, but something which has already gone through one round of testing and tweaking) and have you say it's totally wrong but offer absolutely no insight on the concrete details of what it fails to accomplish or what the alternative design is that you think is correct.  What are the specifics of Mac smooth scrolling that are unrepresentable in this algorithm?

[Peter Kasting]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 44721 [details] [details])
> > It seems odd that smooth scrolling is implemented in Scrollbar, as it’s not a
> > scrollbar feature, but rather a feature of scrollable areas.
> 
> The code in Scrollbar is not just a View observing how an area is scrolled.  It
> function as a Model responsible for handling scrolling, calculating how things
> should be updated and sending out those updates.

This wasn't as clear as it could have been.  Let me try again.  Apologies if this is already clear to you (though if it is then I'm even more confused by your comments).

Smooth scrolling _is_ a scrollbar feature.  It affects all the types of scrolling performed inside the scrollbar -- single scrolls up or down and autoscrolling, triggered by mouse or keyboard -- and causes them to appear to the scrollbar client as more frequent, smaller scrolls.  That it's the scrollbar which performs these is clear from the scroll() API, which takes a high-level description of the type of scrolling to be performed and decides what the ultimate results will be.  Callers don't delve into the low-level details of what the appearance of this scroll will be, and clients don't do anything more complex than simply respond to the controlling scrollbar.

By contrast, the types of "scrolling" which are simply setting the scroll position, rather than moving from one position to another, bypass scroll() and call setValue() instead.  Even here, it is the Scrollbar object which tracks the scroll position and notifies the client that it has changed, but in this case the Scrollbar is not really transforming a high-level input into low-level outputs.

If I try to imagine putting smooth scrolling on the scrollview, then I get the strange effect that the scrollbar tells the scrollview to change its position, and then the scrollview, having converted that to a series of smaller scroll increments, tells the scrollbar to change _its_ position (backwards).  Not only does this possibly result in scrollbar jitter, and represent the first ever communication backwards from scrollview to scrollbar (muddying the model/view picture), but it also tangles with any autoscrolling the scrollbar is in the midst of performing.

Perhaps I'm totally misinterpreting where you were thinking the smooth scrolling code should go?

> It's impossible for me to design anything different without knowing the precise
> details of how each platform smooth scrolls.  I put some basic tweaking knobs
> in to start with.  We can change them if we need to.

I should have mentioned more specifically that it's really easy to extend the set of knobs to make the scrolling algorithm arbitrarily complex.  I avoided doing this not because it would be hard but because I didn't want to parameterize what I didn't know needed to be.  The few knobs in there are samples and the things it seemed most likely for a platform to want to change, like whether there was an initial acceleration curve and how long it took.  And of course there's a function for a platform to disable smooth scrolling entirely, either because it never wants to do it or because we don't yet have the implementation correct.

If, for example, a platform needs nonlinear acceleration, it would be very easy for me to change the oracle function to take an input time and output an acceleration or velocity.

Some quick observation of Mac smooth scrolling suggests it has linear acceleration (over a longer time period than in my default implementation) as well as deceleration.  If you have precise details that'd be awesome.  It's definitely feasible to support this.

(In reply to comment #11)
> What are the specifics of Mac smooth scrolling that are unrepresentable in this algorithm?

The shape of the animation curves, the total duration of the animation, how the animation changes when the target is changed during animation, scroll by document being animated, and no dropping of keyboard events are the ones I have noticed.

Rather than trying to add just enough complexity to the cross-platform code to support any known platform behavior, leaving the animator implementation to platform-specific code allows for arbitrarily complex behavior, and for leveraging—rather than replicating—platform code. This last point, while not especially important for Mac OS X WebKit, can benefit ports that may run on different versions of the underlying OS, which may have different behaviors.

[Peter Kasting]

(In reply to comment #13)
> Rather than trying to add just enough complexity to the cross-platform code to
> support any known platform behavior, leaving the animator implementation to
> platform-specific code allows for arbitrarily complex behavior, and for
> leveraging—rather than replicating—platform code.

What are you suggesting, concretely?  That the implementations of setDesiredPos(), smoothScroll(), etc. all be placed in PlatformScrollbar?

The downside of this is that on platforms which have no platform code that can be hooked at a fine-grained level to get the right scroll behaviors out, which is certainly Windows and I imagine the majority of other OSes WebKit is ported to, this does cause replication, on every platform that has to completely reimplement smooth scrolling.  Is even Mac able to provide the hooks you need here?  After all, we're not actually using native widgets, so we can't simply toggle a smooth scroll attribute and be done.

It would help me immensely for you to lay out a detailed, concrete design for what the platform-specific implementation you're suggesting looks like.

How about something like this? A scrollable area can own an instance of ScrollAnmiator, which in turn has a (weak) reference back to the scrollable area (a ScrollbarClient or a perhaps ScrollAnimatorClient?). The ScrollAnimator has methods to stop animation, query if animation is in progress, set the animation target (a 2D offset; animating each scroll axis independently is yet another thing that makes the Scrollbar-based implementation unsuitable for Mac OS X), and query the current animation target. Upon setting a target, the ScrollAnimator drives its client to the target. The ScrollbarClient interface is extended to allow scrollbars (and other callers) to specify whether a scroll operation should be immediate or may be animated. ScrollAnimator implementations are platform- and/or port-specific.

[Peter Kasting]

Thanks a bunch for the detail, that makes things noticeably clearer.

It seems a little odd that the flow for setting a target is Scrollbar->ScrollbarClient->ScrollAnimator->ScrollAnimatorClient, rather than just Scrollbar->ScrollAnimator->ScrollAnimatorClient, especially if the Scrollbar is the one determining whether animation should actually occur-- it seems slightly more natural if the Scrollbar just tells the animator "here's what happened" and the animator decides everything else.  Do you think this is a reasonable model?  A downside would be if the Scrollbar wants to tell the ScrollbarClient something that the ScrollAnimator is never going to care about, we'd have to have useless passthrough functions.

One other bit of strangeness with today's code is that there seems to be a second implementation of scrolling inside the RenderListBo, so that scrolling <select> dropdowns smooth-scrolls today (but feels strange doing it).  I don't understand how this code works and I am suspicious that maybe it should be refactored.

(In reply to comment #16)
> It seems a little odd that the flow for setting a target is
> Scrollbar->ScrollbarClient->ScrollAnimator->ScrollAnimatorClient, rather than
> just Scrollbar->ScrollAnimator->ScrollAnimatorClient, especially if the
> Scrollbar is the one determining whether animation should actually occur-- it
> seems slightly more natural if the Scrollbar just tells the animator "here's
> what happened" and the animator decides everything else.  Do you think this is
> a reasonable model?  A downside would be if the Scrollbar wants to tell the
> ScrollbarClient something that the ScrollAnimator is never going to care about,
> we'd have to have useless passthrough functions.

I guess I just find it odd that all manner of scrolling goes through Scrollbar. For example, if the area is scrolled to the bottom right corner and you press Home, ideally the ScrollAnimator should just be told to go to the origin once, instead of having the horizontal scrollbar tell it to go to the left and then the vertical scrollbar tell it to go to the top(-left). Then again, since the end result will be the same (at least for the animator implementation I have in mind), I don’t think it’s important to do it one way or another in the context of fixing this bug. “Whatever goes with the existing code” seems fine to me.

> One other bit of strangeness with today's code is that there seems to be a
> second implementation of scrolling inside the RenderListBo, so that scrolling
> <select> dropdowns smooth-scrolls today (but feels strange doing it).  I don't
> understand how this code works and I am suspicious that maybe it should be
> refactored.

Hm… <select> drop-downs are implemented by PopupListBox. The Chromium variant (PopupMenuChromium.cpp) appears to be a FramelessScrollView. Not sure how it manages scrolling. Other variants appear to use different scrolling mechanisms (if any).

[Peter Kasting]

(In reply to comment #17)
> I guess I just find it odd that all manner of scrolling goes through Scrollbar.
> For example, if the area is scrolled to the bottom right corner and you press
> Home, ideally the ScrollAnimator should just be told to go to the origin once,
> instead of having the horizontal scrollbar tell it to go to the left and then
> the vertical scrollbar tell it to go to the top(-left). Then again, since the
> end result will be the same (at least for the animator implementation I have in
> mind), I don’t think it’s important to do it one way or another in the context
> of fixing this bug. “Whatever goes with the existing code” seems fine to me.

I think that's a valid issue but slightly different than the one I was raising -- it has to do with the code one level up, which is currently calling the scrollbar methods.  It's not clear to me whether that should tell a scrollview to change its position rather than a scrollbar, or maybe even talk to a scrollanimator directly.

> > One other bit of strangeness with today's code is that there seems to be a
> > second implementation of scrolling inside the RenderListBox, so that scrolling
> > <select> dropdowns smooth-scrolls today (but feels strange doing it).  I don't
> > understand how this code works and I am suspicious that maybe it should be
> > refactored.
> 
> Hm… <select> drop-downs are implemented by PopupListBox. The Chromium variant
> (PopupMenuChromium.cpp) appears to be a FramelessScrollView. Not sure how it
> manages scrolling. Other variants appear to use different scrolling mechanisms
> (if any).

Sorry, I'm referring to <select multiple>.  See the code in RenderListBox.cpp compared to RenderLayer.cpp.  There are different implementations of panscrolling, for example.

[Peter Kasting]

I'm not sure having the ScrollAnimator be a separate object buys much.  Looking at the current code, maybe the easiest thing to do is simply to extend the ScrollbarClient interface slightly.

The trickiest bit here is making sure the scrollbar itself is animated in sync with the page.  It seems like the theoretical best way to do this would be to make the scrollbar be a dumb view of the relevant scroll position, which would be kept on the scrollanimator or scrollable view.  I'm concerned about how large a change this would be.

Maybe a fallback is to add a parameter to Scrollbar::setValue() that indicates it should not ask the animation to reset, then plumb that through the scrollable view, and have animation-related updates set that flag.  Then you get a call pattern that looks something like EventHandler->ScrollView->Scrollbar->ScrollbarClient->Scrollbar.

I really wish I had hyatt's input here; I'm much less familiar with the ScrollbarClient/ScrollView side of things than the Scrollbar side.

[Peter Kasting]

Created attachment 44938 [details]
patch v2

This patch implements something like what mitz described.  There is a ScrollAnimator class that can be subclassed on particular ports or platforms to perform one- or two-dimensional animation; the APIs to call it are extremely simple, so the class can implement pretty much any algorithm it desires.  There is a ScrollAnimatorClient class that the animator uses to convey the scroll amounts back to the scrollbars; for simplicity's sake, ScrollbarClient inherits from this class.  ScrollbarClient also adds the couple of hooks needed for the Scrollbar to communicate to the ScrollAnimator that a scroll is desired.

ScrollAnimator.h contains a stub implementation that will do no scroll animation (i.e. the same behavior as before this patch), and everyone but PLATFORM(CHROMIUM) uses it.  I also thought about putting the one relevant function here in something like a ScrollAnimatorNone.cpp and compiling that in to projects where appropriate, I wasn't sure which would be better.

The smooth-scrolling logic from the last patch is moved to ScrollAnimatorChromiumWin.  It has also been updated to not clamp keyboard scrolling rates to the scrollbar autorepeat rate (I found a way to make keyboard scrolling fairly smooth even at higher rates).  We still do need the added plumbing in the rest of the code that tells us whether we're performing a keyboard-triggered scroll or not.

There are a few problems with this patch:
* XCode project not updated with ScrollAnimator.h -- I don't have a Mac with XCode handy (someone want to add the relevant bits here so I can put them in the patch?)
* Won't link on non-Windows Chromium (need to get the right #ifdefs in place)

[Peter Kasting]

Created attachment 44939 [details]
patch v2, for reals

Argh, try checking the "patch" box

[WebKit Review Bot]

Attachment 44939 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/ScrollAnimator.h:66:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/chromium/ScrollAnimatorChromiumWin.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/chromium/ScrollAnimatorChromiumWin.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3

[Peter Kasting]

Created attachment 44940 [details]
patch v2.1

Try to fix style errors

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44940 [details] without any errors.

Comment on attachment 44940 [details]
patch v2.1

Thanks, this looks promising! I am going to need some more time to review the entire patch. Here are a few comments, not necessarily the most important or deepest ones (the fact that I refer to a method or use a parameter name in the following doesn’t mean that I think that it should stay :) ).

1. Since it turns out that all ScrollAnimatorClients are ScrollbarClients, you can probably roll the former interface into the latter
2. Empty method implementations are written as { }, not {}
3. Consider using an enum type instead of bool for fromKeypress, to improve clarity of call sites that currently just pass 'false' or 'true'
4. changedSomething should be a bool&, as it’s not allowed to be 0 anyway
5. We are trying to move away from passing 2D offsets and sizes as 2 ints, so setScrollOffsetsFromAnimation() should take a const IntSize&

[WebKit Review Bot]

Attachment 44940 [details] did not pass chromium-ews:
Full output: http://webkit-commit-queue.appspot.com/results/128553

[Adam Barth]

Sorry for the noise.  This appears to be a bug in chromium-ews.

[Peter Kasting]

(In reply to comment #25)
> 1. Since it turns out that all ScrollAnimatorClients are ScrollbarClients, you
> can probably roll the former interface into the latter

OK.  I debated this and initially decided the current route was clearer, but thinking more, I think you're right, it would be better to just have ScrollbarClient.  (Added bonus: makes ScrollAnimator.h only contain one class, its namesake.)

> 2. Empty method implementations are written as { }, not {}

Will fix.

> 3. Consider using an enum type instead of bool for fromKeypress, to improve
> clarity of call sites that currently just pass 'false' or 'true'

The tempting route if I go with an enum is to enumerate all sources (e.g. KEYPRESS, WHEEL, PANSCROLL, PROGRAMMATIC, etc.).  That would definitely add clarity.  My worry is that I might not know the right type everywhere.  Probably this is just due to me not knowing the code well enough.  I'll look into doing this.

> 4. changedSomething should be a bool&, as it’s not allowed to be 0 anyway

OK; should that be added to the style guide?  Right now it doesn't say anything about when to use one or the other (I'm coming from a background of the Google style guide, which forbids non-const refs.)

> 5. We are trying to move away from passing 2D offsets and sizes as 2 ints, so
> setScrollOffsetsFromAnimation() should take a const IntSize&

Will change, the reasons I originally wanted to do this didn't end up being all that serious.

One other bad thing about this patch is that it doesn't provide smooth scrolling to Safari/Win, which probably wants to use a similar algorithm (as I think dhyatt alluded to on IRC when we first chatted).  Maybe instead of having this as ChromiumWin I should just do it for Windows in general.  Not sure that all Windows ports (e.g. Chromium) use the win/ code though...

[Peter Kasting]

Created attachment 45025 [details]
patch v3

Changes from the last patch:

* Addresses all mitz' comments
* Adds modification to project.pbxproj for Mac build
* Makes implementation be used by all PLATFORM(OS_WIN) targets
* Minor tweaks to animator implementation for better comments and ability to handle autoscroll delays other than 0.05

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45025 [details] without any errors.

Comment on attachment 45025 [details]
patch v3

> +    static ScrollAnimator* possiblyCreateWithClient(ScrollbarClient*);

I would like you to rename this method to “create”. We have a few static create() methods that may return 0, and we don’t call out that fact in the name. We also don’t specify the role of the parameter when it is implied by the type.

> +    virtual bool possiblyAnimateScroll(ScrollDirection, ScrollGranularity, ScrollSource, float step, float multipliedStep, float maxPos, bool& changedSomething) { return false; }

No surprise here: I would like to get rid of “possibly” (or at least move it to the end); since this is a ScrollAnimator method, animation is implied, so I would also get rid of “animate”. At the same time, I feel that the meanings of the return value and of changedSomething aren’t clear enough. I *think* changedSomething says whether the scroll offset will change (although it doesn’t actually change by the time the method returns), whereas the return value says whether the requested scroll is animatable.

It looks like anyone calling this method actually wants scrolling to occur (if needed), whether animated or not, i.e. if this method returns false, the caller is just going to do an immediate scroll. I like that the animator decides whether to animate, but I don’t think the caller should care about the decision. If the animator decides not to animate (which yours does in the scroll-by-document case), it should just do an immediate scroll (or infinite-velocity animated scroll).

So here’s the first draft of this API that I’d suggest:
    // Computes a scroll destination for the given parameters. Returns false if already at the destination. Otherwise, starts scrolling towards the destination
    // and returns true. Scrolling may be immediate or animated.
    virtual bool scroll(ScrollDirection, ScrollGranularity, ScrollSource, float step, float multipliedStep, float maxPos);

But I’d rather see a multiplier instead of a multipliedStep. It bothers me that ScrollDirection is only used to determine the axis (because the direction along the axis is given by the multiplier). We can use the ScrollbarOrientation enum to indicate the axis (maybe rename it to ScrollAxis someday). Finally, I don’t think the caller should precompute maxPos (why is it a float?) and pass it down here. We can add a ScrollbarClient method (virtual IntSize scrollSize() const;) to return the max offsets.

So here’s the second draft:
    // Computes a scroll destination for the given parameters. Returns false if already at the destination. Otherwise, starts scrolling towards the destination
    // and returns true. Scrolling may be immediate or animated.
    virtual bool scroll(ScrollbarOrientation, ScrollGranularity, ScrollSource, float step, int multiplier);

> +    virtual void setScrollPositionAndStopAnimation(ScrollbarOrientation, float) { }

I was expecting something like
    // Stops animated scrolling if it is in progress.
    virtual void stop();

I need to keep reading to understand how setScrollPositionAndStopAnimation() works. Who would want to stop animation along one axis and not the other? Why does the caller need to pass a float (0..1) position? I’ll keep reading and see.

I’m skipping ScrollAnimatorWin for now. Except
> +    // This is an animatible scroll.  Calculate the scroll delta.
s/animatible/animatable/

> +        ScrollbarPartOrMenu,  // "Scrollbar" alone clashes with the class name
But what does “OrMenu” mean?

> +void ScrollView::setScrollOffsetFromAnimation(const IntPoint& offset)
> +{
> +    if (m_horizontalScrollbar)
> +        m_horizontalScrollbar->setValue(offset.x(), true);
> +    if (m_verticalScrollbar)
> +        m_verticalScrollbar->setValue(offset.y(), true);
> +}
> +

We’ll want things without scrollbars to be able to scroll smoothly. Going through the scrollbars won’t do.

> -        scrollBy(IntSize(-deltaX, -deltaY));
> +
> +        // Should we fall back on scrollBy() if there is no scrollbar for a non-zero delta?
> +        if (deltaY && m_verticalScrollbar)
> +            m_verticalScrollbar->scroll(ScrollUp, ScrollByPixel, Wheel, deltaY);
> +        if (deltaY && m_horizontalScrollbar)
> +            m_horizontalScrollbar->scroll(ScrollLeft, ScrollByPixel, Wheel, deltaX);

This is introducing dependency on scrollbars.

> -    bool setValue(int);
> +    bool setValue(int, bool fromScrollAnimation);

So I don’t think this is the best distinction to make. Instead, you want to distinguish between the case where the change comes from the client (driven by smooth-scrolling animation, by drag-to-scroll auto animation, or whatever) and the other case, where the change needs to go to the client. Currently I see things like RenderLayer’s valueChanged() implementation having to pass a boolean to its scrollToOffset() method so that it will not call back into the scrollbars. Seems like we have an opportunity to fix this.

I don’t have time to finish this review right now, but I wanted to let you know what my thoughts were so far. The most troubling issue is that in our existing code, scrolling is driven by Scrollbar even when it is not initiated by a scrollbar, which makes it difficult to let ScrollAnimator drive scrolling.

[Peter Kasting]

(In reply to comment #31)
> (From update of attachment 45025 [details])
> > +    static ScrollAnimator* possiblyCreateWithClient(ScrollbarClient*);
> 
> I would like you to rename this method to “create”.

Will rename.

> > +    virtual bool possiblyAnimateScroll(ScrollDirection, ScrollGranularity, ScrollSource, float step, float multipliedStep, float maxPos, bool& changedSomething) { return false; }
> 
> It looks like anyone calling this method actually wants scrolling to occur (if
> needed), whether animated or not, i.e. if this method returns false, the caller
> is just going to do an immediate scroll. I like that the animator decides
> whether to animate, but I don’t think the caller should care about the
> decision. If the animator decides not to animate (which yours does in the
> scroll-by-document case), it should just do an immediate scroll (or
> infinite-velocity animated scroll).

OK.

> But I’d rather see a multiplier instead of a multipliedStep. It bothers me that
> ScrollDirection is only used to determine the axis (because the direction along
> the axis is given by the multiplier). We can use the ScrollbarOrientation enum
> to indicate the axis (maybe rename it to ScrollAxis someday).

OK.

> Finally, I don’t
> think the caller should precompute maxPos (why is it a float?) and pass it down
> here. We can add a ScrollbarClient method (virtual IntSize scrollSize() const;)
> to return the max offsets.

This means all ScrollbarClients will have to keep this value around, right?  Right now only the Scrollbar has it stored, in the form of the m_totalSize and m_visibleSize members, which get set by the setProportion() function.  Most uses of these could be removed and converted to calls to m_client->scrollSize(), removing the setProportion() function as well, but there's one use of m_totalSize to calculate the scroll step for ScrollByDocument.  Maybe that can also be changed to a call to scrollSize(), however, since the total scroll can't exceed that?...

> I was expecting something like
>     // Stops animated scrolling if it is in progress.
>     virtual void stop();
> 
> I need to keep reading to understand how setScrollPositionAndStopAnimation()
> works.

When something directs the scrollbar to do a non-animated scroll to a particular position, we need to stop the animation; but because the ScrollAnimator also has to track the current position in order to know how to animate when the next animated scroll appears, we also have to mirror that value over.

This is a mess.  If ScrollAnimator drove everything, and Scrollbar was much dumber, we could avoid this.  To accomplish this we'd need to modify the code that calls setValue() and have it call the ScrollAnimator, as well as fixing the issues you note below with the scrollbar driving the scroll.

> Who would want to stop animation along one axis and not the other?

I'm not sure.  I wanted to avoid thinking deeply about this case for the moment and just add animation to the current axis-independent interface.  I was hoping to tackle issues like this in a second pass, perhaps.

> Why
> does the caller need to pass a float (0..1) position?

It's not 0..1.  It's on the same scale that is used to pass (integer) scroll offsets to the ScrollbarClient.  It's just that the Scrollbar/ScrollAnimator keep a more precise representation to avoid building up errors due to rounding.

> > +    // This is an animatible scroll.  Calculate the scroll delta.
> s/animatible/animatable/

Are you sure?  I looked both up in a few dictionaries when writing this (because it bugged me too!) and couldn't find either, but Google had a lot more results for "animatible"...

> > +        ScrollbarPartOrMenu,  // "Scrollbar" alone clashes with the class name
> But what does “OrMenu” mean?

The scrollbar context menu.  In the Qt port and Windows native (although this isn't implemented in Chromium/Win, dunno if it's in Safari/Win), scrollbars have a context menu.  Look through the patch for the uses of this value.

The name sucks, but "ScrollbarPart" alone felt misleading...

> > +void ScrollView::setScrollOffsetFromAnimation(const IntPoint& offset)
> > +{
> > +    if (m_horizontalScrollbar)
> > +        m_horizontalScrollbar->setValue(offset.x(), true);
> > +    if (m_verticalScrollbar)
> > +        m_verticalScrollbar->setValue(offset.y(), true);
> > +}
> > +
> 
> We’ll want things without scrollbars to be able to scroll smoothly. Going
> through the scrollbars won’t do.

But right now, no scrolling happens except what goes through scrollbars.  This doesn't make the situation worse.

I agree with you about the ideal design goals.  I'm reluctant to try and rearchitect scrolling entirely in the same patch where I'm adding scroll animation.  Can we take things in stages?

> > -        scrollBy(IntSize(-deltaX, -deltaY));
> > +
> > +        // Should we fall back on scrollBy() if there is no scrollbar for a non-zero delta?
> > +        if (deltaY && m_verticalScrollbar)
> > +            m_verticalScrollbar->scroll(ScrollUp, ScrollByPixel, Wheel, deltaY);
> > +        if (deltaY && m_horizontalScrollbar)
> > +            m_horizontalScrollbar->scroll(ScrollLeft, ScrollByPixel, Wheel, deltaX);
> 
> This is introducing dependency on scrollbars.

Yes... but much of the scrolling capability in this file already depends on them.  If I don't do this, then wheel scrolls never reach the ScrollAnimator.

Again, this is entangled in how the Scrollbar drives most scrolling, except when it doesn't.  I admit to being a bit confused about how ScrollView works here, but fixing it in this patch, as I said above, scares me.  Maybe you understand the problem better and are more confident of the fix...

> > -    bool setValue(int);
> > +    bool setValue(int, bool fromScrollAnimation);
> 
> So I don’t think this is the best distinction to make. Instead, you want to
> distinguish between the case where the change comes from the client (driven by
> smooth-scrolling animation, by drag-to-scroll auto animation, or whatever) and
> the other case, where the change needs to go to the client. Currently I see
> things like RenderLayer’s valueChanged() implementation having to pass a
> boolean to its scrollToOffset() method so that it will not call back into the
> scrollbars. Seems like we have an opportunity to fix this.

If we're mucking with this, then it seems like the ideal end stage is that all changes come from the client, and none go to the client; that is, that the ScrollAnimator drives all scrolling and the Scrollbar merely displays the results on its thumb, and notifies the ScrollAnimator of any input events it sees directly.

> I don’t have time to finish this review right now, but I wanted to let you know
> what my thoughts were so far.

Thanks!

> The most troubling issue is that in our existing
> code, scrolling is driven by Scrollbar even when it is not initiated by a
> scrollbar, which makes it difficult to let ScrollAnimator drive scrolling.

Yes!  This was what I first alluded to at the end of comment 19 paragraph 2.

What a mess!

[Maciej Stachowiak]

Comment on attachment 45025 [details]
patch v3

Marking r- per prior comments, so this doesn't clog the queue. It's clear more review is needed but at least the already mentioned issues should be fixed in any case.

[Peter Kasting]

Created attachment 54744 [details]
patch v3.1

Resurrecting this.  First step: updated the previous patch to apply against current trunk.

[Peter Kasting]

Created attachment 65325 [details]
patch v3.2

Resurrecting the old patch again.

[Peter Kasting]

Created attachment 65459 [details]
patch v4.0

This patch implements some of the requests from comment 31.  Specifically, all the changes to the signatures of create() (formerly possiblyCreateWithClient()) and scroll() (formerly possiblyAnimateScroll()) have been made.  One of the requested changes, the addition of a ScrollbarClient::scrollSize() function, seems kind of like unnecessary extra plumbing to me at this stage, given that the old method with the scroll() caller providing a |maxPos| was easy to do since that function only has one caller.  However, maybe it is a step toward a better scrolling implementation.

Additionally, this makes all clients have a ScrollAnimator, which by default does no animation.  This makes the scrolling model slightly more consistent and thus less confusing.

I did not make major changes to ScrollView, such as trying to re-plumb scrolling to not go through the scrollbars.  If we want to do that, I think we need to redesign the entire flow for scrolling as a separate patch, rather than do it in this.  I don't think that should gate this landing.

[WebKit Review Bot]

Attachment 65459 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/ScrollView.cpp:278:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/ScrollView.cpp:279:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/ScrollAnimator.h:60:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/ScrollAnimator.h:61:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/ScrollAnimator.cpp:58:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
WebCore/rendering/RenderDataGrid.cpp:175:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/ScrollTypes.h:48:  One space before end of line comments  [whitespace/comments] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WebCore/rendering/RenderListBox.cpp:528:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/rendering/RenderLayer.cpp:1623:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/rendering/RenderLayer.cpp:1624:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/win/PopupMenuWin.cpp:669:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/public/WebScrollbar.h:65:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 12 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Created attachment 65461 [details]
patch v4.1

Fix style errors.  Thanks MSVC for re-indenting my lines like I didn't want.

[Early Warning System Bot]

Attachment 65461 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3787495

[Eric Seidel (no email)]

Attachment 65461 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3766620

[Peter Kasting]

Created attachment 65463 [details]
patch v4.2

Fix Qt and Mac compile errors.

[Eric Seidel (no email)]

Attachment 65463 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3793393

[Early Warning System Bot]

Attachment 65463 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3789535

[Peter Kasting]

Created attachment 65471 [details]
patch v4.3

Fix more Qt errors; guess at Mac errors (scrolled off EWS output :( )

[Eric Seidel (no email)]

Attachment 65471 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3753647

[Early Warning System Bot]

Attachment 65471 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3769692

[Peter Kasting]

Hmm.  I bet I can do this without plumbing scroll sources everywhere.  Re-reading my patch more closely (because I wrote the darn thing more than 8 months ago), I think the only real reason I need them is to determine whether the source is "scrollbar autorepeat timer" or "everything else".  I can do that with a small subset of the existing code.

However, there's an open question as to whether "programmatic" scrolls should ever be smooth-scrolled.  It's reasonable to say no, in which case I still do need some form of this plumbing so I can distinguish these two types of scrolling.  Or I need to somehow change these to call setValue() instead of scroll()...

[WebKit Review Bot]

Attachment 65471 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3809047

[WebKit Review Bot]

Attachment 65471 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3772657

[Peter Kasting]

Created attachment 65609 [details]
patch v5.0

After thinking about the math some more, I realized my current implementation doesn't actually need command sources at all.  So this patch removes that plumbing entirely.  Yay simplification!

[Eric Seidel (no email)]

Attachment 65609 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3770738

[Peter Kasting]

Created attachment 65613 [details]
patch v5.1

Fix Mac compile break.

[Eric Seidel (no email)]

Attachment 65613 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3746702

[Early Warning System Bot]

Attachment 65613 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3793426

[Peter Kasting]

Created attachment 65627 [details]
patch v5.2

I'm not sure what the Qt error is caused by.  This should fix the Mac error by only forward-declaring ScrollAnimator in ScrollbarClient.h, and moving functions that need the definition to a new ScrollbarClient.cpp file.

[Early Warning System Bot]

Attachment 65627 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3725700

[Peter Kasting]

Created attachment 65640 [details]
patch v5.3

OK, added changes to CMakeLists.txt and WebCore.pro, fingers crossed that I did it correctly.

[Peter Kasting]

Created attachment 66142 [details]
patch v5.4

Changes setValue() and setCurentPos() from taking a bool to taking an enum at hyatt's request in IRC.

[Dave Hyatt]

Comment on attachment 66142 [details]
patch v5.4

r=me

[Eric Seidel (no email)]

Attachment 66142 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3885200

[Peter Kasting]

Created attachment 66148 [details]
patch v5.5

Fix compile failure

[Early Warning System Bot]

Attachment 66148 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3907002

[Peter Kasting]

Created attachment 66150 [details]
patch v5.6

Fix other compile errors that I didn't catch because they weren't built for Chromium

[Peter Kasting]

Created attachment 66152 [details]
patch v5.7

Hmm, upload was incomplete

[Dave Hyatt]

Comment on attachment 66152 [details]
patch v5.7

r=me

[WebKit Commit Bot]

Comment on attachment 66152 [details]
patch v5.7

Rejecting patch 66152 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20893 test cases.
fast/events/continuous-platform-wheelevent-in-scrolling-div.html -> failed

Exiting early after 1 failures. 7160 tests run.
142.72s total testing time

7159 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

Full output: http://queues.webkit.org/results/3956028

[Eric Seidel (no email)]

Comment on attachment 66142 [details]
patch v5.4

Cleared David Hyatt's review+ from obsolete attachment 66142 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Peter Kasting]

Created attachment 66762 [details]
patch v5.8

Helps when you don't accidentally delete an important block of code when merging in the latest svn update...

This also fixes a bug where horizontal wheel events would be ignored.

[WebKit Commit Bot]

Comment on attachment 66762 [details]
patch v5.8

Rejecting patch 66762 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 66762, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
bkit.org/attachment.cgi?id=66762&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=32356&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 66762 from bug 32356.
Dave Hyatt found in /Users/eseidel/Projects/CommitQueue/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Peter Kasting]

Created attachment 66777 [details]
patch v5.9

For crying out loud.  "Dave" -> "David" for the commit bot.

[Eric Seidel (no email)]

FYI, you can use "webkit-patch land-safely" to automatically fill in the reviewer and then have it upload your current patch for commit.  Adam and I use that command when making small last-minute edits like these.  I guess I never think about the dave vs. david question because webkit-patch always fills in the reviewer for me these days.

[Eric Seidel (no email)]

Comment on attachment 66152 [details]
patch v5.7

Cleared David Hyatt's review+ from obsolete attachment 66152 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Peter Kasting]

Comment on attachment 66777 [details]
patch v5.9

Forgot to mark this as "patch", no wonder it didn't land

[WebKit Commit Bot]

Comment on attachment 66777 [details]
patch v5.9

Clearing flags on attachment: 66777

Committed r67001: <http://trac.webkit.org/changeset/67001>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67001 might have broken Qt Linux Release

[Darin Fisher (:fishd, Google)]

FYI:  I checked in a change to add a compile time option for this, and it is now disabled by default on all platforms.

See https://bugs.webkit.org/show_bug.cgi?id=45689

[Peter Kasting]

Thanks Darin!  (I asked Darin to do this for me as a few issues came up and I'm on vacation for a week.)