1213716 - APZ assumes all scrollable layers for a given scroll id will have the same ancestor transform

Status: RESOLVED DUPLICATE of bug 1168263

(Core :: Panning and Zooming, defect)

Description

[Matt Woodrow (:mattwoodrow)]

With the patches from bug 1168263, this isn't always true any more.

With those patches we end up with a layer tree that looks like this:

     A
   /   \
  B     C
        |
        D

Where B and D have FrameMetrics for the same scroll id, and C has a transform (the perspective transform).

When we scroll the scrollframe, we want to apply the async transforms directly to B and D, such that D moves by a different number of pixels on the screen (this gives us the parallax scrolling effect we want).

However, when we do a touch scroll, we always want the movement of B to track the user's finger, never the movement of D.

It seems that the current code would pick which layer to track the finger arbitrarily (depending on the exact layer tree structure, and which layer was picked by hit testing), which is going to give us results that don't match what the main thread would do.

I think the simplest thing to do is to annotate the FrameMetrics for D with something that specifies that its parent layer is the 'virtual' location of the scrollframe (naming ideas welcome), and should be used as the location for determining the translation amount for the async scroll.

This is a very specific case, and I don't think we're planning on having other things that could hit this assertion, so a single boolean for perspective transforms seems simpler than a general solution.

Botond, how does that sound?

Comment 1

[Botond Ballo [:botond]]

So for the example layer tree above, C would have a boolean flag specifying that it has a perspective transform?

Comment 2

[Matt Woodrow (:mattwoodrow)]

I was thinking that the FrameMetrics of D would have a flag saying 'use my parent layer for determining scroll amounts'.

C won't have FrameMetrics, so we'd have to put the flag on the layer itself to do it there.

Comment 3

[Botond Ballo [:botond]]

Will the perspective transform always be the direct parent of the scrollable layer (i.e. there will never be something that inserts an additional layer in between)?

Comment 4

[Matt Woodrow (:mattwoodrow)]

I believe so, I can't think of any ways to get other layers in between.

Comment 5

[Matt Woodrow (:mattwoodrow)]

I could annotate C instead though to be safe.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Ok, so I just found another issue with clipping.

C and D used to be a single layer, and we basically want to preserve a lot of that behavior even though we've split it into two separate ones here.

In particular any clipping that used to happen for the combined layer should be applied to C (since that's the 'post-transform' location when the two are multiplied together).

I have that working at the display list level, but the metrics for D still include the clip data, so APZ will re-apply it (as a shadow-clip) to D. This clip on D forces an intermediate surface for C, and breaks rendering.

Might it make more sense to move the Metrics onto C and treat C as the scrollable layer, with an annotation to say that the async transform should be applied to the child (there will only ever be one)?

Comment 7

[Botond Ballo [:botond]]

What if we gave layers a "post-async transform", i.e. a transform that applies on top of the async transform? C could then have the metrics, an empty regular transform, and the perspective transform as its post-async transform.

Comment 8

[Matt Woodrow (:mattwoodrow)]

I don't love the idea of adding yet another transform property to Layer that we have to account for in all places that deal with transforms.

Changing APZ to just apply the computed async scroll to a different layer (which is always the only child of the normal layer) seems like a much more isolated change.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

For the record, this change seems scary to me. I don't remember the details, but I remember this assumption (the one in the bug summary) coming up explicitly at some point during the multi-layer-apz work and there's probably code in APZ that depends on it in subtle ways. I'm not opposed to changing it but we should be extra-careful and probably audit any APZ and tiling code that walks around in the layers tree to make sure it won't break if this assumption changes.

Comment 10

[Botond Ballo [:botond]]

Note that the work on this is now being done in bug 1168263; specifically, see the Part 2 patch, and my review of it in bug 1168263 comment 13.

Comment 11

[Botond Ballo [:botond]]

Please see bug 1168263 (particularly bug 1168263 comment 13 and bug 1168263 comment 16 onwards) for continued discussion on this. I'm closing this bug as I don't intend to land any patches here.