Closed Bug 1516048 Opened Last year Closed 3 months ago

Adjust bottom fixed position to visible viewport while toolbar is transitioning

Categories

(GeckoView :: General, defect)

All
Android
defect
Not set

Tracking

(firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

RESOLVED FIXED
mozilla68
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [geckoview:fenix:m4])

Attachments

(2 files)

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..
Assignee: nobody → eitan
OS: Linux → Android
Priority: -- → P1
Hardware: x86_64 → All

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

Flags: needinfo?(eitan)

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.

Assignee: eitan → nobody
Flags: needinfo?(eitan)

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

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.

Flags: needinfo?(eitan)
Flags: needinfo?(dbolter)
Flags: needinfo?(cpeterson)
Flags: needinfo?(s.kaspari)

Looping in Sean as well for awareness.

Flags: needinfo?(svoisen)

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

Other notable examples: instagram, pinterest, google maps

Other problem sites:

  • Reddit bottom nav bar
  • Quora bottom nav bar
  • flights.google.com: the airport search mode's Cancel/Apply buttons are hidden.
Flags: needinfo?(cpeterson)
Whiteboard: [geckoview:fenix:m4][layout:triage-discuss]
See Also: → 1536222, 1507569

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

Flags: needinfo?(dbolter)

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

Flags: needinfo?(svoisen) → needinfo?(hikezoe)

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

Flags: needinfo?(hikezoe)

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

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.

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

Whiteboard: [geckoview:fenix:m4][layout:triage-discuss] → [geckoview][layout:triage-discuss]
Flags: needinfo?(s.kaspari)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.

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

Whiteboard: [geckoview][layout:triage-discuss] → [geckoview]
Flags: needinfo?(eitan)
Whiteboard: [geckoview] → [geckoview:fenix:m4]
Assignee: nobody → eitan
Depends on: 1546139
Attachment #9033045 - Attachment description: Bug 1516048 - Introduce GeckoView.setVerticalClipping. → Bug 1516048 - Introduce GeckoView.setVerticalClipping. r?snorp
Attachment #9033045 - Attachment description: Bug 1516048 - Introduce GeckoView.setVerticalClipping. r?snorp → Bug 1516048 - Introduce GeckoView.setVerticalClipping. r?snorp!
Duplicate of this bug: 1536222
See Also: → 1314686
Pushed by eisaacson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71f09f4113cd
Introduce GeckoView.setVerticalClipping. r=snorp
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Blocks: 1583380

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:

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.

Flags: needinfo?(cpeterson)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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

Flags: needinfo?(cpeterson)
Priority: P1 → --

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.

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

Example sites in the screenshots attached:

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

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

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

(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?

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

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.

Status: REOPENED → RESOLVED
Closed: 9 months ago3 months ago
Resolution: --- → FIXED

(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:

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

(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:

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.

You need to log in before you can comment on or make changes to this bug.