modify async scrollbar positioning code to deal with containerless root scrollables

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

No description provided.
Posted patch patchSplinter Review
Attachment #8605672 - Flags: review?(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 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.
Attachment #8605672 - Flags: review?(botond) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/1184021538f6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.