Can't interact with adjusted bottom bar with new setVerticalClipping API
Categories
(GeckoView :: Toolbar, defect, P1)
Tracking
(firefox67 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 wontfix, firefox71 fixed)
People
(Reporter: ekager, Assigned: botond)
References
Details
(Whiteboard: [geckoview:m1909] [geckoview:m1910])
Attachments
(17 files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
In 1516048 setVerticalClipping was added which allows us to adjusts bottom aligned content with our moving toolbar. When the bottom content is adjusted, it cannot be interacted with.
STR:
- Visit site with bottom bar (ex sozone.de) on Fenix/RB
- Try to disimss pushed up cookie banner
Expected: Can dismiss
Actual: Cannot dismiss until toolbar is collapsed
See: https://github.com/mozilla-mobile/fenix/issues/2574
Can reproduce with Fenix and RB.
Comment 1•5 years ago
|
||
Adding [geckoview:fenix:m5]
whiteboard tag because we need to fix this dynamic toolbar bug for Fenix MVP.
Comment 2•5 years ago
|
||
Adding [geckoview:fenix:p2]
whiteboard tag and 68=wontfix. We don't need to fix this for Fenix MVP because MVP will use a fixed-position bottom address bar.
Comment 3•5 years ago
|
||
This is a dynamic address bar problem.
Fenix issue: https://github.com/mozilla-mobile/fenix/issues/2574
Comment 4•5 years ago
|
||
I'm editing a bunch of GeckoView bugs. If you'd like to filter all this bugmail, search and destroy emails containing this UUID:
e88a5094-0fc0-4b7c-b7c5-aef00a11dbc9
Comment 5•5 years ago
|
||
We need this API so Fenix can fix bottom nav bar issue https://github.com/mozilla-mobile/fenix/issues/2574.
Comment 6•5 years ago
|
||
snorp is going to talk to Jessie and then assign this bug to Botond or himself.
Comment 7•5 years ago
|
||
Tentatively assigning to Botond because snorp says he's going to look at this bottom toolbar bug soon.
This bug is still P1 because Fenix's dynamic bottom nav bar depends on this bug.
Assignee | ||
Comment 8•5 years ago
|
||
Note that to investigate this I'll need to build Fenix against a custom GeckoView. Are there instructions for how to do that?
Comment 9•5 years ago
|
||
Adding this bug to GV's September sprint.
(In reply to Botond Ballo [:botond] from comment #8)
Note that to investigate this I'll need to build Fenix against a custom GeckoView. Are there instructions for how to do that?
Comment 12•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #11)
How can I enable the dynamic toolbar in Fenix to reproduce this?
Sebastian? ^
Comment 13•5 years ago
|
||
Looks like this has been answered on Slack already:
https://github.com/csadilek/fenix/commit/b48310092ac7a56ab51930356879929fdb024444
Another option would be to just use Reference Browser where this is enabled by default for all installations.
Assignee | ||
Comment 14•5 years ago
|
||
(In reply to Emily Kager [:ekager] from comment #0)
In 1516048 setVerticalClipping was added which allows us to adjusts bottom aligned content with our moving toolbar. When the bottom content is adjusted, it cannot be interacted with.
STR:
- Visit site with bottom bar (ex sozone.de) on Fenix/RB
- Try to disimss pushed up cookie banner
Expected: Can dismiss
Actual: Cannot dismiss until toolbar is collapsed
I tested this in Reference Browser. The first thing I'm noticing is that the "pushing up" behaviour doesn't happen. The position of the bottom bar doesn't seem to be adjusted, rather it's partially obscured by the toolbar.
It's not clear yet whether that's because we've regressed the behaviour of the setVerticalClipping
API in the platform, or if R-B stopped using that API.
Assignee | ||
Comment 15•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #14)
It's not clear yet whether that's because we've regressed the behaviour of the
setVerticalClipping
API in the platform, or if R-B stopped using that API.
It's the latter: R-B is not calling setVerticalClipping
.
Sebastian, are you aware of a change related to this in R-B or Android Components since this bug was filed?
Assignee | ||
Comment 16•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #15)
It's the latter: R-B is not calling
setVerticalClipping
.
The same is true of Fenix with this patch applied to enable the dynamic toolbar.
Assignee | ||
Comment 17•5 years ago
|
||
Tangentially related, but on the platform side setVerticalClipping
will need a WebRender implementation as well; filed bug 1583380 for this.
Assignee | ||
Comment 18•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #15)
R-B is not calling
setVerticalClipping
.
Snorp suggested that I work around this locally by hacking in a call to setVerticalClipping
around here. I did so, and I can confirm that I can now reproduce this bug as originally reported.
I filed an A-C bug about Fenix and R-B not calling setVerticalClipping
.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 19•5 years ago
|
||
diagnosis |
(In reply to Botond Ballo [:botond] from comment #18)
Snorp suggested that I work around this locally by hacking in a call to
setVerticalClipping
around here. I did so, and I can confirm that I can now reproduce this bug as originally reported.
The problem here is that the implementation of setVerticalClipping
added in bug 1516048 is only adjusting the position of the bottom-fixed content for the purposes of rendering. We need to make a corresponding adjustment for purposes of hit testing.
Assignee | ||
Comment 20•5 years ago
|
||
This is not going to be a trivial fix.
In Fennec, the async transforms introduced by the dynamic toolbar were transient -- they would go away as soon as we repaint. As a result, we obeyed them during rendering so that dynamic toolbar transitions would be smooth, but we ignored them during hit testing because you were unlikely to try to interact with an element during the brief time when we have updated its position visually but have not repainted yet.
Fenix's approach to the dynamic toolbar, however, involves an async transform that's persistent as long as the toolbar is partially or fully on-screen. This means we have to obey it during hit testing, which will require some extra machinery. It will also require support in WebRender, like the rendering part.
Comment 21•5 years ago
|
||
Though I might be misunderstanding what the issue is here, what I can tell from the perspective of a web compat issue (bug 1515980) is, regardless of whether the toolbar is static or dynamic, GeckoView needs to set client height which is NOT including the toolbar height to GeckoSession.mHeight and notify the toolbar height to Gecko independently. So that in the case of the dynamic toolbar Gecko can use the toolbar height to resolve vh units. (Note that the height without toolbar is used for ICB, resolving %
units, etc.) With these we don't need to use setVirticalClipping and I suppose this issue is fixed. Note that I think we also need an API to tell whether the toolbar is static or dynamic from Gecko, or just adding the info into the toolbar height notification .
Assignee | ||
Comment 22•5 years ago
|
||
Yes, telling the main thread about the toolbar position and letting it position bottom-fixed items accordingly would be an alternative approach.
The current approach is to let the main thread position bottom-fixed items behind the toolbar, and have APZ fix up their positioning.
Assignee | ||
Comment 23•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #22)
Yes, telling the main thread about the toolbar position and letting it position bottom-fixed items accordingly would be an alternative approach.
It's worth noting that, even if we do this, we'll still need the setVerticalClipping
API. Otherwise, the position of bottom-fixed items would only be adjusted when we repaint, not on every composite (i.e. the movement wouldn't be smooth).
Assignee | ||
Comment 24•5 years ago
|
||
One of the things I realized as I was thinking about how to fix this, is that APZ actually has two codepaths where it applies transforms for hit-testing purposes (three if you count WebRender). I wrote some notes about this.
Updating the first codepath (ComputeTransformForNode()
) to account for the fixed margins adjustment should be straightforward, at least for non-WebRender.
The second codepath (GetScreenToApzcTransform()
+ GetApzcToGeckoTransform()
) is the trickier one. A general solution would involve updating the transform model to allow non-APZC nodes in the hit testing tree to have async transforms. This would be a fairly invasive change, and it would upset several assumptions:
- The assumption that the transform that APZ applies to convert an input event form screen coordinates to Gecko coordinates is a function of the event's target APZC only. With the above change, it would be a function of the target node. (In particular, in the common case of a scrollable page with a bottom-fixed item, it would depend on whether we hit the fixed item's node.)
- The assumption that transforms between APZCs ("ancestor transforms") only change during hit testing tree updates. We would no longer be able to cache the ancestor transforms the way we do today.
To avoid having to make these invasive changes, I'm thinking of a more targeted approach, that relies on the fact the transform representing the dynamic toolbar's movement is basically at the top level, and can be applied after the entire screen-to-gecko transform matrix. This way, we can apply it as a translation directly in ReceiveInputEvent
, and avoid having to modify the screen-to-gecko transform itself.
Comment 25•5 years ago
|
||
Tracking for GV's October sprint
Updated•5 years ago
|
Assignee | ||
Comment 26•5 years ago
|
||
Some WIP patches for this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7263aad80329da65a4d34d811bd37753d9f530ce
Updated•5 years ago
|
Assignee | ||
Comment 27•5 years ago
|
||
It's still only used on Android, but keeping it and all code that interacts
with it inside an #ifdef is too much trouble for a tiny amount of space
saving.
Assignee | ||
Comment 37•5 years ago
|
||
This is a relic from earlier times when GetAPZCAtPoint() was recursive.
Depends on D48379
Assignee | ||
Comment 41•5 years ago
|
||
Populating them is hooked up for non-WebRender, with a comment outlining
possible implementation strategies for WebRender.
Depends on D48383
Assignee | ||
Comment 43•5 years ago
|
||
Assignee | ||
Comment 44•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #20)
Fenix's approach to the dynamic toolbar, however, involves an async transform that's persistent as long as the toolbar is partially or fully on-screen. This means we have to obey it during hit testing, which will require some extra machinery. It will also require support in WebRender, like the rendering part.
Filed bug 1586843 for the WebRender part.
Comment 45•5 years ago
|
||
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7bbff3ca9c53 Define AsyncCompositionManager::mFixedLayerMargins and its getter/setter on all platforms. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/f8a125f2969e Factor out a helper CompositorBridgeParent::SetFixedLayerMargins(). r=tnikkel https://hg.mozilla.org/integration/autoland/rev/daefc4088b5b Store fixed layer margins in APZCTreeManager. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/6310462ffafb Fix include-what-you-use violations in LayerAttributes.h. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/0674fa5af30f Define bitwise operators for SideBits. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/a8a4f618d3d3 Use SideBits rather than int32_t to represent fixed-position sides in the Layers API. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/67c4bc35fc74 Propagate the fixed-position sides to the hit testing tree. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/00824be6572b Factor out a helper AsyncCompositionManager::ComputeFixedMarginsOffset(). r=tnikkel https://hg.mozilla.org/integration/autoland/rev/597c42949166 Make APZCTreeManager::GetTargetAPZC(layersId, scrollId) const. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/7c1b57fdf9e2 Add a helper APZCTreeManager::IsFixedToRootContent(node). r=tnikkel https://hg.mozilla.org/integration/autoland/rev/ff8e1fa23dea Handle fixed layer margins in ComputeTransformForNode(). r=tnikkel https://hg.mozilla.org/integration/autoland/rev/50fb72cc4a73 Remove the 'aNode' parameter of GetAPZCAtPoint(). r=tnikkel https://hg.mozilla.org/integration/autoland/rev/5482a581ab19 Remove the unused function APZCTreeManager::HitTestAPZC(). r=tnikkel https://hg.mozilla.org/integration/autoland/rev/5b65905fba5c Allow HitTestingTreeNodeAutoLock to be moved. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/824e732b442a Introduce a HitTestResult structure for grouping the results of APZ hit testing. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/3c3c35952f7b Include the fixed-position sides in HitTestResult. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/6506806b3bf8 Handle fixed layer margins in ReceiveInputEvent. r=tnikkel
Comment 46•5 years ago
|
||
Though I might be still misunderstanding what the problem here is, this problem might be fixed by making VisualViewport behavior match Chrome's. On Chrome, VisualViewport height is dynamically changed by the dynamic toolbar transition (and VisualViewport.onresize is repeatedly called during the transition). So to me, once after we made it happen we no longer need setVerticalClipping API?
(Note that on current GeckoView with CoordinatorLayout VisualViewport height is including the toolbar height regardless of whether it's dynamic or static)
Comment 47•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7bbff3ca9c53
https://hg.mozilla.org/mozilla-central/rev/f8a125f2969e
https://hg.mozilla.org/mozilla-central/rev/daefc4088b5b
https://hg.mozilla.org/mozilla-central/rev/6310462ffafb
https://hg.mozilla.org/mozilla-central/rev/0674fa5af30f
https://hg.mozilla.org/mozilla-central/rev/a8a4f618d3d3
https://hg.mozilla.org/mozilla-central/rev/67c4bc35fc74
https://hg.mozilla.org/mozilla-central/rev/00824be6572b
https://hg.mozilla.org/mozilla-central/rev/597c42949166
https://hg.mozilla.org/mozilla-central/rev/7c1b57fdf9e2
https://hg.mozilla.org/mozilla-central/rev/ff8e1fa23dea
https://hg.mozilla.org/mozilla-central/rev/50fb72cc4a73
https://hg.mozilla.org/mozilla-central/rev/5482a581ab19
https://hg.mozilla.org/mozilla-central/rev/5b65905fba5c
https://hg.mozilla.org/mozilla-central/rev/824e732b442a
https://hg.mozilla.org/mozilla-central/rev/3c3c35952f7b
https://hg.mozilla.org/mozilla-central/rev/6506806b3bf8
Assignee | ||
Comment 48•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
Though I might be still misunderstanding what the problem here is, this problem might be fixed by making VisualViewport behavior match Chrome's. On Chrome, VisualViewport height is dynamically changed by the dynamic toolbar transition (and VisualViewport.onresize is repeatedly called during the transition). So to me, once after we made it happen we no longer need setVerticalClipping API?
I'm not sure what the relevance of VisualViewport
is here. Fixed-position elements are attached to the layout viewport, not the visual viewport.
Comment 49•5 years ago
|
||
Right, but the problem doesn't happen in the case where the position:fixed elements are outside of the visual viewport, right? A problem I can see is that (at least from the perspective of a web compat issue) with the current dynamic toolbar implementation visual viewport height is including the toolbar height (I am saying scale=1 case). Oh, hmm. You are right in such case the layout viewport is also including the toolbar height, bug 1586144 should fix the problem, I think (I was assuming bug 1586986 should fix it).
Assignee | ||
Comment 50•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #49)
in such case the layout viewport is also including the toolbar height, bug 1586144 should fix the problem, I think
What will be the layout viewport height after bug 1586144:
- Always including the toolbar height?
- Always excluding the toolbar height?
- Changes depending on whether the toolbar is hidden or shown?
Comment 51•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #50)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #49)
in such case the layout viewport is also including the toolbar height, bug 1586144 should fix the problem, I think
What will be the layout viewport height after bug 1586144:
- Changes depending on whether the toolbar is hidden or shown?
Chrome behavior looks this. At least for the case of position:fixed; bottom: 0
elements. But it's hard to tell without debugging Chrome whether the layout viewport height is static or dynamic. What I am currently supposing is that the layout viewport height is static just like ICB, but position:fixed elements stick to the dynamic toolbar transitions.
Comment 52•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #51)
What I am currently supposing is that the layout viewport height is static just like ICB,
Well, the layout viewport height is static as if the toolbar is there, i.e. excluding the toolbar height.
Assignee | ||
Comment 53•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #51)
What I am currently supposing is that the layout viewport height is static just like ICB, but position:fixed elements stick to the dynamic toolbar transitions.
That would be my guess too.
Our current mechanism for making position:fixed elements stick to the bottom during dynamic toolbar transitions is the setVerticalClipping
API.
Even if we add a mechanism for changing the position of fixed elements in the main thread, that will only be updated during a paint, and we'll need the setVerticalClipping
API to cover the cases where we get a composite (that changes the dynamic toolbar position relative to the last paint) without a paint.
Assignee | ||
Comment 54•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #52)
Well, the layout viewport height is static as if the toolbar is there, i.e. excluding the toolbar height.
If that's what Chrome does, that's interesting. I thought they had a constraint where the layout viewport had to be at least as large as the visual viewport.
Comment 55•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #53)
Even if we add a mechanism for changing the position of fixed elements in the main thread, that will only be updated during a paint, and we'll need the
setVerticalClipping
API to cover the cases where we get a composite (that changes the dynamic toolbar position relative to the last paint) without a paint.
That's what I am concerned right now to fix bug 1586986. To fix bug 1586986, we also need to update visual viewport size in response to setVerticalClipping
calls on the main thread, which results a reflow for position:fixed elements on the main thread. We probably need a machinery to bypass it.
Assignee | ||
Comment 56•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #55)
That's what I am concerned right now to fix bug 1586986. To fix bug 1586986, we also need to update visual viewport size in response to
setVerticalClipping
calls on the main thread, which results a reflow for position:fixed elements on the main thread. We probably need a machinery to bypass it.
Ok, I understand now. We probably want to make sure setVerticalClipping
does not affect the layout viewport (and that the placement of position:fixed elements is only affected by the layout viewport).
Comment 57•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #54)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #52)
Well, the layout viewport height is static as if the toolbar is there, i.e. excluding the toolbar height.
If that's what Chrome does, that's interesting. I thought they had a constraint where the layout viewport had to be at least as large as the visual viewport.
Indeed, I may be wrong here. If Chrome's layout view height matches window.innerHeight (I am also saying scale=1 case),
- the layout viewport height is excluding the toolbar height in the case where the toolbar is completely shown
- the layout viewport height is excluding the toolbar height as if the toolbar is completely shown in the case of the toolbar transition
- the layout viewport height is whole visible area's height in the case where the toolbar is hidden
At least for window.innerHeight this is what Chrome does, so it looks the visual viewport height exeeds the layout viewport height during the toolbar transition, but I haven't actually checked.
Updated•2 years ago
|
Description
•