1249040 - scrolling between points with `scroll-snap-type: proximity` very strange

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Arpad Borsos [:Swatinem]]

Attachment: scrollsnap.html

Sorry for the strange summary. The behavior is very hard to describe.

See the attached testcase. I have a large scrollable container with scroll-snap-type: proximity and two snap points on the top, and two on the bottom. The snap points work like expected, but scrolling in the middle that is outside the proximity radius feels very strange. It is sometimes slow and erratic, sometimes the scrolling gets stuck completely.

Comment 1

[Arpad Borsos [:Swatinem]]

Oh, forgot to mention that I’m on linux with a fairly recent nightly. I get the strange behavior both with touchpad scrolling as well as with a real scrollwheel.

Comment 2

[Arpad Borsos [:Swatinem]]

So I have done a little more testing. Surprisingly one click on the mousewheel scrolls me by roughly one grid row.

With scroll snapping disabled, the amount mousewheel clicks determine the distance. 3 mousewheel clicks scroll by roughly 3 grid rows. No matter how fast i cycle the mousewheel.

With scroll snapping enabled, if I cycle the mousewheel quite fast, it scrolls me less than the expected distance. So if I cycle the scrollwheel two times very quickly, it scrolls me by ~1.5 grid rows. The scroll distance does not really correspond to the amount I scrolled on the scrollwheel, but depends on the speed of the mousewheel as well.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I can reproduce the issue. It doesn't happen with APZ disabled; I'll take a look.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Regression from bug 1141884, when APZ is enabled. Basically what's happening is that the main thread is telling APZ where to smooth-scroll to, and when multiple wheel events come in quickly the main-thread code doesn't track the cumulative destination and each smooth-scroll basically clobbers the previous one. I can think of two possible approaches to fix this:
1) If the scroll isn't actually snapping to anything then just let APZ handle it instead of having the main thread process the scroll
2) Have the main thread track the destination so that it can deal with multiple scrolls in a row properly.

I'll see which one makes more sense.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

So the problem with (1) is that on the main thread we can't know if the scroll will snap to something without knowing the "real" scroll offset from the compositor. We can use the main-thread scroll offset which will be a little off and that might work well enough.

Still thinking about option (2).

There's also option (3): go whole-hog and ship over snap points to the compositor, so that we can do this entirely in the compositor (i.e. implement bug 1219296). That would be my preferred long-term solution so maybe it's better to just bite the bullet and do it now.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> There's also option (3): go whole-hog and ship over snap points to the
> compositor, so that we can do this entirely in the compositor (i.e.
> implement bug 1219296). That would be my preferred long-term solution so
> maybe it's better to just bite the bullet and do it now.

I'm going to try implementing bug 1219296.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Botond is looking at bug 1219296, so unassigning this for now.

Comment 8

[Botond Ballo [:botond]]

I have patches up in bug 1219296 which lay the groundwork for fixing this, by giving APZ the ability to do scroll snapping directly, without going through the main thread. 

The next step is to hook this up for APZ handling of wheel events, which is tracked by bug 1259296.

I'll use this bug to track anything further that may need to be done to fix this specific STR.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Sounds good. Most likely we'll be marking this wontfix for 47 as well since it's a lot of code to uplift. But we can wait on that decision until everything's in.

Comment 10

[Botond Ballo [:botond]]

I tested the attached testcase with the patches in bug 1259296. It still scrolls poorly, so some additional work will need to be done in this bug.

Comment 11

[Botond Ballo [:botond]]

I haven't debugged this yet, but I can make an educated guess as to what we'll need to do: instead of applying the wheel delta to the current scroll position, choosing a snap point near the result, and updating the smooth scroll animation that's in progress to target that snap point, we'll need to do apply the wheel delta to the target of the current smooth scroll animation, choose a snap pint near *that* result, and update the smooth scroll animation to target that.

Comment 12

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1249040 - Allow wheel scrolls to accumulate in the presence of scroll snapping. r=kats

Review commit: https://reviewboard.mozilla.org/r/45137/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45137/

Comment 13

[Botond Ballo [:botond]]

This patch implements the approach described in comment 11, and solves the problem for me locally.

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8739236 [details]
MozReview Request: Bug 1249040 - Allow wheel scrolls to accumulate in the presence of scroll snapping. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45137/diff/1-2/

Comment 15

[Botond Ballo [:botond]]

Cleaned it up a bit and posted it for review.

There's some code repetition between MaybeAdjustDeltaForScrollSnapping() and the code added to handle the SCROLLMODE_SMOOTH case; I didn't find an obvious way to remove it (suggestions welcome).

Comment 16

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2f3015fa7dc

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8739236 [details]
MozReview Request: Bug 1249040 - Allow wheel scrolls to accumulate in the presence of scroll snapping. r=kats

https://reviewboard.mozilla.org/r/45137/#review41727

The code duplication isn't horrible, but I think you might be able to make it a bit cleaner by changing MaybeAdjustDeltaForScrollSnapping so that it takes the scroll offset as another in/out parameter, and if it finds a snap point, it updates both the scroll offset and the delta. In the "instant" branch you can use the modified delta, and in the "smooth scroll" branch you can use the modified scroll offset. You might have to move to locking to the callsites to make it work better. Up to you if you want to try this or not.

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8739236 [details]
MozReview Request: Bug 1249040 - Allow wheel scrolls to accumulate in the presence of scroll snapping. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45137/diff/2-3/

Comment 19

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> The code duplication isn't horrible, but I think you might be able to make
> it a bit cleaner by changing MaybeAdjustDeltaForScrollSnapping so that it
> takes the scroll offset as another in/out parameter, and if it finds a snap
> point, it updates both the scroll offset and the delta. In the "instant"
> branch you can use the modified delta, and in the "smooth scroll" branch you
> can use the modified scroll offset. You might have to move to locking to the
> callsites to make it work better. Up to you if you want to try this or not.

Thanks for the suggestion! I made this change in the updated patch.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/269d0fff9187

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/269d0fff9187