Closed Bug 1376526 Opened 6 years ago Closed 6 years ago

APZ: Get scroll snapping working with async keyboard scrolling

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(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]
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?
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]
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.
Attachment #8882265 - Flags: review?(bugmail) → review+
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88a0ae57cbc
Perform scroll snapping with async keyboard scrolling. r=kats
https://hg.mozilla.org/mozilla-central/rev/f88a0ae57cbc
Status: NEW → RESOLVED
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.