APZ: Get scroll snapping working with async keyboard scrolling

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rhunt, Assigned: rhunt)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

Assignee

Description

2 years ago
No description provided.
Assignee

Comment 1

2 years 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

2 years ago
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
Posted 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+

Comment 6

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f88a0ae57cbc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.