APZ: Get scroll snapping working with async keyboard scrolling

RESOLVED FIXED in Firefox 56

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: rhunt, Assigned: rhunt)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
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.
(Assignee)

Comment 2

a year ago
Created attachment 8882265 [details] [diff] [review]
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.
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?
Priority: -- → P3
Created attachment 8882299 [details]
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+

Comment 6

a year ago
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88a0ae57cbc
Perform scroll snapping with async keyboard scrolling. r=kats

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f88a0ae57cbc
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.