1459312 - Basic support for APZ zooming with containerless scrolling

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1459312 - Add a layer property that indicates async zoom container layers.

Review commit: https://reviewboard.mozilla.org/r/241806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241806/

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1459312 - Create nsDisplayAsyncZoom which creates a ContainerLayer that is marked as an async zoom container.

Review commit: https://reviewboard.mozilla.org/r/241808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241808/

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1459312 - Wrap the root scroll frame contents into an nsDisplayAsyncZoom when using APZ zooming and containerless scrolling.

Review commit: https://reviewboard.mozilla.org/r/241810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241810/

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8973344 [details]
Bug 1459312 - Create nsDisplayAsyncZoom which creates a ContainerLayer that is marked as an async zoom container.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241808/diff/1-2/

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8973345 [details]
Bug 1459312 - Wrap the root scroll frame contents into an nsDisplayAsyncZoom when using APZ zooming and containerless scrolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241810/diff/1-2/

Comment 6

[Chris Peterson [:cpeterson]]

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.

Comment 7

[Botond Ballo [:botond]]

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.

Comment 8

[Botond Ballo [:botond]]

Attachment: Bug 1521644 - Add a layer property that indicates async zoom container layers. r=mattwoodrow

MozReview-Commit-ID: 6qkFXhRDFs

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1521644 - Create nsDisplayAsyncZoom which creates a ContainerLayer that is marked as an async zoom container. r=mattwoodrow

MozReview-Commit-ID: Fi1rKer53Jt

Depends on D12602

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1521644 - Wrap the root scroll frame contents into an nsDisplayAsyncZoom when using APZ zooming and containerless scrolling. r=mstange,mattwoodrow

Depends on D12603

Comment 11

[Botond Ballo [:botond]]

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.

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1459312 - AsyncCompositionManager changes to apply the async zoom to the async zoom container layer if appropriate. r=kats

Comment 13

[Botond Ballo [:botond]]

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

Comment 14

[Botond Ballo [:botond]]

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

[Matt Woodrow (:mattwoodrow)]

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.

Comment 16

[Markus Stange [:mstange]]

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

Comment 17

[Botond Ballo [:botond]]

(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

[Matt Woodrow (:mattwoodrow)]

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

Comment 19

[Markus Stange [:mstange]]

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.

Comment 20

[Markus Stange [:mstange]]

Er, "Yes" as in "Yes that code still makes sense, no we can't get rid of it."

Comment 21

[Botond Ballo [:botond]]

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

Comment 22

[Botond Ballo [:botond]]

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.

Comment 23

[Botond Ballo [:botond]]

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.

Comment 24

[Botond Ballo [:botond]]

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

Comment 25

[Botond Ballo [:botond]]

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

Comment 26

[Botond Ballo [:botond]]

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

Comment 27

[Botond Ballo [:botond]]

(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

Comment 28

[Markus Stange [:mstange]]

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.

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:

1. The asrSetter is not on the stack, and
2. The Viewport clip is on the stack.

The rest should stay the same.

Comment 29

[Botond Ballo [:botond]]

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

Comment 30

[Botond Ballo [:botond]]

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

Comment 31

[Botond Ballo [:botond]]

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.

Comment 32

[Botond Ballo [:botond]]

APZ patches now cleaned up:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef94da9876f546a5dc57d15f82da46d3b36ac597

Comment 33

[Botond Ballo [:botond]]

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

Comment 34

[Botond Ballo [:botond]]

Attachment: Bug 1459312 - AsyncPanZoomController support for exposing the scroll and zoom portions of the async transform independently. r=kats

Comment 35

[Botond Ballo [:botond]]

Attachment: Bug 1459312 - Make AsyncPanZoomController::GetCurrentAsyncViewportTransform() private. r=kats

It is an implementation detail of GetCurrentAsyncTransformForFixedAdjustment().

Depends on D13348

Comment 36

[Botond Ballo [:botond]]

Attachment: Bug 1459312 - Remove an outdated comment in TransformShadowTree(). r=kats

Depends on D17719

Comment 37

[Botond Ballo [:botond]]

Attachment: Bug 1459312 - Have AlignFixedAndStickyLayers take the ClipPartsCache by reference rather than pointer. r=kats

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

Comment 38

[Botond Ballo [:botond]]

Attachment: Bug 1459312 - Factor out a helper function from AlignFixedAndStickyLayers. r=kats

This helper will be reused for translating layers fixed to the RCD-RSF
with containerless scrolling.

Depends on D17722

Comment 39

[Botond Ballo [:botond]]

Attachment: Bug 1459312 - Expose IsAsyncZoomContainer() in LayerMetricsWrapper. r=kats

Depends on D17723

Comment 40

[Botond Ballo [:botond]]

Attachment: Bug 1459312 - Have APZC expose the async transform of the visual viewport relative to the layout viewport. r=kats

For brevity, this is referred to as the "relative visual offset/transform"
in the code.

Depends on D17724

Comment 41

[Botond Ballo [:botond]]

Attachment: Bug 1459312 - Scroll layers that are fixed w.r.t. the RCD-RSF by the relative visual transform. r=kats

Depends on D17725

Comment 42

[Botond Ballo [:botond]]

As usual Phabricator is terrible about putting the patches in the right order...

Comment 43

[Botond Ballo [:botond]]

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

Comment 44

[Botond Ballo [:botond]]

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

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

[Pulsebot]

Pushed by [e-mail]:
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

[Arthur Iakab [arthur_iakab]]

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