1371796 - Add listener for content scroll events

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[Eugen Sawin [:esawin]]

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

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 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.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 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 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

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...

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

See comment #3

Comment 5

[Eugen Sawin [:esawin]]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 6

[Randall Barker [:rbarker]]

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).

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f85a213aac
[1.1] Add ScrollListener for content scroll events to GeckoView. r=snorp

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7f85a213aac