1507569 - Teach canScrollVertically about fullscreen and scrollable pages (to deal with dynamic toolbar issues)

Status: RESOLVED WONTFIX

(GeckoView :: Toolbar, enhancement, P1)

Description

[Emily Kager [:ekager]]

We are dealing with an issue with the collapsing toolbar in Focus (and FFTV and AC), where on a page meant to be fullscreen, you can still scroll the distance that the toolbar collapses. For example on maps.google.com etc. 

https://github.com/mozilla-mobile/focus-android/issues/3815

I have a WV workaround that uses (getWebView() as NestedWebView).canScrollVertically(1) to tell if we should remove/add the scroll flags once a site is loaded. 

WIP Workaround: https://github.com/ekager/focus-android/commit/09c0618918a0c2a3e022c6d89c62153a9c5c4767

Currently, canScrollVertically for GV does not return anything different for fullscreen sites vs scrollable sites. Would it be possible to make this respond to the ability of the page to scroll?

Comment 1

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

This really annoys me in Focus so I'll take this.

Comment 2

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

With WV, they use `View.canScrollVertically()` to determine this, but there isn't any notification of when this changes. We can do better with GV by adding a listener/delegate to the `GeckoView` class.

Comment 3

[Emily Kager [:ekager]]

That would be awesome!

Comment 4

[Chris Peterson [:cpeterson]]

P1 for Fenix

Comment 5

[Chris Peterson [:cpeterson]]

GV apps need this functionality to know when to show or hide their dynamic toolbars.

Comment 6

[Chris Peterson [:cpeterson]]

Unassigning James because we don't need to work on this issue until Fenix asks for it. The viewport issues might be less of a problem with Fenix's bottom toolbar compared to Fennec's top toolbar.

Comment 7

[Chris Peterson [:cpeterson]]

James, can this canScrollVertically work be delegated to Gecko's Layout team? Or is this bug just asking for a GV API?

Comment 8

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

(In reply to Chris Peterson [:cpeterson] from comment #7)

James, can this canScrollVertically work be delegated to Gecko's Layout team? Or is this bug just asking for a GV API?

I think APZ has all the info we need, just need to expose it in GV.

Comment 9

[Sean Voisen (:svoisen)]

I'm CC'ing Hiro in case there's anything we can help with on the layout or APZ side, but based on comment 8 it sounds like this is mostly a GV API issue.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I am not sure I am understanding exactly what we need in this bug, but it seems to me VisualViewport class knows all the info what we need (I believe the VisualViewport class should have it). And yeah, the work we need is mostly in GV.

Comment 11

[Chris Peterson [:cpeterson]]

Reference Browser issue https://github.com/mozilla-mobile/reference-browser/issues/464 might depend on this bug.

Comment 12

[Jonathan Almeida [:jonalmeida]]

Attachment: Screenshot_20190325-145828.jpeg

Wanted to add this as potential insight: Chrome has an interesting implementation for their bottom nav bar that is currently in their Canary channel which is worth checking out. They stick web content that usually anchors itself to the bottom of the page to be anchored on top of the toolbar. Reddit's website/app chooser, for example, then scrolls up/down but the rest of the content window doesn't change.

Here's a screenshot of what that looks like mid-scroll.

Comment 13

[Chris Peterson [:cpeterson]]

67=wontfix. Fenix MVP will use GeckoView 68, so we don't need to uplift this fix to 67 Beta.

Comment 14

[Chris Peterson [:cpeterson]]

Andreas will suggest to Vesta that Fenix MVP use a fixed bottom address bar. That would avoid a lot of these viewport scaling issues for MVP. We can revisit a hiding address bar post-MVP.

Comment 15

[Chris Peterson [:cpeterson]]

Andreas, have you asked Vesta about using a fixed bottom address bar in Fenix MVP? That would avoid a lot of these viewport scaling issues for MVP. We can revisit a hiding address bar post-MVP.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

The patch from bug 1516048 is working great for scrollable pages. This here would be the last piece to make the bottom toolbar work in all cases (as far as we know right now).

Comment 17

[Chris Peterson [:cpeterson]]

Resolving this bug as WONTFIX. James recommends Fenix use the DynamicToolbarAnimator API, which is what Fennec uses. canScrollVertically is not a complete solution because it doesn't notify the app when pages change scrollable state.

Comment 18

[Chris Peterson [:cpeterson]]

Moving toolbar bugs to the new GeckoView::Toolbar component.