Closed Bug 1376526 Opened 6 years ago Closed 6 years ago

APZ: Get scroll snapping working with async keyboard scrolling


(Core :: Panning and Zooming, enhancement, P3)




Tracking Status
firefox56 --- fixed


(Reporter: rhunt, Assigned: rhunt)



(Whiteboard: [gfx-noted])


(2 files)

      No description provided.
Whoops, forgot to add a description.

Currently, we disable async keyboard scrolling for scroll frames that have scroll snap points because the default async scrolling behavior doesn't match the sync keyboard scroll snapping behavior. We should fix this.
Attached patch key-snap.patchSplinter Review
I'm not sure why this didn't work the first time I tried this.

I've ran this on a sample page I have and it seems to work. I haven't done a try run with it.
Attachment #8882265 - Flags: review?(bugmail)
Comment on attachment 8882265 [details] [diff] [review]

Review of attachment 8882265 [details] [diff] [review]:

I feel like this will probably work in some cases with snap points that are relatively close together, but once they are far apart, I don't see how it would know to go all the way to the next snap point. Wouldn't it just move the scroll offset a small amount and then find the one it just left as the "nearest" snap point?
Attached file test page
For the record here's a test page that does horizontal scroll snapping. I would very surprised if the patch produces correct behaviour with APZ keyboard scroll snapping on this page. If it does we should figure out how it's doing that...
Attachment #8882299 - Attachment mime type: text/plain → text/html
Comment on attachment 8882265 [details] [diff] [review]

Review of attachment 8882265 [details] [diff] [review]:

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +4283,5 @@
> +    CSSPoint& aDestination)
> +{
> +  ReentrantMonitorAutoEnter lock(mMonitor);
> +  if (Maybe<CSSPoint> snapPoint = FindSnapPointNear(aDestination, aScrollUnit)) {
> +    aDestination = *snapPoint;

Not sure that this is worth a separate function, you could just inline it.
Attachment #8882265 - Flags: review?(bugmail) → review+
Pushed by
Perform scroll snapping with async keyboard scrolling. r=kats
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.