Closed Bug 1249040 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

47 Branch
Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- unaffected
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: Swatinem, Assigned: botond)

References

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(2 files)

Attached file 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.
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.
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.
I can reproduce the issue. It doesn't happen with APZ disabled; I'll take a look.
Blocks: apz-desktop
Keywords: correctness
OS: Unspecified → Linux
Whiteboard: [gfx-noted]
Version: unspecified → 47 Branch
Assignee: nobody → bugmail.mozilla
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.
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.
Depends on: 1219296
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Botond is looking at bug 1219296, so unassigning this for now.
Assignee: bugmail.mozilla → nobody
Depends on: 1259296
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.
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.
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.
Assignee: nobody → 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.
This patch implements the approach described in comment 11, and solves the problem for me locally.
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/
Attachment #8739236 - Flags: review?(bugmail.mozilla)
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 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.
Attachment #8739236 - Flags: review?(bugmail.mozilla) → review+
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/
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
https://hg.mozilla.org/mozilla-central/rev/269d0fff9187
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.