1036119 - No async scrolling in Firetext app

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Attachment: Display list dump

STR [copied from https://bugzilla.mozilla.org/show_bug.cgi?id=967844#c10]:
------------------------
1) Install https://marketplace.firefox.com/app/firetext.
2) Click the menu icon in the top left corner.
3) Click the settings icon in the bottom left corner of the sidebar.
4) Try to use overscroll on the settings page.

Expected result:
Overscroll works fine ^^

Actual result:
No overscroll
------------------------

We looked at a display list dump with Timothy, and it seems that the scroll frame for which we are creating a scroll layer are being correctly merged, and therefore that this is a different issue than bug 967844.

See also bug 1036100 which is about continuous repainting in this app; I'm not sure if they are related.

Comment 1

[Botond Ballo [:botond]]

My statement in https://bugzilla.mozilla.org/show_bug.cgi?id=967844#c11 that the inner APZC does not have a scroll range was incorrect. It does hav a scroll range, of 672 CSS pixels, and a composition bounds of 516 CSS pixels, which is consistent with what I'm seeing on the phone and in App Manager, i.e. this is the APZC that we want to be async-scrolling. The question remains, why is it not.

Comment 2

[Botond Ballo [:botond]]

Further investigation reveals that we are doing async scrolling, it just seems like sync scrolling because the app is slow due to all the repaints (bug 1036100). However, we are not overscrolling for some reason. I will continue to investigate.

Comment 3

[Botond Ballo [:botond]]

Further investigation reveals:

  - We are, after all, doing sync scrolling of the settings screen, not async scrolling.
    My previous assertion that we were doing async scrolling was based on testing for
    the wrong thing. My mistake.

  - The reason we are doing sync scrolling is that the scrollable layer is fixed-position,
    and the fixed-position transform cancels out the async scrolling transform in the
    compositor. This makes this issue very much like the one in bug 999021, and in fact
    the Option B patch there fixes this issue, although the page still sync-scrolls, this
    time due to bug 967844 (multi-layer-apz).
    (Note that this suggests that the slowness Kip mentions in 
    https://bugzilla.mozilla.org/show_bug.cgi?id=999021#c29 is also due to falling back
    to sync scrolling because of multi-layer-apz.)

Therefore, one way to resolve this bug is to land the Option B patch in bug 999021, and also fix multi-layer-apz.

However, Kats suggested on IRC that we may be able to apply a different fix than the current Option B patch in bug 999021, which avoids running into multi-layer-apz, and thus achieves async scrolling without having to fix multi-layer-apz.

Comment 4

[Botond Ballo [:botond]]

Attachment: Display list dump with Option B fix from bug 999021 applied

Comment 5

[Botond Ballo [:botond]]

Some further updates:

  - multi-layer-apz would in fact remove the code the Option B patch in bug 999021
    touches, so if that patch just causes us to hit multi-layer-apz, there isn't
    really any point to it. Also, Timothy indicated on IRC that giving position:fixed
    elements their own ContainerLayer is problematic for similar reasons that
    giving scrollable elements their own ContainerLayer is (i.e. multi-layer-apz).

  - Next, instead I investigated the Option A patch in bug 999021. It turns out
    that it fixes the problem only by accident, and causes other problems. After
    some debugging, I discovered that the bug is in TranslateShadowLayer2D [1].
    This function
      1. queries the *GetTransform()* of a layer via GetBaseTransform2D()
      2. adjusts this transform by a translation
      3. sets the *shadow transform* to the resulting adjusted matrix
    GetTransform() and the shadow transform differ by the async transform, so
    by getting the GetTransform() but setting the shadow transform, this
    function effectively drops the async transform even if the translation is
    (0, 0).

    I believe it is this dropping of the async transform which causes this bug 
    and bug 999021.

    The Option A patch causes the code inside AlignFixedAndStickyLayers (AFS)
    which calls TranslateShadowLayer2D, to not run for some calls to AFS.
    This has the effect of not dropping the async transform (good) but also of
    not making the translation adjustment which is the purpose of
    TranslateShadowLayer2D (bad).

    I believe the proper solution is to fix TranslateShadowLayer2D so that
    it does not drop the async transform.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?rev=f17fb925e26e#141

Comment 6

[Botond Ballo [:botond]]

Attachment: Have GetBaseTransform2D always include the async transform

This is a first attempt at fixing TranslateShadowLayer2D as described in the previous comment, by simply modifying GetBaseTransform2D to always include the async transform.

This patch fixes this bug, but I'm not sure if it breaks other things.

Chris, what do you think - is this too big of a hammer? Is the logic in GetBaseTranform2D where it includes the async transform in some cases but not others, there for a good reason? Should I try a more targeted fix instead, where I leave GetBaseTransform2D alone, but instead modify TranslateShadowLayer2D to call something else that always includes the async transform?

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 8453404 [details] [diff] [review]
Have GetBaseTransform2D always include the async transform

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

I'm uncertain what effect this change would have at a glance, but my initial thought is that wouldn't this break successive composites with async scrolling (when there's no content render between)? You'd end up async-transforming the async-transform, wouldn't you?

If the above isn't correct, I'll look at this in more detail.

Comment 8

[Botond Ballo [:botond]]

Attachment: Have AsyncCompositionManager support layers that are both scrollable and fixed-position

During an IRC discussion with Chris, it became clear that my first solution attempt was quite a bit off the mark :)

Now that, with Chris' help, I understand what is going on a bit better, let me try to describe the problem more accurately:

    AsyncCompositionManager::ApplyAsyncContentTransformToTree sets a
    shadow transform on a layer for two reasons:

      - For scrollable layers, a transform for the async scroll amount.
        Let's call this 'Scroll'.

      - For fixed-position layers, a transform to compensate for the
        async scroll amount of the nearest scrollable ancestor layer.
        Let's call this 'Fixed'.

    Let's call the whole shadow transform 'Shadow'.

    Currently, for scrollable layers we do:

        Shadow = Scroll

    and for fixed layers:

        Shadow = Fixed

    For a layer which is both scrollable and fixed, we do:

        Shadow = Scroll   // earlier in the code
        Shadow = Fixed    // later in the code

    The result is that Shadow == Fixed, which is wrong because we've
    clobbed Scroll. What we want is Shadow == Scroll + Fixed.

    My first attempt changed the setting of Fixed to do

        Shadow += Fixed

    instead of

        Shadow = Fixed

    This fixed the problem for layers which are both scrollable
    and fixed, but for layers which are only fixed, and the code
    'Shadow = Scroll' never runs, we just keep adding Fixed
    to Shadow on every frame and Shadow keeps growing, which is
    wrong.

    This patch adds code to reset Shadow to 0 at the beginning
    of every frame, so that 'Shadow += Fixed' doesn't clobber
    'Scroll' if it's there, but also doesn't compound with
    itself over multiple frames.

Comment 9

[Botond Ballo [:botond]]

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

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 8453928 [details] [diff] [review]
Have AsyncCompositionManager support layers that are both scrollable and fixed-position

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

Yup, this looks sound to me :)

Comment 11

[Botond Ballo [:botond]]

Attachment: Have AsyncCompositionManager support layers that are both scrollable and fixed-position

The Try push in comment 9 had a number of reftest failures.

After several attempts at fixing them, each yielding other reftest failures, I finally realized the problem was that to clear the async transform I was doing:

    SetShadowTransform(GetTransform())

but this is wrong, since GetTransform() includes the pre- and post-scale, but the shadow transform does not. Changing this to:

    SetShadowTransform(GetBaseTransform())

fixed all the failures. Carrying r+ for this change.

Successful Try push: https://tbpl.mozilla.org/?tree=Try&rev=5be659091669

Comment 12

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd974f99638b

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/bd974f99638b

Comment 14

[Botond Ballo [:botond]]

[Blocking Requested - why for this release]:

[Feature/regressing bug #]: 
  The issue with the page not async scrolling has probably always been there,
  but overscrolling (bug 1020045) made it much more noticeable.

[User impact if declined]: 
  Some pages do sync scrolling instead of async scrolling. Sync scrolling is
  slower / less smooth, and has no overscrolling.

[Describe test coverage new/current, TBPL]: 
  On master for 3+ weeks.

[Risks and why]: 
  Touching AsyncCompositionManager code is not particularly low-risk, but this 
  patch has been on master for 3+ weeks without any reported regressions.

[String/UUID change made/needed]: 
  None.

Comment 15

[Andrew Overholt [:overholt]]

I spoke with Botond about this and it's difficult to know how common scrollable position:fixed elements are on the web.  In the particular case here of Firetext, it doesn't seem to be a regression and while it creates poor scrolling performance, it doesn't stop a user from using the app.