958549 - Settings app appears to not scroll when first opened

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

I've seen this on a Peak device using a master build. It may occur on other devices as well; I haven't checked.

STR:

1. On a B2G device with APZC enabled for gaia, start the Settings app.
2. Immediately start scrolling

Expected results:

Scroll down the settings app and scrollbar moves in sync

Actual results:

The scrollbar moves as if we're scrolling, but the content doesn't move. After a few seconds the content jumps to where it should be, and after that scrolling works fine.

Note that this is just a transient issue, and fixes itself after a couple of seconds. It's almost as if we haven't properly layerized the content and so we can't move it, except that the scrollbar *does* move which is odd.

Comment 1

[Jason Smith [:jsmith]]

Can someone confirm this on a Buri device? This sounds bad if it's reproducing on a production device.

Comment 2

[Sarah Parsons]

This issue does reproduce on the Buri BuildID: 20140110004009

Gaia   c606b129a2d1647c0fc7bfb197555026e9b27f9e
SourceStamp c5109884ae3a
BuildID 20140110004009
Version 28.0a2

Comment 3

[Chris Lord [:cwiiis]]

I see this in other apps on the Peak too, such as the Marketplace.

Comment 5

[Preeti Raghunath(:Preeti)]

1.3+ blocking for bad behavior

Comment 6

[Chris Lord [:cwiiis]]

I'm looking at this, but haven't really made any progress yet. Feel free to take if you have a good lead.

Comment 7

[Chris Lord [:cwiiis]]

Certainly the layers between when the scrolling doesn't work correctly and when it does are changing, but I've not been able to see exactly how yet - I haven't managed to get the right verbosity or find the right hooks to find out anything of major consequence yet.

Comment 8

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #7)
> Certainly the layers between when the scrolling doesn't work correctly and
> when it does are changing, but I've not been able to see exactly how yet - I
> haven't managed to get the right verbosity or find the right hooks to find
> out anything of major consequence yet.

Are you seeing a scrollable layer (in which I'm including scroll info layers) appear in the layer tree? You can see this in the layer dump as a layer which has a "[metrics= {viewport=... viewportScroll=... displayport=... ...}] attribute".

If this is the case, a potentially useful direction of investigation would be to see which scroll frame is creating this layer, and then see why it's not creating the layer initially, by looking at the condition here [1].

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2427

Comment 9

[Chris Lord [:cwiiis]]

A note: Forcing all scroll frames to create scrollable layers by altering the condition pointed to in comment #8 to disregard the 'wantLayerV/H' variables doesn't fix (or change) this.

The amount of time that the app remains unscrollable feels like it's way too long to correspond with layerisation too, though I won't say that it doesn't.

Comment 10

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

I suspect layerization is working as expected, because otherwise the scrollbar wouldn't move the way it does. I could be wrong though.

Comment 11

[Chris Lord [:cwiiis]]

It's also worth noting that disabling APZ, settings remains unscrollable for the same amount of time as with it enabled, just the scroll indicator doesn't move while you try to scroll it. I'm starting to suspect that perhaps the settings app is just doing something weird.

Comment 12

[Chris Lord [:cwiiis]]

While it seems worst on settings, I can reproduce this in basically any of the default apps (Contacts, Calendar, Dialer) that has a scrollable view - for the same period where without APZ, it wouldn't be responding, we get the same situation, except we have a moving scrollbar.

I'll take a more careful look at the layers.

Comment 13

[Chris Lord [:cwiiis]]

Assuming this is layerisation, is there anything we could do to mitigate this? Perhaps we could use the will-change/animate property as an indicator that we should build a scrollable layer? (though then we have the situation that that could be set on an element that may break scrolling... So there would be more cases the APZ code would need to handle)

Comment 14

[Chris Lord [:cwiiis]]

Ugh, so I wasted a lot of time yesterday... This is exactly layerisation, and the issue disappears if you force scroll layer items to be created instead of scroll-info. So my comment #13 stands, what do we want to do about this? I think using the will-change property makes sense to force scroll layer item creation. I'll talk to BenWa about that.

Note that, although the issue 'disappears' by forcing scroll layers, on initial scroll there is no displayport set, and so scrolling during this period just reveals blank area... If we can't immediately set a displayport on will-change layers, I expect we'll want to disable scroll-indicator movement until the layerisation happens to mitigate the weirdness.

A third issue is that the settings app is inactive for *way* too long on startup (seconds, on a Peak).

So, summary, three problems:

1- Scrollable layers in apps don't immediately build scroll layers or have displayports. We can probably use will-change/animate as a hint and fix this.
2- This will always be an issue without a hint. We should probably disable scroll indicator movement while the layer isn't scrolling.
3- Settings app takes too long to become interactive. This can likely be fixed in Gaia.

I'm going to focus on 1 and 2, I'll contact Gaia team to handle 3.

Comment 15

[Chris Lord [:cwiiis]]

Attachment: Disable scrollbar movement for empty layers

This fixes the disparity. I still think there could be something more done to make sure the main content panel in apps always has a displayport, as happens with the browser.

Comment 16

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

Comment on attachment 8363113 [details] [diff] [review]
Disable scrollbar movement for empty layers

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

r=me with nits addressed, but BenWa should probably review too.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +546,5 @@
> +{
> +  for (Layer* child = aContainer->GetFirstChild();
> +       child; child = child->GetNextSibling()) {
> +    ContainerLayer* container;
> +    if (!(container = child->AsContainerLayer()) ||

Move the assignment up a line to the variable declaration. That will make the conditional (!container || ...) which is easier to read.

@@ +582,5 @@
>        continue;
>      }
>      const FrameMetrics& metrics = scrollTarget->AsContainerLayer()->GetFrameMetrics();
> +    if (metrics.mScrollId != aLayer->GetScrollbarTargetContainerId() ||
> +        !LayerHasNonContainerDescendants(scrollTarget->AsContainerLayer())) {

Seeing as we already have three "if (..) { continue }" statements inside this loop I'd rather break this out this new clause into a fourth similar statement rather than tacking it on like this - again for readability's sake.

Comment 17

[Chris Lord [:cwiiis]]

Comments addressed and pushed to inbound: https://bugzilla.mozilla.org/show_bug.cgi?id=958549

I don't think this bug should track multiple issues, so I'm not putting leave-open. I'll discuss our current displayport behaviour with the rest of the gfx team and see if there's anything we want to do about it, but in the short term, this bug and bug 962058 will fix the issue.

Comment 18

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

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/243d1045401a

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 8363113 [details] [diff] [review]
Disable scrollbar movement for empty layers

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +569,5 @@
> +  // We only apply the transform if the scroll-target layer has non-container
> +  // children (i.e. when it has some possibly-visible content). This is to
> +  // avoid moving scroll-bars in the situation that only a scroll information
> +  // layer has been built for a scroll frame, as this would result in a
> +  // disparity between scrollbars and visible content.

Sorry for being late to review this bug.

I don't understand this comment so the non-APZC export readers will likely not understand it as well. Also I think this comment should go near the continue branch for this.

scroll information/ScrollInfoLayer

I understand that we wouldn't want scrollbars on ScrollInfoLayer layer but this doesn't seem like a good way to detect a ScrollInfoLayer? We *really* need to avoid making assumption about what a layer tree will look like. Assuming that an empty containerlayer is a ScrollInfoLayer is such an assumption that I want to avoid.

Now I don't understand the case where we get a container layer with no visible content.

Comment 22

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #21)
> Comment on attachment 8363113 [details] [diff] [review]
> Disable scrollbar movement for empty layers
> 
> Review of attachment 8363113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/AsyncCompositionManager.cpp
> @@ +569,5 @@
> > +  // We only apply the transform if the scroll-target layer has non-container
> > +  // children (i.e. when it has some possibly-visible content). This is to
> > +  // avoid moving scroll-bars in the situation that only a scroll information
> > +  // layer has been built for a scroll frame, as this would result in a
> > +  // disparity between scrollbars and visible content.
> 
> Sorry for being late to review this bug.
> 
> I don't understand this comment so the non-APZC export readers will likely
> not understand it as well. Also I think this comment should go near the
> continue branch for this.
> 
> scroll information/ScrollInfoLayer
> 
> I understand that we wouldn't want scrollbars on ScrollInfoLayer layer but
> this doesn't seem like a good way to detect a ScrollInfoLayer? We *really*
> need to avoid making assumption about what a layer tree will look like.
> Assuming that an empty containerlayer is a ScrollInfoLayer is such an
> assumption that I want to avoid.
> 
> Now I don't understand the case where we get a container layer with no
> visible content.

It's not really a way of detecting ScrollInfoLayer so much as a way of detecting if a layer has anything visible on the screen. If a ContainerLayer only contains ContainerLayers or nothing, it's a reasonable assumption that we don't want to draw scroll-bars for it, I think. This happily happens to correspond to the case where we have a ScrollInfoLayer.

This patch has already landed, a week ago now - if there are issues, please file follow-up bugs.

Comment 23

[Benoit Girard (:BenWa)]

I guess I can let this one go. We should know when we're building the layer if we want the thing to scroll and identify it properly.

What we're doing here is we're not identifying things correctly and we're looking for a proxy (no visible content) and using that to hide the scroll bar. I don't think that's a good fix.