Add listener for content scroll events

RESOLVED FIXED in Firefox 56

Status

enhancement
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

Trunk
mozilla56
All
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

2 years ago
GeckoView should expose content scroll events.
This could be used to to implement features like pull-to-refresh.
Assignee

Updated

2 years ago
Version: 51 Branch → Trunk
Assignee

Comment 1

2 years ago
Add ScrollListener interface with the onScrollChanged callback.

We might want to override getScroll{X,Y}, scrollTo, and scrollBy.
For this it would be better to keep scroll state directly in GeckoView.
Attachment #8876273 - Flags: review?(snorp)
Assignee

Comment 2

2 years ago
This is an example implementation based on SwipeRefreshLayout to enable pull-to-refresh in geckoview_example.
Comment on attachment 8876273 [details] [diff] [review]
0001-Bug-1371796-1.0-Add-ScrollListener-for-content-scrol.patch

Review of attachment 8876273 [details] [diff] [review]:
-----------------------------------------------------------------

I think we really might need to hook up to APZ for this. The content scroll change is delayed and is basically meaningless because of that. This might be good enough for pull-to-refresh, but likely still not great for that either.

Also, we should only hook up to the scroll event if we actually have a java listener.

Randall sorta nuked most of the stuff we could use to implement something like this against APZ. Maybe we could put some stuff back? Compositor process would make this a pain...
Attachment #8876273 - Flags: review?(snorp) → review-
Assignee

Comment 5

2 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3)
> Also, we should only hook up to the scroll event if we actually have a java
> listener.

This holds for most events, I would like to solve this generally for all GeckoView listeners in a separate bug.
We currently get the overscroll distance for the root in NativePanZoomController.java. The ImmutableViewportMetrics.java contains the scroll offset but is only updated when the page is in a steady state. I'm not sure the content scroll values are *that* delayed. The only other option would be send the scroll position every time they change in APZ. One of the reasons we moved the toolbar into the compositor was so the frame metrics would not need to be sent back to the UI thread after every update (at frame rate).
Flags: needinfo?(rbarker)
Comment on attachment 8876273 [details] [diff] [review]
0001-Bug-1371796-1.0-Add-ScrollListener-for-content-scrol.patch

Review of attachment 8876273 [details] [diff] [review]:
-----------------------------------------------------------------

Talked some more about this and it seems ok. I would like the scrolling bits separated into another content script, though, so we can enable it on-demand.
Attachment #8876273 - Flags: review- → review+
Assignee

Comment 8

2 years ago
Addressed comment: added a dedicated scroll frame script.

Module registration handling will be fixed in bug 1372681.
Attachment #8876273 - Attachment is obsolete: true
Attachment #8881854 - Flags: review+

Comment 9

2 years ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f85a213aac
[1.1] Add ScrollListener for content scroll events to GeckoView. r=snorp

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7f85a213aac
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

6 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 56 → mozilla56
You need to log in before you can comment on or make changes to this bug.