Basic support for APZ zooming with containerless scrolling
Categories
(Core :: Panning and Zooming, enhancement, P3)
Tracking
()
People
(Reporter: mstange, Assigned: botond)
References
Details
(Whiteboard: [gfx-noted][geckoview:p3])
Attachments
(9 files, 6 obsolete files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
Currently we only support APZ zooming with containerful scrolling. We'd like to get rid of containerful scrolling at some point, so we should figure out how to do zooming with containerless scrolling. I have some ideas that I'd like to try out in this bug.
Assignee | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 6•6 years ago
|
||
Matt Woodrow says this bug will add the features needed by mobile clients to desktop containerless scrolling so that mobile can switch to desktop's containerless scrolling implementation. Botond says he plans to work on containerless scrolling after Android fling physics.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 7•6 years ago
|
||
I plan to spend some time thinking about this and identifying any architectural issues that may need discussion in advance of the All Hands, so we can discuss and resolve any such issues there.
Assignee | ||
Comment 8•6 years ago
|
||
MozReview-Commit-ID: 6qkFXhRDFs
Assignee | ||
Comment 9•6 years ago
|
||
MozReview-Commit-ID: Fi1rKer53Jt Depends on D12602
Assignee | ||
Comment 10•6 years ago
|
||
Depends on D12603
Assignee | ||
Comment 11•6 years ago
|
||
I unbitrotted Markus' patches and pushed them to Phabricator. To provide some context here: Markus' patches are for the Layout changes required by this work. There are also APZ changes required, for which I need to write patches.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 12•6 years ago
|
||
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #12) > Created attachment 9028507 [details] > [WIP] Bug 1459312 - Apply the async zoom to the async zoom container layer > if appropriate. This is a (very) WIP draft of an approach to the APZ changes that I've been exploring. Lots of things need to be fixed in this patch, including async zooming, position:fixed rendering, scrollbar rendering, and position:fixed hit-testing (and probably other things that I haven't realized), but the general idea looks workable so far.
Assignee | ||
Comment 14•6 years ago
|
||
I've fixed async zooming and some clipping related issues locally, and I'm working on position:fixed rendering now. I realized that there is an interaction with bug 656036: with that in place, position:fixed elements can now scroll relative to the visual viewport. With container scrolling, they would scroll as part of the container, and we'd adjust them to remain fixed; to accommodate bug 656036, we would adjust them less. With containerless scrolling, though, there is nothing that would scroll them in the first place. I'll have to think a bit about how to handle that.
Comment 15•6 years ago
|
||
I had a think about this, since it was intriguing. I don't think I've entirely internalized bug 656036 yet, so I might be missing something. It seems to me that normal scrolling is operating on the root (or sub) scroll frames, but zooming (and then panning around that zoomed area) is operating on the document as a whole. That's why position:fixed items move, since they're part of the document we pan around, even though they're not attached to any scroll frame. Can we create the nsDisplayAsyncZoom at the ViewportFrame/nsLayoutUtils::PaintFrame level, to match that model more closely? It's not obvious what extra items would be included that (except maybe the unscrolled background color we add for overscroll), and if that isn't desired. It also means we don't have to duplicate the CreateDisplayListForStackingContext flattening logic. We could setup ASR/ScrollMetadata (or add code for this case to APZ) such that scrolling around the layout viewport happens by adding offsets to the nsDisplayAsyncZoom, that way we capture moving the position:fixed content. The obvious issue is that scroll hand-off is weird, I think we want to scroll the outermost scrollable layer (the nsDisplayAsyncZoom) first, and then progress with inner-to-outer for 'real' scrollframes.
Reporter | ||
Comment 16•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #14) > With containerless scrolling, though, there is nothing that would scroll > them in the first place. I'll have to think a bit about how to handle that. I realize that this suggestion would require more changes on the APZ side, but I think ideally we'll want to do the following: Treat the nsDisplayAsyncZoom as the "layout viewport", and apply two kinds of transforms to the ContainerLayer it generates: 1. the scale for the zoom, and 2. the translation for the offset between the layout viewport and the visual viewport. The translation for the offset between the root scroll frame's contents and the layout viewport would still go on the PaintedLayer that has the ScrollMetaData for the root scroll frame. (In reply to Matt Woodrow (:mattwoodrow) from comment #15) > Can we create the nsDisplayAsyncZoom at the > ViewportFrame/nsLayoutUtils::PaintFrame level, to match that model more > closely? I think if we do what I suggested above, we will match this model extremely closely. There's a problem with creating the nsDisplayAsyncZoom at the ViewportFrame/nsLayoutUtils::PaintFrame level: If we did that, the nsDisplayAsyncZoom would contain the root scroll frame's scrollbars, so zooming would move the scrollbars offscreen. The approach I took in this bug solves the scrollbar problem: The nsDisplayAsyncZoom is created at the exact point where we're inside of the root scroll frame's clip (which is important on platforms with classic scrollbars) but not scrolled by the root scroll frame yet.
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #16) > I realize that this suggestion would require more changes on the APZ side, > but I think ideally we'll want to do the following: Treat the > nsDisplayAsyncZoom as the "layout viewport", and apply two kinds of > transforms to the ContainerLayer it generates: 1. the scale for the zoom, > and 2. the translation for the offset between the layout viewport and the > visual viewport. I've thought about this on my way home, and the solution I came up with is along these lines, but implemented by adding the "translation for the offset between the layout viewport and the visual viewport" transform to fixed layers themselves. This can be done with a fairly limited amount of APZ changes, and no changes to the annotations Layout code creates.
Comment 18•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #17) > > I've thought about this on my way home, and the solution I came up with is > along these lines, but implemented by adding the "translation for the offset > between the layout viewport and the visual viewport" transform to fixed > layers themselves. This can be done with a fairly limited amount of APZ > changes, and no changes to the annotations Layout code creates. Does the code for finding and adjusting position:fixed Layers within a scroll container make sense in a post-containerful-scrolling world? Could we get rid of that code if we didn't do this, and is that a useful simplification for APZ?
Reporter | ||
Comment 19•6 years ago
|
||
Yes, we still need it for the case where you have a scrolled clip-path/mask with some position:fixed or background-attachment:fixed content. In these cases, the container layer with the mask layer needs to have the scroll meta data, otherwise the mask layer won't be moved correctly.
Reporter | ||
Comment 20•6 years ago
|
||
Er, "Yes" as in "Yes that code still makes sense, no we can't get rid of it."
Assignee | ||
Comment 21•6 years ago
|
||
I've partially fixed fixed-position rendering, but clipping of fixed-position elements is still wrong. For reference, my current patch series is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f76c50b2aef87e130d782be3577f2294fd1859a
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 22•6 years ago
|
||
I now have fixed-position rendering working locally, with correct clipping and all! Here is my plan going forward: * Land what I have so far, without enabling containerless scrolling by default. This patch series is long enough for one bug, especially given moz-phab's terrible performance on long patch series. * Get things into a state where we can enable containerless scrolling on nightly. This will include fixing scrollbar rendering, and getting tests to pass with it enabled. I'll do this in separate bugs. * Enable containerless scrolling on nightly. * Fix fixed-position hit testing. This is enough of an edge case that I think the pref can be enabled on nightly without this for a few days. (I could be wrong - that's what backouts are for.) * Once things are in good enough shape, let containerless scrolling ride the trains.
Assignee | ||
Comment 23•6 years ago
|
||
Here is the latest patch series, without the pref flip: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a36a12bf5fab1b0ea0368880c1e48af4f13217d It's triggering some assertions, even though it's not really supposed to change anything without flipping the pref.
Assignee | ||
Comment 24•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #23) > It's triggering some assertions, even though it's not really supposed to > change anything without flipping the pref. The assertions are introduced by Markus' third patch, "Wrap the root scroll frame contents into an nsDisplayAsyncZoom when using APZ zooming and containerless scrolling." I find that surprising, because the only functional change in that patch is conditional on apz.allow_zooming being enabled, which it is not in the desktop tests in question.
Assignee | ||
Comment 25•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #22) > * Enable containerless scrolling on nightly. Tracked in bug 1158392. > * Once things are in good enough shape, let containerless > scrolling ride the trains. Tracked in bug 1137890. I'm going to move the dependencies of this bug over to bug 1137890.
Assignee | ||
Comment 26•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #25) > I'm going to move the dependencies of this bug over to bug 1137890. Things blocked by this bug, I mean.
Assignee | ||
Comment 27•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #24) > The assertions are introduced by Markus' third patch, "Wrap the root scroll > frame contents into an nsDisplayAsyncZoom when using APZ zooming and > containerless scrolling." It looks like the patch is changing the scopes of some variables in a way that was not obvious to me initially. Prior to the patch, the AutoCurrentScrollParentIdSetter variable declared here [1] was destroyed here [2]. With the patch, it lives longer, until here [3]. In particular, it's still on the stack when an nsDisplayScrollInfoLayer is created here [4], which was not the case before. As a result, the scroll info layer is made to think it is its own scroll parent, which causes the assertions. Markus, was this change of scopes an intended side effect of the patch? Should the patch be doing something differently? [1] https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/layout/generic/nsGfxScrollFrame.cpp#3418 [2] https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/layout/generic/nsGfxScrollFrame.cpp#3576 [3] https://hg.mozilla.org/mozreview/gecko/file/b2cc7e09e89e/layout/generic/nsGfxScrollFrame.cpp#l3793 [4] https://hg.mozilla.org/mozreview/gecko/file/b2cc7e09e89e/layout/generic/nsGfxScrollFrame.cpp#l3734
Reporter | ||
Comment 28•6 years ago
•
|
||
Oh dear, the scopes of things in that function are utterly inscrutable. No, this change of scopes was definitely not an intended effect of the patch. We need to change the scoping such that the AutoCurrentScrollParentIdSetter
gets destroyed in the same spot as before, i.e. ideally just before the
if (mWillBuildScrollableLayer && aBuilder->IsPaintingToWindow()) {
aBuilder->ForceLayerForScrollParent();
}
block. I hope this doesn't conflict with any of the other scope changes that the patch made.
In terms of outside-to-inside wrapping of things, I think we want the following order:
- Viewport clip, affects:
- optional
nsDisplayAsyncZoom
container item, wraps:- optional
nsDisplayScrollInfoLayer
- scroll parent ID setter, affects:
asrSetter
, affects:- the items which are built in the call to
mOuter->BuildDisplayListForChild
.
- the items which are built in the call to
- optional
- optional
So really, the only scope change that this patch wants to make is to have a scope for the nsDisplayAsyncZoom
for which the following are true:
- The asrSetter is not on the stack, and
- The Viewport clip is on the stack.
The rest should stay the same.
Assignee | ||
Comment 29•6 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #28)
In terms of outside-to-inside wrapping of things, I think we want the following order:
- Viewport clip, affects:
- optional
nsDisplayAsyncZoom
container item, wraps:
- optional
nsDisplayScrollInfoLayer
- scroll parent ID setter, affects:
asrSetter
, affects:
- the items which are built in the call to
mOuter->BuildDisplayListForChild
.
Assuming the "viewport clip" is this, that's not consistent with the current state of things! Currently, the viewport clip does not affect the optional nsDisplayScrollInfoLayer
(the viewport clip goes out of scope here, and the nsDisplayScrollInfoLayer
is only built here).
Assignee | ||
Comment 30•6 years ago
|
||
(Answered in person. As the optional scroll info layer is currently outside the viewport clip, it can remain outside the viewport clip and the async zoom container.)
Assignee | ||
Comment 31•6 years ago
|
||
I have a patch series that passes Try (without flipping the pref). I will split it into two parts to keep the size of the patch series manageable (because of moz-phab performance issues and such): the Layout parts in bug 1521644, and the APZ parts here.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 32•6 years ago
|
||
APZ patches now cleaned up:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef94da9876f546a5dc57d15f82da46d3b36ac597
Assignee | ||
Comment 33•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #22)
Get things into a state where we can enable containerless
scrolling on nightly. This will include fixing scrollbar
rendering, and getting tests to pass with it enabled.I'll do this in separate bugs.
Scrollbar rendering is tracked in bug 1522338.
- Fix fixed-position hit testing. This is enough of an edge
case that I think the pref can be enabled on nightly
without this for a few days. (I could be wrong - that's
what backouts are for.)
Fixed-position hit testing is tracked in bug 1522714. (And I changed my mind about deferring it until after the pref is enabled on Nightly. I intend to fix it first.)
Assignee | ||
Comment 34•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 35•6 years ago
|
||
It is an implementation detail of GetCurrentAsyncTransformForFixedAdjustment().
Depends on D13348
Assignee | ||
Comment 36•6 years ago
|
||
Depends on D17719
Assignee | ||
Comment 37•6 years ago
|
||
This reflects the fact that it's no longer optional (the code path that
wouldn't pass one was removed with JPZC).
Depends on D17720
Assignee | ||
Comment 38•6 years ago
|
||
This helper will be reused for translating layers fixed to the RCD-RSF
with containerless scrolling.
Depends on D17722
Assignee | ||
Comment 39•6 years ago
|
||
Depends on D17723
Assignee | ||
Comment 40•6 years ago
|
||
For brevity, this is referred to as the "relative visual offset/transform"
in the code.
Depends on D17724
Assignee | ||
Comment 41•6 years ago
|
||
Depends on D17725
Assignee | ||
Comment 42•6 years ago
|
||
As usual Phabricator is terrible about putting the patches in the right order...
Assignee | ||
Comment 43•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #42)
As usual Phabricator is terrible about putting the patches in the right order...
Ok, after some manual use of "Edit Related Revisions", the "Stack" view in Phabricator now shows the correct order.
Assignee | ||
Comment 44•6 years ago
|
||
Here is a high level description of what these patches are doing:
The Layout changes in bug 1521644 make it so that, with layout.scroll.root-frame-containers=false
and apz.allow_zooming=true
, the layer structure looks something like this:
- root container layer
- async zoom container layer
- scrollable content layers, with scroll metadata
- fixed content layers (no scroll metadata, annotated
isFixedPosition
)
- scrollbar layers
- async zoom container layer
The patches in this bug implement correct rendering of this tree in AsyncCompositionManager. Notably:
- We lift the zoom portion of the async transform of the RCD-RSF APZC, from the scrollable content layers to the async zoom container layer (so that the zoom applies to the fixed layers too).
- We transform the fixed content layers by the amount the visual viewport has scrolled relative to the layout viewport. This preserves the behaviour for fixed layers implemented in bug 656036. Previously, this same net transformation was accomplished by having fixed layers carry the scroll metadata, and un-transforming them by the desired amount in
AlignFixedAndStickyLayers
. (See comments 14-17 for more discussion of this.)
Comment 45•6 years ago
|
||
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c858a0fe0fea AsyncPanZoomController support for exposing the scroll and zoom portions of the async transform independently. r=kats https://hg.mozilla.org/integration/autoland/rev/4d6a4c43775b AsyncCompositionManager changes to apply the async zoom to the async zoom container layer if appropriate. r=kats https://hg.mozilla.org/integration/autoland/rev/11ac6c2ac639 Make AsyncPanZoomController::GetCurrentAsyncViewportTransform() private. r=kats https://hg.mozilla.org/integration/autoland/rev/016c3e822444 Remove an outdated comment in TransformShadowTree(). r=kats https://hg.mozilla.org/integration/autoland/rev/949c039c7de9 Have AlignFixedAndStickyLayers take the ClipPartsCache by reference rather than pointer. r=kats https://hg.mozilla.org/integration/autoland/rev/5deb799c5ba3 Factor out a helper function from AlignFixedAndStickyLayers. r=kats https://hg.mozilla.org/integration/autoland/rev/9b072d3caedb Expose IsAsyncZoomContainer() in LayerMetricsWrapper. r=kats https://hg.mozilla.org/integration/autoland/rev/494c256e2e37 Have APZC expose the async transform of the visual viewport relative to the layout viewport. r=kats https://hg.mozilla.org/integration/autoland/rev/a03c30492ed7 Scroll layers that are fixed w.r.t. the RCD-RSF by the relative visual transform. r=kats
Comment 46•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c858a0fe0fea
https://hg.mozilla.org/mozilla-central/rev/4d6a4c43775b
https://hg.mozilla.org/mozilla-central/rev/11ac6c2ac639
https://hg.mozilla.org/mozilla-central/rev/016c3e822444
https://hg.mozilla.org/mozilla-central/rev/949c039c7de9
https://hg.mozilla.org/mozilla-central/rev/5deb799c5ba3
https://hg.mozilla.org/mozilla-central/rev/9b072d3caedb
https://hg.mozilla.org/mozilla-central/rev/494c256e2e37
https://hg.mozilla.org/mozilla-central/rev/a03c30492ed7
Updated•5 years ago
|
Description
•