1990215 - [composable toolbar] Surface scroll data from GeckoView

Status: RESOLVED FIXED

(Firefox for Android :: Toolbar, task, P2)

Description

[Petru-Mugurel Lingurar [:petru]]

In order to sync the dynamic toolbar movement with webpage scrolling we need to surface the GeckoSession.CompositorScrollDelegate data to Android-Components.

Comment 1

[Petru-Mugurel Lingurar [:petru]]

Prior similar work was done for supporting properly placing the reader mode content - bug 1820404 / 1535 GH PR which added the GeckoSession scroll data to ReaderState.

I would prefer to not go through our BrowserState but rather expose a StateFlow from the EngineView which already exposes scroll related data.
Advantages:

• avoid spurious updates when the page is scrolled. BrowserState is already a very big object updated and observed from multiple places so the scroll updates would just add more pressure.
• avoid delays from our MVI model which would have the scroll updates go through the entire list of BrowserStore middlewares + reducer to then be observed by the toolbar code.
  In my tests this would take 1ms from receiving the APZ callback to using this data where we need it vs ~4ms I observe for the scroll data we currently use through ReaderState.

Disadvantages:

• missing previous scroll data when going back to or restoring a tab (if we'd persist the scroll information together with other data from BrowserState)

 
@Botond Would you think it's possible to have APZ send the current scroll position whenever a new CompositorScrollDelegate is registered - probably through the same callback just to ensure we have the baseline without needing to persist this for each tab?

Comment 2

[Botond Ballo [:botond]]

(In reply to Petru-Mugurel Lingurar [:petru] from comment #1)

@Botond Would you think it's possible to have APZ send the current scroll position whenever a new CompositorScrollDelegate is registered - probably through the same callback just to ensure we have the baseline without needing to persist this for each tab?

That should be doable, yeah. I filed bug 1990617 about it.

Let me know to what extent this is blocking your work, and I will prioritize it accordingly. Thanks!

Comment 3

[Petru-Mugurel Lingurar [:petru]]

(In reply to Botond Ballo [:botond] from comment #2)

That should be doable, yeah. I filed bug 1990617 about it.
Let me know to what extent this is blocking your work, and I will prioritize it accordingly. Thanks!

That's great, thank you!
I can just account for that and move on but we'd need this before using the new behaviour in official releases - in a few weeks.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1990215 - Surface scroll data and scroll deltas from APZ to AC's EngineView r=#android-reviewers

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ec94943015fd
https://hg.mozilla.org/integration/autoland/rev/d718216397d5
Surface scroll data and scroll deltas from APZ to AC's EngineView r=android-reviewers,ohall,jonalmeida,skhan,m_kato

Comment 6

[Serban Stanca [:SerbanS]]

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