1061327 - Fix up async scrollbar thumb offset code for multi layer APZ

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Markus Stange [:mstange]]

After bug 967844, async scrolling sometimes doesn't update the scrollbar's position on the Compositor because AsyncCompositionManager doesn't find the scrolled layer in some cases.

Comment 1

[Markus Stange [:mstange]]

Attachment: part 1: Don't stop searching for scrolled layers when encountering a ScrollInfoLayer.

Returning true from LayerIsScrollbarTarget for ScrollInfoLayers means that we return that layer from FindScrolledLayerForScrollbar and then take the early exit in ApplyAsyncTransformToScrollbarForContent.
Instead we need to skip scroll info layers and keep on searching for the real scrolled layer.

Comment 2

[Markus Stange [:mstange]]

Attachment: part 2: When the scrolled layer is not an ancestor of the scrollbar layer, search the whole layer tree.

With a nested scroll frame I had this layer tree:

                     +---+
                     | A |
                     +---+
                    /  |  \
                   /   |   \
                  /    |    \
           +-----+  +-----+  +-----+
           |  B  |  |  C  |  |  D  | scrollbar own layer
           +-----+  +-----+  +-----+
           | FM1 |  | FM1 |  | FM1 |
           +-----+  | FM0 |  +-----+
                    +-----+    / \
                              /   \
                          +---+  +---+
  scrolltrack thebeslayer | E |  | F | scrollbar thumb own layer, has target FM0
                          +---+  +---+
                                   |
                                 +---+
                                 | G | scrollbar thumb thebes layer
                                 +---+

A: ContainerLayer of the content page
B: Scrolled page background thebes layer
C: Nested scrollable thebes layer
D: ContainerLayer for overlay scrollbar on top of C (that scrolls C)
E: ThebesLayer for scrollbar track
F: ContainerLayer for overlay scrollbar thumb (annotated as scrollbar layer)
G: ThebesLayer for overlay scrollbar thumb

The scrollbar of the nested scrollframe (wrapped by D) scrolls along with the parent page, so it's annotated with the parent frame metrics FM1.

We have F and need to find C|FM0. The existing search algorithm doesn't find it because the sibling of D|FM1 is C|FM1 and we don't descend again.

I haven't found a solution for this which I'm happy with. Searching the whole layer tree can be expensive. Any ideas?

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8482409 [details] [diff] [review]
part 1: Don't stop searching for scrolled layers when encountering a ScrollInfoLayer.

Review of attachment 8482409 [details] [diff] [review]:
-----------------------------------------------------------------

This fix looks fine in principle. For my own understanding though, could you show what layer tree required this? (It seems you'd need two layers sharing a scroll id, with one being layerized (i.e. it's not a scroll info layer) and the other not - something I wouldn't expect, but then I haven't fully assimilated multi-layer-apz yet.)

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8482410 [details] [diff] [review]
part 2: When the scrolled layer is not an ancestor of the scrollbar layer, search the whole layer tree.

Review of attachment 8482410 [details] [diff] [review]:
-----------------------------------------------------------------

I'm OK with landing this to un-regress the behaviour until we agree on a more performant solution.

> I haven't found a solution for this which I'm happy with. Searching the
> whole layer tree can be expensive. Any ideas?

What if we walked the Layer tree directly rather than via LayerMetricsWrapper, and iterated over the FrameMetrics on each layer manually? We should then be able to continue to restrict ourselves to the siblings-and-ancestors search (at the Layer level).

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +752,5 @@
> +{
> +  // Search ancestors first.
> +  LayerMetricsWrapper scrollbar(aScrollbar);
> +  for (LayerMetricsWrapper ancestor = scrollbar; ancestor; ancestor = ancestor.GetParent()) {
> +    if (LayerIsScrollbarTarget(ancestor, aScrollbar)) {

In the previous code, we had an explicit 'scrollTarget != scrollbar' check. I guess it was just an optimization, as the scrollbar layer shouldn't be its own scroll target, but we might as well keep it. (Or alternately, just start the loop at 'scrollbar.GetParent()').

@@ +762,5 @@
> +  // If the scrolled target is not an ancestor, search the whole layer tree.
> +  // XXX It would be much better to search the APZC tree instead of the layer
> +  // tree. That way we would ignore non-scrollable layers, and we'd only visit
> +  // each scroll ID once. In the end we only need the APZC and the FrameMetrics
> +  // of the scrolled target.

We should be able to walk the APZC tree here if we want. We would just need to add a method to APZCTreeManager that does so (acquiring mTreeLock at the beginning) and returns the target APZC. However, I'd like us to consider my other suggestion (walking the Layer tree directly rather than via LayerMetricsWrapper) first.

Comment 5

[Botond Ballo [:botond]]

Since these are regressions from multi-layer-apz, we should probably ask for uplift once these patches land.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #3)
> This fix looks fine in principle. For my own understanding though, could you
> show what layer tree required this? (It seems you'd need two layers sharing
> a scroll id, with one being layerized (i.e. it's not a scroll info layer)
> and the other not - something I wouldn't expect, but then I haven't fully
> assimilated multi-layer-apz yet.)

Bug 1063158 is on file for having a layer tree like this. Perhaps we should wait until that's resolved and re-evaluate the fixes here before landing.

Comment 9

[Botond Ballo [:botond]]

Markus, I believe that roc's patch in bug 1063158 makes the Part 1 patch in this bug unnecessary, because layer trees where some but not all layers sharing a scroll id are scrollinfo layers, are no longer created. (Do you agree?)

I'm not sure about Part 2 - do you know whether Part 2 is still necessary?

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #9)
> Markus, I believe that roc's patch in bug 1063158 makes the Part 1 patch in
> this bug unnecessary, because layer trees where some but not all layers
> sharing a scroll id are scrollinfo layers, are no longer created. (Do you
> agree?)
> 
> I'm not sure about Part 2 - do you know whether Part 2 is still necessary?

Bug 1063158 was closed as WONTFIX, making the Part 1 patch in bug necessary after all. I think we can go ahead and land it (and probably Part 2 as well, fixing layer-tree-walking-performance issues in a follow-up).

Comment 11

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b450c92f7b86
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e77e360354a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b450c92f7b86
https://hg.mozilla.org/mozilla-central/rev/0e77e360354a

Comment 13

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

Does this need uplifting to 34?

Comment 14

[Markus Stange [:mstange]]

Looks like it does. I'll check whether the patches apply.

Comment 15

[Markus Stange [:mstange]]

They apply without problems.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8482409 [details] [diff] [review]
part 1: Don't stop searching for scrolled layers when encountering a ScrollInfoLayer.

Approval Request Comment
[Feature/regressing bug #]: regression from bug 967844
[User impact if declined]: scrollbars lagging behind during scrolling
[Describe test coverage new/current, TBPL]: none
[Risks and why]: low, got lots of testing on central + aurora
[String/UUID change made/needed]: none

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8482409 [details] [diff] [review]
part 1: Don't stop searching for scrolled layers when encountering a ScrollInfoLayer.

Beta+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0174d3047d1a
https://hg.mozilla.org/releases/mozilla-beta/rev/eed413466305

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/0174d3047d1a
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/eed413466305