1040087 - Overscroll flickers when scrollgrab used on pages with a transparent background

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Ben Francis [:benfrancis]]

STR:
* Create a certified app with an iframe with a src of google.com wrapped in a black div with scrollgrab turned on

<div id="wrapper">
  <iframe src="google.com">
</div>

wrapper.scrollgrab = true;

wrapper {
  overflow: scroll;
}

* Overscroll the iframe


Alternative STR:
* Apply Gaia patch from bug 1039519 (or checkout Gaia feature branch at https://github.com/benfrancis/gaia/tree/1039519).
* Build Gaia with HAIDA=1 (warning: the UI in this build is going to look pretty broken, but will allow you to reproduce this bug).
* Tap the Rocketbar
* Type google.com
* Wait for Google to load
* Overscroll Google

Expected:
* Solid colour behind the transformed content

Actual:
* Flicker as the inner iframe and outer div appear to compete for overscroll



Video of flicker: https://www.youtube.com/watch?v=wSQyE1g2sFk

Comment 1

[Chris Lord [:cwiiis]]

So with my more recent version of bug 1039519, I get no flickering, but it does seem to overscroll the wrong layer. I don't know if this bug is still useful/valid anymore, I'll do some investigation and talk to Botond.

Comment 2

[Chris Lord [:cwiiis]]

I'll reopen if I encounter this again, but things look fine atm.

Comment 3

[Chris Lord [:cwiiis]]

Ok, this was masked by bug 1045520 regressing, but the issue is still there.

My suggestion as to why this happens: A page has transparent background colour, so the overscroll colour sees through to whatever's behind it. Layout correctly optimises away the layer behind the page, so you see through to black.

I think there are possibly two bugs here:
- That it flickers white at all shows that there's a layer there that shouldn't be there. Either that, or we arbitrarily pick our GL clear colour/don't clear/get left in some inconsistent state(?) More likely the latter, thinking about it.
- That it goes to black suggests that when there's an alpha component to the background colour, we should recurse further up the layer tree and blend with the next available background colour, up to either alpha saturation or the root layer, whichever comes first.

You can see this bug by checking out this branch: https://github.com/vingtetun/gaia/tree/smooth.animation.all, going to google.com via the homescreen search bar and overscrolling by scrolling up (dragging down).

Botond, do you have any time to look at this? I realise you're probably pretty tied up as it is, but I'll ask anyway.

Comment 4

[Chris Lord [:cwiiis]]

Similar to this;

- Open a site with a background colour (for the sake of argument/consistency, say planet.gnome.org)
- Collapse the rocketbar by scrolling down (swiping up)
- Expand the rocketbar very quickly by making a quick scroll-up gesture (swipe down quickly)

Expected:

- The exposed space should be the background colour of the browser container.

Actual:

- The exposed space's colour oscillates between that background colour and black.


Note that the branch currently doesn't set a background colour on the browser container, so you'll need to open up the inspector and add one (or wait for the branch to be updated).

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Chris, how does this fit in priority compared to bug 1042974?

Comment 6

[Chris Lord [:cwiiis]]

(In reply to Milan Sreckovic [:milan] from comment #5)
> Chris, how does this fit in priority compared to bug 1042974?

This is definitely more important than bug 1042974 - We could easily live without that with the current method of showing/hiding the rocketbar (though it's still a would-be-nice).

As far as I can tell, there's not really any way to work around this issue.

Comment 7

[Chris Lord [:cwiiis]]

Just an update on reproducing this; You can now reproduce this on master (ae7e0f7891b67513a3a71b69dbc1da15f648407b) without altering anything. Just go to google.com via the homescreen search bar and drag down to overscroll.

Slightly more worrying to me is that if you go to the homescreen, then back to the browser via the task switcher, instead of flickering between black and white (the background colour), it flickers between the wallpaper and white. This suggests to me that we're always drawing the wallpaper underneath the browser, which unless we're clipping it (and not stencil-clipping, mind), is probably terrible for perf.

I'll have a look at that, it's probably a Gaia bug and something we can fix outside the scope of this bug.

Comment 8

[Chris Lord [:cwiiis]]

huh, that said, the element that the background comes from doesn't appear to change, I wonder if this is some kind of rounding issue...

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's see if we can fix this in 34.

Comment 10

[Botond Ballo [:botond]]

I'm not sure if this is related, but I notice the layers of the Homescreen app are being retained while viewing google.com. I filed bug 1047058 for this.

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> I'm not sure if this is related, but I notice the layers of the Homescreen
> app are being retained while viewing google.com. I filed bug 1047058 for
> this.

The bug identified as regressing bug 1047058 - bug 1042744 - is also what introduced the use of 'scrollgrab'. I'm therefore inclined to wait until bug 1047058 is resolved before investigating this further.

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> The bug identified as regressing bug 1047058 - bug 1042744 - is also what
> introduced the use of 'scrollgrab'. I'm therefore inclined to wait until bug
> 1047058 is resolved before investigating this further.

Bug 1047058 was resolved as a duplicate of bug 1047390. This fix for bug 1047390 does not solve this issue, so I will continue investigating.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Ok what I observe in current master is that If I minimized the rocketbar and pan quickly to the bottom to see it, then there is a 'black' flash. This black colour is the color of the div.fade-overlay element of the homescreen behind the current app window.

If I change this color to red with:
.homescreen .fade-overlay {
  background-color: red !important;
}

Then I see a red flash.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #13)
> Ok what I observe in current master is that If I minimized the rocketbar and
> pan quickly to the bottom to see it, then there is a 'black' flash. This
> black colour is the color of the div.fade-overlay element of the homescreen
> behind the current app window.
> 
> If I change this color to red with:
> .homescreen .fade-overlay {
>   background-color: red !important;
> }
> 
> Then I see a red flash.

Not sure this is exactly the same issue but in case this is, and for the one I see I have a Gaia workaround in bug 1047829

Comment 15

[Botond Ballo [:botond]]

I investigated this some more. Here's what I've found:

  - The background flashes between grey and white.

  - There is a grey ColorLayer in the layer tree, and a white ColorLayer.
    The white ColorLayer is on top. The browser iframe's layer, in turn,
    is on top of the white ColorLayer.

  - The grey ColorLayer is always drawn.

  - The white ColorLayer is sometimes drawn, sometimes not. When it's
    drawn, we see white. When it's not, we see grey.

  - The reason the white ColorLayer is sometimes not drawn, is that
    code in ContainerPrepare [1] determines that the layer on top
    of it (which is the RefLayer for the browser iframe), occludes
    it completely. (This determination does not take into account
    the overscroll transform applied to the root layer inside the
    browser iframe).

    The reason this doesn't always happen is that this optimization
    only kicks in when the layer's effective transform is the identity.
    Sometimes, the effective transform is a little bit off from the
    identity (for example, [ 1 0; 0 1; -3.57628e-07 0; ]), probably
    due to rounding / floating-point error somewhere.

The way I look at it, this is a legitimate optimization on the part of the compositor, and the real problem is that we are misleading the compositor into thinking that browser iframe's layer is opaque, when as a result of the overscroll transform we only draw to part of its composited region.

I can think of two approaches for solving this:

  1) Have the compositor take into account the overscroll effect when
     reasoning about opacity. This would ensure that the optimization
     described above is never triggered in this case.

  2) Always draw a background for the overscroll effect, even if we
     have to pick an arbitrary color like white to draw.

I lean towards (2) because it's simpler, and because it allows us to control what color we show as the background, rather than having it be whatever's underneath (which might then need to be tweaked in different scenarios). I will discuss with Timothy what is the best way to choose a background color to draw.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp?rev=b072e506b81b#339

Comment 16

[Botond Ballo [:botond]]

Attachment: bug1040087.patch

This patch implements option (2) by using the same solid background color for a transparent root content document that we choose in PresShell::UpdateCanvasBackground().

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8468789 [details] [diff] [review]
bug1040087.patch

Is there a reason we want to limit this to only root content documents? The behaviour seems reasonable for all documents (even if presShell->GetCanvasBackground() will not be opaque in those cases), are am I missing something?

Can we simplify this and just unconditionally use presShell->GetCanvasBackground() for all root scroll frames? And not bother with the check if the color we get is not opaque, and we can also avoid the conditional on isRootScrollFrame for backgroundFrame.

Comment 18

[Botond Ballo [:botond]]

Attachment: bug1040087.patch

Sounds reasonable, and doesn't appear to break anything.

Comment 19

[Botond Ballo [:botond]]

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

Comment 20

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #19)
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=af1ad3440b27

I think that looks as good as it's going to given the Try problems we're having.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2617a272e282

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2617a272e282