1019996 - Overscroll transform does not play nicely with position:fixed elements

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

STR:
  1. Enable overscrolling in the developer prefs.
  2. Open www.motorola.ca in the B2G browser.
  3. Overscroll the page past the top.

While most of the page content shrinks and scrolls down, the position:fixed header shrinks and stays in place, resulting in a strange, asymmetric look.

We should probably account for overscroll transforms when handling position:fixed layers in AsyncCompositionManager.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Another way to reproduce:

1) Open twitter app
2) Scroll up until you hit overscroll
3) Watch the twitter header flex horizontally in addition to vertically

Comment 2

[Botond Ballo [:botond]]

After reading over the relevant code in AsyncCompositionManager (mostly AlignFixedAndStickyLayers and its call sites), I think the following, relatively simple, approach should work:

Instead of having AlignFixedAndStickyLayers calculate 'newRootTransform' as the transformed subtree root's GetLocalTransform() [1], make it a parameter to the function (much like how 'oldRootTransform' comes from the 'aPreviousTransformForRoot' parameter), and at the call site pass in a transform which is like the local transform, but excludes the portion of the APZC transform that is due to overscroll (we would get APZC to expose this in SampleContentTransformForFrame). The idea is that this portion of the transform would not be unapplied, and therefore the overscroll transform would be applied to the fixed-position layer as well, which I understand is what we want.

Chris, does this sound like a reasonable solution?

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?from=AsyncCompositionManager.cpp#265

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Botond Ballo [:botond] from comment #2)
> After reading over the relevant code in AsyncCompositionManager (mostly
> AlignFixedAndStickyLayers and its call sites), I think the following,
> relatively simple, approach should work:
> 
> Instead of having AlignFixedAndStickyLayers calculate 'newRootTransform' as
> the transformed subtree root's GetLocalTransform() [1], make it a parameter
> to the function (much like how 'oldRootTransform' comes from the
> 'aPreviousTransformForRoot' parameter), and at the call site pass in a
> transform which is like the local transform, but excludes the portion of the
> APZC transform that is due to overscroll (we would get APZC to expose this
> in SampleContentTransformForFrame). The idea is that this portion of the
> transform would not be unapplied, and therefore the overscroll transform
> would be applied to the fixed-position layer as well, which I understand is
> what we want.
> 
> Chris, does this sound like a reasonable solution?
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> AsyncCompositionManager.cpp?from=AsyncCompositionManager.cpp#265

Yes, this sounds reasonable, though I suppose you'll have to change the passed in Matrix when the transformed sub-tree root changes. But yes, good idea :)

Comment 4

[Botond Ballo [:botond]]

Attachment: Part 1 - Remove an unnecessary call to AlignFixedAndStickyLayers

It would make my work easier in this bug if I only had to supply the {overscroll transform}-less async transform to AlignFixedAndStickyLayers() at the call site in ApplyAsyncContentTransformToTree().

There is however another call site to AlignFixedAndStickyLayers() at [1] that would need to provide this transform. I can do this, but upon closer inspection, this call site seems unnecessary to me, because ApplyAsyncContentTransformToTree() will have already called AlignFixedAndStickyLayers() on this descendant scrollable layer when it recursed on it at [2]. (The early return, to prevent recursing on the descendant layer here [3], _is_ necessary.)

Chris, what do you think about removing this call site (but keeping the early return)? The attached patch does this. I tested it with the STR in bug 950993, which is the bug whose fix introduced the call site, and it does not regress the behaviour.

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?from=AsyncCompositionManager.cpp#343
[2] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?from=AsyncCompositionManager.cpp#510
[3] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?from=AsyncCompositionManager.cpp#347

Comment 5

[Botond Ballo [:botond]]

Attachment: Part 2 - Apply overscroll effect to fixed and sticky layers

Seems to be working!

Comment 6

[Botond Ballo [:botond]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a7baab88a6f0

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 8439481 [details] [diff] [review]
Part 1 - Remove an unnecessary call to AlignFixedAndStickyLayers

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +335,5 @@
>  
>    // Fixed layers are relative to their nearest scrollable layer, so when we
> +  // encounter a scrollable layer, bail. ApplyAsyncContentTransformToTree will
> +  // have already recursed on this layer and called AlignFixedAndStickyLayers
> +  // on it with its own transforms.

There are two paths that call AlignFixedAndStickyLayers though, right? Though I believe the fennec path won't be affected by this anyway, so I think this is ok.

Comment 8

[Chris Lord [:cwiiis]]

Comment on attachment 8439610 [details] [diff] [review]
Part 2 - Apply overscroll effect to fixed and sticky layers

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

LGTM, some nits and things to consider.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +160,5 @@
>     * called (though not necessarily). |aSampleTime| is the time that this is
>     * sampled at; this is used for interpolating animations. Calling this sets a
>     * new transform in |aNewTransform| which should be multiplied to the transform
> +   * in the shadow layer corresponding to this APZC. If the layer is 
> +   * overscrolled, |aOverscrollTransform| is also set, which should also be 

nit, trailing space on these two lines.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +534,5 @@
>                          metrics.mDisplayPort : metrics.mCriticalDisplayPort);
>      LayerMargin fixedLayerMargins(0, 0, 0, 0);
>      ScreenPoint offset(0, 0);
> +    SyncFrameMetrics(scrollOffset, treeTransformWithoutOverscroll.mScale.scale, 
> +                     metrics.mScrollableRect, mLayersUpdated, displayPort, 

Trailing space.

@@ +572,5 @@
> +    // parameter. This ensures that the overscroll transform is not unapplied,
> +    // and therefore that the visual effect applies to fixed and sticky layers.
> +    Matrix4x4 transformWithoutOverscroll;
> +    ToMatrix4x4(gfx3DMatrix(treeTransformWithoutOverscroll), transformWithoutOverscroll);
> +    transformWithoutOverscroll = transformWithoutOverscroll * aLayer->GetTransform();

I wonder if it's worth factoring this out into a function? Only 3 lines, but I don't like the idea that you could change one and forget the other... I might be being overly-paranoid, your call.

Something like 'Matrix4x4 GetTreeTransformForLayer(Layer* aLayer, gfx3DMatrix& aTreeTransform)' ?

Also, maybe things would read better if ViewTransform had a function for multiplying it with another ViewTransform and producing a gfx3DMatrix?

Comment 9

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

Comment on attachment 8439610 [details] [diff] [review]
Part 2 - Apply overscroll effect to fixed and sticky layers

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

r=me with the comment addressed. I mostly deferred to Chris on the actual fixed-position code bits.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +544,5 @@
>      // Apply the render offset
>      mLayerManager->GetCompositor()->SetScreenRenderOffset(offset);
>  
>      Matrix4x4 transform;
> +    ToMatrix4x4(gfx3DMatrix(treeTransformWithoutOverscroll) * gfx3DMatrix(overscrollTransform),

This is going to give a different result than the old code. Assuming t1 and s1 are the translation and scale matrices for normal scroll and t2 and s2 are overscroll, the old code produced (t1 * t2 * s1 * s2) whereas the new code produces (t1 * s1 * t2 * s2). Something might need to be adjusted in ApplyOverscrollEffect to account for the fact that t2 is no longer getting scaled by s1.

Comment 10

[Botond Ballo [:botond]]

Attachment: Part 2 - Apply overscroll effect to fixed and sticky layers

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> This is going to give a different result than the old code. Assuming t1 and
> s1 are the translation and scale matrices for normal scroll and t2 and s2
> are overscroll, the old code produced (t1 * t2 * s1 * s2) whereas the new
> code produces (t1 * s1 * t2 * s2). Something might need to be adjusted in
> ApplyOverscrollEffect to account for the fact that t2 is no longer getting
> scaled by s1.

Good catch! This change was causing the overscroll translation to be dampened by a factor equal to the zoom when zoomed in.

Updated to fix this, and addressed Chris' comments as well. Requesting re-view for the changed bits.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 8440918 [details] [diff] [review]
Part 2 - Apply overscroll effect to fixed and sticky layers

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

LGTM, with some nits - I wouldn't feel comfortable being the sole reviewer though, so it's a good thing kats is reviewing too :)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2097,5 @@
> +      // part of aNewTransform->mScale, this results in an overscroll
> +      // translation whose magnitude varies with the zoom. To avoid this,
> +      // we adjust for that here.
> +      aOverscrollTransform->mTranslation.x *= aNewTransform->mScale.scale;
> +      aOverscrollTransform->mTranslation.y *= aNewTransform->mScale.scale;

I guess you can't just do mTranslation *=, rather than each component separately? BasePoint has a * operator implementation, if it helps.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +162,5 @@
>     * new transform in |aNewTransform| which should be multiplied to the transform
> +   * in the shadow layer corresponding to this APZC. If the layer is
> +   * overscrolled, |aOverscrollTransform| is also set, which should also be
> +   * multiplied to the transform in the shadow layer corresponding to this
> +   * APZC in some contexts. |aOverscrollTransform| may be null.

I think this comment needs re-wording - I understand the code better by reading it than by reading this comment. Unfortunately, I don't have a suggestion of how to better explain this... :/

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +499,5 @@
>  
>    return activeAnimations;
>  }
>  
> +Matrix4x4 CombineWithCSSTransform(const gfx3DMatrix& treeTransform, Layer* aLayer)

nit, return type and function name should be on separate lines.

Comment 12

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #11)
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +2097,5 @@
> > +      // part of aNewTransform->mScale, this results in an overscroll
> > +      // translation whose magnitude varies with the zoom. To avoid this,
> > +      // we adjust for that here.
> > +      aOverscrollTransform->mTranslation.x *= aNewTransform->mScale.scale;
> > +      aOverscrollTransform->mTranslation.y *= aNewTransform->mScale.scale;
> 
> I guess you can't just do mTranslation *=, rather than each component
> separately? BasePoint has a * operator implementation, if it helps.

The splitting of this transform into the non-overscroll and overscroll portions introduces some new coordinate spaces that we don't currently model with our strongly typed units, so the units wouldn't work out to use BasePoint::operator*(ScaleFactor).

Comment 13

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

Comment on attachment 8440918 [details] [diff] [review]
Part 2 - Apply overscroll effect to fixed and sticky layers

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2087,5 @@
>  
> +    // If we are overscrolled, we would like the compositor to apply an
> +    // additional transform that produces an overscroll effect.
> +    if (aOverscrollTransform && IsOverscrolled()) {
> +      ApplyOverscrollEffect(aOverscrollTransform);

I'm not sure it really makes sense to call this function "ApplyOverscrollEffect" anymore. It probably makes more sense as "GetOverscrollTransform" or some such.

If there was an existing transform inside aOverscrollTransform that you wanted to keep, then I think the multiplication stuff you added below would need to apply only to incremental translation that resulted from the overscroll. And if this code doesn't handle an existing aOverscrollTransform then it doesn't make sense to call the function ApplyOverscrollEffect because that implies it does handle it.

Anyway, I don't really care much about this point, feel free to leave it as-is.

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 2 - Apply overscroll effect to fixed and sticky layers

(In reply to Chris Lord [:cwiiis] from comment #11)
> ::: gfx/layers/apz/src/AsyncPanZoomController.h
> @@ +162,5 @@
> >     * new transform in |aNewTransform| which should be multiplied to the transform
> > +   * in the shadow layer corresponding to this APZC. If the layer is
> > +   * overscrolled, |aOverscrollTransform| is also set, which should also be
> > +   * multiplied to the transform in the shadow layer corresponding to this
> > +   * APZC in some contexts. |aOverscrollTransform| may be null.
> 
> I think this comment needs re-wording - I understand the code better by
> reading it than by reading this comment. Unfortunately, I don't have a
> suggestion of how to better explain this... :/

I rewrote the comment. Hope I didn't make things worse!


(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> Comment on attachment 8440918 [details] [diff] [review]
> Part 2 - Apply overscroll effect to fixed and sticky layers
> 
> Review of attachment 8440918 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +2087,5 @@
> >  
> > +    // If we are overscrolled, we would like the compositor to apply an
> > +    // additional transform that produces an overscroll effect.
> > +    if (aOverscrollTransform && IsOverscrolled()) {
> > +      ApplyOverscrollEffect(aOverscrollTransform);
> 
> I'm not sure it really makes sense to call this function
> "ApplyOverscrollEffect" anymore. It probably makes more sense as
> "GetOverscrollTransform" or some such.

Agreed. Renamed.

Carrying r+.

Comment 15

[Botond Ballo [:botond]]

Having trouble pulling inbound at the moment, marking as checkin-needed.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/383ee3bc79a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/327ca8519533

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/383ee3bc79a6
https://hg.mozilla.org/mozilla-central/rev/327ca8519533

Comment 18

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

This should be uplifted to 2.0 as well to go with the rest of the overscrolling work.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8a03ce51320
https://hg.mozilla.org/releases/mozilla-aurora/rev/40567789c30a