1376526 - APZ: Get scroll snapping working with async keyboard scrolling

Status: RESOLVED FIXED

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

Comment 1

[Ryan Hunt [:rhunt]]

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.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: key-snap.patch

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.

Comment 3

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

Comment on attachment 8882265 [details] [diff] [review]
key-snap.patch

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?

Comment 4

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

Attachment: 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...

Comment 5

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

Comment on attachment 8882265 [details] [diff] [review]
key-snap.patch

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88a0ae57cbc
Perform scroll snapping with async keyboard scrolling. r=kats

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f88a0ae57cbc