1056423 - Scrollbars flicker when applied to vertical home screen root scroll frame

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Kevin Grandon :kgrandon]

When we change the scrollable container in the vertical home screen it seems that it causes weirdness with scrollbar rendering. When scrolling quickly through the home screen the scrollbar flickers quite a bit and is distracting.

You can apply this patch to enable scrollbars in the root scroll frame of the vertical home screen: https://github.com/mozilla-b2g/gaia/pull/21703

Comment 1

[Kevin Grandon :kgrandon]

Hey, just wondering if you guys are aware of this yet, and if anyone could help us look into it. To reproduce you can checkout the above branch and view the vertical home screen on a FxOS device. Thanks!

Comment 3

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

This is on master now; no longer need to apply the branch to reproduce it. It's quite distracting; I'll investigate.

Comment 4

[Botond Ballo [:botond]]

Jeff noticed this (or something similar) yesterday.

Comment 5

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

Just a quick update: I looked into this yesterday and was unable to determine the source of the flickering. The visible region of the scrollbar layer doesn't change in the time I see the flickering, so I don't think layout is clipping the layer (that was my original suspicion). I also did a layers dump post-compositor-culling and the effective visible region was unchanged, so it wasn't the compositor culling code trimming out parts. The culling code also wasn't skipping the layer entirely. So I'm not really sure what's going on. I'll continue investigating.

Comment 6

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

By increasing the APZ repaint interval to 5 seconds and disabling the code that applies an async transform to the scrollbars and observing the results I'm pretty sure the scrollbars are getting erroneously clipped in the compositor somewhere.

Comment 7

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

Attachment: Patch

Scrollbar clip rect needs adjusting as well in the case where the scrollbar layer is a descendant of the layer with the async transform.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8491548 [details] [diff] [review]
Patch

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +143,5 @@
> +  const nsIntRect* clipRect = aLayer->GetClipRect();
> +  if (clipRect) {
> +    gfx::Rect transformed = aTransform.TransformBounds(
> +        gfx::Rect(clipRect->x, clipRect->y, clipRect->width, clipRect->height));
> +    nsIntRect shadowClip(transformed.x, transformed.y, transformed.width, transformed.height);

Assuming it's appropriate to consider the clip rect to be in Layer pixels, this can be written a bit more concisely as:

nsIntRect shadowClip = TransformTo<LayerPixel>(LayerIntRect::FromUntyped(*clipRect)).ToUntyped();

(#incude "UnitTransforms.h" for TransformTo).

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> nsIntRect shadowClip =
> TransformTo<LayerPixel>(LayerIntRect::FromUntyped(*clipRect)).ToUntyped();

Er, that should be:

nsIntRect shadowClip = TransformTo<LayerPixel>(aTransform, LayerIntRect::FromUntyped(*clipRect)).ToUntyped();

Comment 10

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

Attachment: Patch v2

Had to add a new TransformTo to make it work, rerequesting review.

Comment 11

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

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

Comment 12

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

Forgot an #include...

https://hg.mozilla.org/integration/mozilla-inbound/rev/18be3ab7b25e

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/b5cbf93f6b48
https://hg.mozilla.org/mozilla-central/rev/18be3ab7b25e

Comment 14

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

Comment on attachment 8491571 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 995519 exposed this bug, because prior to that we didn't show scrollbars on the root scrollable frame anywhere. Now that we do, this bug appeared because the code to handle that case was wrong
[User impact if declined]: scrollbars on the root scrollframe of any B2G app or in the browser will flicker when scrolling around rapidly
[Describe test coverage new/current, TBPL]: local testing only
[Risks and why]: low-risk, code is well understood, fix is well contained
[String/UUID change made/needed]: none

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8491571 [details] [diff] [review]
Patch v2

Aurora+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5bf0e57cf5e2

Comment 17

[SandKing]

This issue has been verified successfully on Flame2.1&2.2
Verify video:"verify_1056423.mp4".
**See detial step in Comment2(bug 1067528).

Flame2.1 build:
Gaia-Rev        5655269098c7e82254e56933f1af05b4abe2a2f3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/86608c9389b5
Build-ID        20141204001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141204.034958
FW-Date         Thu Dec  4 03:50:09 EST 2014
Bootloader      L1TC00011880

Flame2.2 bulid:
Gaia-Rev        984e6d79aa799d2695f9ca132dfdc1665a56c019
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/a9fc46355661
Build-ID        20141204040202
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141204.072651
FW-Date         Thu Dec  4 07:27:05 EST 2014
Bootloader      L1TC00011880

Comment 18

[SandKing]

Attachment: Verified video: verify_1056423.mp4