Closed Bug 1036119 Opened 6 years ago Closed 6 years ago
No async scrolling in Firetext app
43.86 KB, text/plain
43.45 KB, text/plain
3.49 KB, patch
|Details | Diff | Splinter Review|
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.
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.
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.
Summary: No async scrolling in Firetext app → No overscrolling in Firetext app
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.
Summary: No overscrolling in Firetext app → No async scrolling in Firetext app
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 . 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.  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?rev=f17fb925e26e#141
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?
Assignee: nobody → botond
Attachment #8453404 - Attachment description: Have GetBaseTransform2D include the async transform → Have GetBaseTransform2D always include the async transform
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.
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 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 :)
Attachment #8453928 - Flags: review?(chrislord.net) → review+
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
[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.
blocking-b2g: --- → 2.0?
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.
blocking-b2g: 2.0? → -
You need to log in before you can comment on or make changes to this bug.