Add listener for content scroll events

RESOLVED FIXED in Firefox 56

Status

()

Firefox for Android
GeckoView
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

Trunk
Firefox 56
All
Android
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 months ago
GeckoView should expose content scroll events.
This could be used to to implement features like pull-to-refresh.
(Assignee)

Updated

4 months ago
Version: 51 Branch → Trunk
(Assignee)

Comment 1

4 months ago
Created attachment 8876273 [details] [diff] [review]
0001-Bug-1371796-1.0-Add-ScrollListener-for-content-scrol.patch

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

4 months ago
Created attachment 8876276 [details] [diff] [review]
0002-Bug-1371796-A.1-Enable-pull-to-refresh-in-geckoview_.patch

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-
See comment #3
Flags: needinfo?(rbarker)
(Assignee)

Comment 5

4 months 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

4 months ago
Created attachment 8881854 [details] [diff] [review]
0001-Bug-1371796-1.1-Add-ScrollListener-for-content-scrol.patch

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

4 months 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

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