1516048 - Adjust bottom fixed position to visible viewport while toolbar is transitioning

Status: RESOLVED FIXED

(GeckoView :: Toolbar, defect)

Description

[Eitan Isaacson [:eeejay]]

In GeckoView applications like Focus or android-components, the view is clipped offscreen withing a nested scrollable container. GV needs an API to tell it how much of its view is obscured, and in return adjust the visible viewport size.

Aside from viewport resizing when the toolbar is completely showing/hidden, there needs to be a transitional state where the bottom-fixed elements "stick" to the bottom.

The WIP I will post here seems to work, even if the scroll is not in sync with the compositor like in Fennec. Tested in a Pixel, resource starved devices might show it stutter? I don't know. The public GeckoView API is also questionable.

Putting this here so I don't lose it in 2019..

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1516048 - Introduce GeckoView.setVerticalClipping. r?snorp!

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Eitan what's next for this bug, do you want to request review?

Comment 3

[Eitan Isaacson [:eeejay]]

I'm going to unassign myself for now so other folks can jump in on it. I may have time later to look into it again.

Comment 4

[Eitan Isaacson [:eeejay]]

Some further deets about this work. Depending on time, I might pick it up again. But I'm first attending to my team's priorities.

1. I think the approach I took in this bug is good for the transitional state when the toolbar is half visible. Besides this, I think we need to have a good implementation of an initial container block (ICB) vs. viewport (bug 1515980). Essentially, there would an initial containing block (ICB) that should be the viewport size minus the toolbar height. CSS height: 100% would match this. Then there would be the dynamic viewport height that grows when the toolbar goes away. CSS height: 100vh would match this.
2. We need to disable the toolbar from hiding when dealing with fullscreen content (ie. body height of less than 100%).
3. We need to design a public GV API that does all those three things (transitional state, ICB vs viewport, and tell a-c that content is not scrollable). This wip has something that answers the first two, that may be good enough for now.

This could probably be tackled piecemeal, with this bug getting resolved first. But since I am not very familiar with layout code I don't know whats best.

Here are some explainers on ICV vs viewport:

1. https://developers.google.com/web/updates/2016/12/url-bar-resizing
2. https://github.com/bokand/URLBarSizing

Comment 5

[vzare]

Thanks for investigating Eitan. I spoke to David about prioritizing this bug as this issue is a potential release blocker for Fenix.

Chris, sounds like fixing this bug is the first step, and the rest of the work also need to be filed and tracked and prioritized.

Sebastian, I am copying you here in case Eitan has questions about the layout code.

Comment 6

[Andreas Bovens [:abovens]]

Looping in Sean as well for awareness.

Comment 7

[Andreas Bovens [:abovens]]

This problem also appears to affect https://www.digg.com/ The bottom navigation bar appears to be hidden.

Comment 8

[vzare]

Other notable examples: instagram, pinterest, google maps

Comment 9

[Chris Peterson [:cpeterson]]

Other problem sites:

• Reddit bottom nav bar
• Quora bottom nav bar
• flights.google.com: the airport search mode's Cancel/Apply buttons are hidden.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Removing my NI. Let me know if this ends up needing GV engineering.

Comment 11

[Sean Voisen (:svoisen)]

@hiro: Can you take some time to investigate this perhaps this week or next and give some details on what work is required from our side – and approximate scope? Per comment 5, it's a potential blocker for Fenix. Looks like it might be dependent on bug 1515980.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

IIUC what the problem in this bug is, this bug doesn't need to depend on bug 1515980.
It seems to me that bug 1515980 is for providing a way to notify the both of sizes of viewport-ish (the one is the size when the urlbar is hidden, the other is the size when the urlbar is shown, we might just need the urlbar size though) and Gecko uses the size for layout.

(In reply to Eitan Isaacson [:eeejay] from comment #4)

1. I think the approach I took in this bug is good for the transitional state when the toolbar is half visible. Besides this, I think we need to have a good implementation of an initial container block (ICB) vs. viewport (bug 1515980). Essentially, there would an initial containing block (ICB) that should be the viewport size minus the toolbar height. CSS height: 100% would match this. Then there would be the dynamic viewport height that grows when the toolbar goes away. CSS height: 100vh would match this.

Correction: 100vh is not changed at all, it's always the size without the utlbar. i.e the size as if the urlbar is hidden.

As for this bug, we need a way to notify the transitioning height of the urlbar to Gecko which is exactly what Eitan did in attachment 9033045 [details]. (I think the API should also take top offset value because I believe toolbar's position dependns on applications). Anyways, it's more related to APZ stuff, not layout.

What the most challending task here to me is that how we can synchronize APZ and the urlbar transition (I assume the urlbar is rendered by application).

CCing kats, I believe he is the best person to ask (And I guess he has already some ideas since bug 1498699).

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)

CCing kats, I believe he is the best person to ask (And I guess he has already some ideas since bug 1498699).

I don't think bug 1498699 is relevant here. This bug basically sounds like yet another dynamic toolbar implementation. Randall wrote the last one and can probably answer stuff here too. We can probably reuse at least some of the code/plumbing we added for his implementation.

In particular I think you'll want to call AsyncCompositionManager::SetFixedLayerMargins with the bottom value set to the visible height of the toolbar. That will cause things like position-fixed items and scrollbars to get scrunched upwards to not overlap/go under the toolbar.

It's not really clear to me from the discussion here exactly what the desired behaviour is but if that can be clarified I can try to help more. I suppose I should also install Fenix and see what the behaviour looks like now...

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)

CCing kats, I believe he is the best person to ask (And I guess he has already some ideas since bug 1498699).

I don't think bug 1498699 is relevant here. This bug basically sounds like yet another dynamic toolbar implementation.

That's what I meant because you left a comment that 'GeckoView-based products won't be using it (at least for now)' in that bug. I think now GeckoView wants their own dynamic toolbar machinery.

Comment 15

[Chris Peterson [:cpeterson]]

Resetting [geckoview] whiteboard tag so GeckoView triage will review this bug's recent discussion about dynamic toolbars.

Comment 16

[Chris Peterson [:cpeterson]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)

It's not really clear to me from the discussion here exactly what the desired behaviour is but if that can be clarified I can try to help more. I suppose I should also install Fenix and see what the behaviour looks like now...

I have asked Fenix Product and UX people for input on the desired behavior.

Comment 17

[Sean Voisen (:svoisen)]

Based on Hiro's input from comment 12 and comment 13, sounds like we don't need further layout team discussion for now.

Comment 19

[Chris Peterson [:cpeterson]]

The corresponding AC issue is mozilla-mobile/android-components#2675.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71f09f4113cd
Introduce GeckoView.setVerticalClipping. r=snorp

Comment 21

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/71f09f4113cd

Comment 22

[Jonathan Almeida [:jonalmeida]]

Attachment: broken sites after fix.png

While testing out the changes :botond had implemented for the dynamic toolbar, I noticed that this issue isn't really fixed in my testing on a lot of sites.

Example sites in the screenshots attached:

• https://bokand.github.io/demo/urlbarsize.html (Chrome test site)
• https://reddit.com (app or web banner)
• https://theguardian.com (privacy banner)
• https://flights.google.com (airport searching cuts out search bar on top)

Re-opening to put this back on the GV board.

:cpeterson, we're not landing the dynamic toolbar in Fenix yet since these are quire common sites that we would get reports on.

Comment 23

[Chris Peterson [:cpeterson]]

:cpeterson, we're not landing the dynamic toolbar in Fenix yet since these are quire common sites that we would get reports on.

I will send this bug back to GV triage. We might want to file a new bug for these specific site issues.

Comment 24

[Jonathan Almeida [:jonalmeida]]

In comment 4, the same demo site was used for testing but it doesn't seem to be working on it, so I'm not sure if this would be regarded as a new issue.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jonathan Almeida [:jonalmeida] from comment #22)

Example sites in the screenshots attached:

• https://bokand.github.io/demo/urlbarsize.html (Chrome test site)

This one hasn't yet been fixed. Bug 1586144 will fix some of problems there, at least the initial rendering result should be the same as Chrome once it landed (we also need to change android-coments though).

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jonathan Almeida [:jonalmeida] from comment #22)

• https://theguardian.com (privacy banner)

The banner looks position:sticky, it's worth filing a new bug for this case.

Comment 27

[Jonathan Almeida [:jonalmeida]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)

The banner looks position:sticky, it's worth filing a new bug for this case.

I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1594451 for this one. Does that mean we should close this? Is is the same case for all the other sites too?

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jonathan Almeida [:jonalmeida] from comment #27)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)

The banner looks position:sticky, it's worth filing a new bug for this case.

I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1594451 for this one. Does that mean we should close this? Is is the same case for all the other sites too?

Thanks for filing that bug. Yes, I think we can close this bug. I don't see any issues on reddit.com and flights.google.com with the fix for bug 1586144. Though I haven't checked the sites with a bare reference-browser, it hopefully will be fixed by the same bug.

Comment 29

[Chris Peterson [:cpeterson]]

Thanks for filing that bug. Yes, I think we can close this bug. I don't see any issues on reddit.com and flights.google.com with the fix for bug 1586144.

Thanks. I'll close this bug again and we can track the new work in bug 1586144.

Comment 30

[Botond Ballo [:botond]]

(In reply to Jonathan Almeida [:jonalmeida] from comment #22)

Created attachment 9106198 [details]
broken sites after fix.png

While testing out the changes :botond had implemented for the dynamic toolbar, I noticed that this issue isn't really fixed in my testing on a lot of sites.

Example sites in the screenshots attached:

• https://bokand.github.io/demo/urlbarsize.html (Chrome test site)

What is the actual issue on this test page? It's not clear to me.

Comment 31

[Jonathan Almeida [:jonalmeida]]

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

(In reply to Jonathan Almeida [:jonalmeida] from comment #22)

Created attachment 9106198 [details]
broken sites after fix.png

While testing out the changes :botond had implemented for the dynamic toolbar, I noticed that this issue isn't really fixed in my testing on a lot of sites.

Example sites in the screenshots attached:

• https://bokand.github.io/demo/urlbarsize.html (Chrome test site)

What is the actual issue on this test page? It's not clear to me.

Only the bottom of the 2nd and 4th bars should both be seen.

Comment 32

[Chris Peterson [:cpeterson]]

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