627651 - Improve the smooth scroll animation

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Attachment: v1

The current smooth scroll animation gets slower when the frame rate drops. That's bad. Instead, the duration of the animation should be fixed and the current position should be based on the current time.

This patch reimplements the animation using bezier splines for the timing function to get smooth acceleration and deceleration.

Faaborg, you seem to care a lot about smooth scrolling, so please make sure I don't regress anything here.

Tryserver builds are in http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-86febfd03a83/

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Sorry about the delay, I'm going to try to get to this review tomorrow.

Comment 2

[Markus Stange [:mstange]]

No rush, this really isn't a priority.

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

>the duration of the animation should be fixed and the
>current position should be based on the current time.

I feel like this was already filed and resolved, but looking into it, I think the (pretty old) bug 418684 might have been incorrectly duped to bug 418351, and you're correct that this is still an issue.

cc'ing dbaron since he was just commenting on how smooth scrolling is considerably slower on his Linux machine (want to test the tryserver builds :)

I tested on windows, but my computer was fast enough to avoid dropping any frames, and keep in time with the events coming in.  Moving on to test on an incredibly slow OS X machine.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 505727 [details] [diff] [review]
v1

yeah, wow.  I also tested out these try server builds and the currently nightly build on a slow OS X machine with smooth scrolling enabled, and the nytimes home page.  With the current nightly build I was able to get the application to scroll on its own for several seconds after receiving input, by first providing about 30 seconds of two finger scroll events.  The try server builds stopped scrolling almost immediately after I took my fingers off of the track pad.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 505727 [details] [diff] [review]
v1

+  return slope / (mDuration / oneSecond) * (aDestination - aStart);

I'd prefer (slope * (aDestination - aStart)) / (mDuration / oneSecond)

+  const TimeDuration oneSecond = mozilla::TimeDuration::FromSeconds(1);

drop "mozilla::"

+    mozilla::TimeStamp now = mozilla::TimeStamp::Now();

Add "using namespace mozilla;" after the #includes and drop mozilla:: here.

Comment 6

[Markus Stange [:mstange]]

Attachment: for checkin

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

>approval2.0? 

I'm very much in favor of approval, this patch has a significant impact on perceived performance on slower machines (although only for the set of users who happen to have turned on smooth scrolling).

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)
> >approval2.0? 
> 
> I'm very much in favor of approval, this patch has a significant impact on
> perceived performance on slower machines (although only for the set of users
> who happen to have turned on smooth scrolling).

I thought smooth scrolling is enabled by default on Windows nowadays.
But maybe that change was reverted.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

discussed but not done, details here: https://bugzilla.mozilla.org/show_bug.cgi?id=590022#c23

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Attachment: For checkin (unrotted)

The previous patch bitrotted. This one takes care of it. Overall, it makes a big difference on "heavy" pages on my laptop, especially when scrolling a long GMail message thread. The only issue I've seen is that things can be a bit jerky when the framerate changes. On the other hand, it certainly makes scrolling more responsive, and responsiveness and feel is one of the most user-visible areas of performance. It would be a real shame for this to miss 4.0.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 510817 [details] [diff] [review]
For checkin (unrotted)

This looks really nice, but it should land after Firefox 4 / Gecko 2 has branched.

Comment 13

[Markus Stange [:mstange]]

dbaron, is this something that could land in the birch repo?

Comment 14

[Leman Bennett [Omega]]

Shouldn't this be added to the post gecko 2.0 queue?
https://bugzilla.mozilla.org/show_bug.cgi?id=610267

Comment 15

[Markus Stange [:mstange]]

Good idea.

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3dbddbb05ae4