Basic support for APZ zooming with containerless scrolling

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: mstange, Assigned: botond)

Tracking

(Blocks 1 bug)

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 affected, firefox67 fixed)

Details

(Whiteboard: [gfx-noted][geckoview:p3])

Attachments

(9 attachments, 6 obsolete attachments)

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

Description

a year ago
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

a year ago
Blocks: desktop-zoom
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
(Assignee)

Updated

11 months ago
No longer blocks: desktop-zoom
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.
Whiteboard: [gfx-noted] → [gfx-noted][geckoview:p3]
(Assignee)

Updated

6 months ago
Blocks: android-rdl
(Assignee)

Updated

6 months ago
Blocks: 1489653
(Assignee)

Updated

5 months ago
Assignee: nobody → botond
(Assignee)

Comment 7

5 months 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

5 months ago
MozReview-Commit-ID: 6qkFXhRDFs
(Assignee)

Comment 11

5 months 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

5 months ago
Attachment #8973343 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8973344 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8973345 - Attachment is obsolete: true
(Assignee)

Comment 13

5 months 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

4 months 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.
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

4 months 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

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

4 months 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

4 months ago
Er, "Yes" as in "Yes that code still makes sense, no we can't get rid of it."
(Assignee)

Comment 21

4 months 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

4 months ago
Summary: Prototype APZ zooming with containerless scrolling → Support APZ zooming with containerless scrolling
(Assignee)

Updated

4 months ago
Depends on: 1514823
(Assignee)

Comment 22

4 months 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

4 months 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

4 months 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)

Updated

4 months ago
Blocks: 1158392
(Assignee)

Comment 25

4 months 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.
Summary: Support APZ zooming with containerless scrolling → Basic support for APZ zooming with containerless scrolling
(Assignee)

Comment 26

4 months 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.
Blocks: 1158392
(Assignee)

Comment 27

4 months 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
Flags: needinfo?(mstange)
(Reporter)

Comment 28

3 months 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.

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.

Flags: needinfo?(mstange)
Blocks: 1489653
(Assignee)

Comment 29

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

Flags: needinfo?(mstange)
(Assignee)

Comment 30

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

Flags: needinfo?(mstange)
(Assignee)

Updated

3 months ago
Depends on: 1521644
(Assignee)

Comment 31

3 months 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.

Attachment #9026830 - Attachment description: Bug 1459312 - Add a layer property that indicates async zoom container layers. → Bug 1521644 - Add a layer property that indicates async zoom container layers. r=mattwoodrow
Attachment #9026831 - Attachment description: Bug 1459312 - Create nsDisplayAsyncZoom which creates a ContainerLayer that is marked as an async zoom container. → Bug 1521644 - Create nsDisplayAsyncZoom which creates a ContainerLayer that is marked as an async zoom container. r=mattwoodrow
Attachment #9026832 - Attachment description: Bug 1459312 - Wrap the root scroll frame contents into an nsDisplayAsyncZoom when using APZ zooming and containerless scrolling. → Bug 1521644 - Wrap the root scroll frame contents into an nsDisplayAsyncZoom when using APZ zooming and containerless scrolling. r=mstange,mattwoodrow
Attachment #9026832 - Attachment is obsolete: true
Attachment #9026831 - Attachment is obsolete: true
Attachment #9026830 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Blocks: 1522338
(Assignee)

Updated

3 months ago
Blocks: 1522714
(Assignee)

Comment 33

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

Attachment #9028507 - Attachment description: [WIP] Bug 1459312 - Apply the async zoom to the async zoom container layer if appropriate. → Bug 1459312 - AsyncCompositionManager changes to apply the async zoom to the async zoom container layer if appropriate. r=kats
(Assignee)

Comment 35

3 months ago

It is an implementation detail of GetCurrentAsyncTransformForFixedAdjustment().

Depends on D13348

(Assignee)

Comment 37

3 months 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

3 months ago

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

Depends on D17722

(Assignee)

Comment 40

3 months ago

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

Depends on D17724

(Assignee)

Comment 42

3 months ago

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

(Assignee)

Comment 43

3 months 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

3 months 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

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

3 months 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
You need to log in before you can comment on or make changes to this bug.