1164767 - modify async scrollbar positioning code to deal with containerless root scrollables

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8605672 [details] [diff] [review]
patch

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +829,5 @@
>      // layers on the ancestor chain from the scrollbar layer up to but not
>      // including the layer with the async transform. Otherwise the scrollbar
>      // shifts but gets clipped and so appears to flicker.
>      for (Layer* ancestor = aScrollbar; ancestor != aContent.GetLayer(); ancestor = ancestor->GetParent()) {
>        TransformClipRect(ancestor, compensation);

If we're IsRootScrollable() but not aScrollbarIsDescendant, do we not still want to apply the resolution-cancelling transform to the clip rect?

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8605672 [details] [diff] [review]
patch

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

(In reply to Botond Ballo [:botond] from comment #2)
> If we're IsRootScrollable() but not aScrollbarIsDescendant, do we not still
> want to apply the resolution-cancelling transform to the clip rect?

After a long IRC discussion, I think we settled on the fact that we actually do *not* want to apply the resolution-cancelling transform to the clip rect ever. (Said application was introduced in bug 1043859 where it "rode along" with a change to apply the resolution-cancelling transform (then called the "non-transient async transform") to the scrollbar layer's transform, the latter change was necessary for correctness.)

That still makes this patch slightly incorrect, but not in the way I thought. I think it should go something like:

if (metrics.IsRootScrollable()) {
  compensation =
      Matrix4x4::Scaling(metrics.GetPresShellResolution(),
                         metrics.GetPresShellResolution(),
                         1.0f).Inverse();
}

if (aScrollbarIsDescendant) {
  Matrix4x4 asyncUntransform = (asyncTransform * apzc->GetOverscrollTransform()).Inverse();
  Matrix4x4 contentTransform = aContent.GetTransform();
  Matrix4x4 contentUntransform = contentTransform.Inverse();

  Matrix4x4 asyncCompensation = contentTransform
                              * asyncUntransform
                              * contentUntransform;
 
  for (Layer* ancestor = aScrollbar; ancestor != aContent.GetLayer(); ancestor = ancestor->GetParent()) {
    TransformClipRect(ancestor, asyncCompensation);
  }

  compensation = compensation * asyncCompensation;
}

transform = transform * compensation;

(the important change being that we don't apply the part of 'compensation' that was potentially set in the 'IsRootScrollable()' block, to the clip).

I'm happy with fixing this in a follow-up if you prefer.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1184021538f6

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #3)
> I'm happy with fixing this in a follow-up if you prefer.

Bug 1165536 is the follow-up for this. I prefer to make the change to the existing behaviour is a separate bug in case it causes regressions.

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1184021538f6