Closed Bug 1061327 Opened 6 years ago Closed 6 years ago

Fix up async scrollbar thumb offset code for multi layer APZ

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox33 --- unaffected
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(2 files)

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.
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.
Attachment #8482409 - Flags: review?(botond)
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?
Attachment #8482410 - Flags: review?(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.)
Attachment #8482409 - Flags: review?(botond) → review+
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.
Attachment #8482410 - Flags: review?(botond) → review+
Since these are regressions from multi-layer-apz, we should probably ask for uplift once these patches land.
(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.
See Also: → 1063158
Duplicate of this bug: 1063101
Duplicate of this bug: 1063333
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?
Flags: needinfo?(mstange)
(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).
Flags: needinfo?(mstange)
https://hg.mozilla.org/mozilla-central/rev/b450c92f7b86
https://hg.mozilla.org/mozilla-central/rev/0e77e360354a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Does this need uplifting to 34?
Flags: needinfo?(mstange)
Looks like it does. I'll check whether the patches apply.
Flags: needinfo?(mstange)
They apply without problems.
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
Attachment #8482409 - Flags: approval-mozilla-beta?
Attachment #8482410 - Flags: approval-mozilla-beta?
Comment on attachment 8482409 [details] [diff] [review]
part 1: Don't stop searching for scrolled layers when encountering a ScrollInfoLayer.

Beta+
Attachment #8482409 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8482410 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.