1014280 - Show correct background color in overscrolled state

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Bug 998025 introduced the ability to overscroll with APZ.

When in an overscrolled state, a layer is translated and shrunk, creating blank space in its composited area. 

Currently, the code does not take measures to show anything in particular in this blank space. So far I've only tested it in the B2G browser on pages with a white background, and in this case the blank space is nicely white, but it's likely that in other situations, Bad Things happen like the wrong color or even garbage content being shown.

We should handle this properly by having the compositor draw a rectangle of the appropriate color before drawing the transformed layer contents.

Comment 1

[Botond Ballo [:botond]]

As an example, when overscrolling people.mozilla.org/~bballo/blue.html, the blank areas are currently white, whereas we want them to page the page's background color (blue).

Comment 2

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a background color attribute on ThebesLayer

Comment 3

[Botond Ballo [:botond]]

Attachment: Part 2 - Add an 'overscrolled' property to ThebesLayerComposite and populate it from APZ

Comment 4

[Botond Ballo [:botond]]

Remaining work: when compositing a thebes layer, if it has a background color and is overscrolled, draw a rectangle of the background color over its composition bounds prior to rendering the overscroll-transformed contents.

Comment 5

[Botond Ballo [:botond]]

Attachment: Part 2 - Fill blank areas created by overscroll transform with background color

This patch implements drawing the background color in the compositor. This approach does not require adding an 'overscrolled' property to ThebesLayerComposite, so I got rid of that patch.

Before asking for review, I'd like to improve this by making a generalization suggested by BenWa: instead of just handling Thebes layers as things that can have backgrounds, also handle Color layers (and store the property in Layer rather than in specific subclasses so that we can add handling for other layer types in the future if we'd like).

Comment 6

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a background color attribute on Layer

Updated patch to allow Layers in general, and not just ThebesLayers, to have a background color. Currently ThebesLayers and ColorLayers actually have one.

Comment 7

[Botond Ballo [:botond]]

Attachment: Part 2 - Fill blank areas created by overscroll transform with background color

Updated in accordance with modifications to Part 1, and also fixed a bug where the background rectangle would be wrong in landscape orientation.

Asking for a lot of reviews because I feel I'm a bit out of my element in the compositor code, and this patch touches a fairly core portion of the compositor code, and I don't want to mess anything up.

Comment 8

[Botond Ballo [:botond]]

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

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8431175 [details] [diff] [review]
Part 1 - Introduce a background color attribute on Layer

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

::: gfx/layers/Layers.h
@@ +1107,5 @@
>    Layer* GetMaskLayer() const { return mMaskLayer; }
>  
> +  // Layers that have a notion of a background color should override this
> +  // and return their background color here.
> +  virtual gfxRGBA GetBackgroundColor() const { return 0; }

0? gfxRGBA is 4 floats. gfxRGBA(0,0,0,0) would be more obvious. It would be nice if we had some constants for common colors.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8431176 [details] [diff] [review]
Part 2 - Fill blank areas created by overscroll transform with background color

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +248,5 @@
>                         1.0f,
>                         transform);
>  }
>  
> +Layer* GetFirstContentLayerChild(ContainerLayer* aContainerLayer)

I'd prefer GetFirstLeafLayerChild. Content is already somewhat synonymous with Thebes (see ContentClient etc).

@@ +335,5 @@
> +  // over the entire composited area before drawing the container contents.
> +  if (AsyncPanZoomController* apzc = aContainer->GetAsyncPanZoomController()) {
> +    if (apzc->IsOverscrolled()) {
> +      if (Layer* child = GetFirstContentLayerChild(aContainer)) {
> +        if (nscolor color = child->GetBackgroundColor().Packed()) {

Check GetBackgroundColor().a != 0.0 here.

@@ +337,5 @@
> +    if (apzc->IsOverscrolled()) {
> +      if (Layer* child = GetFirstContentLayerChild(aContainer)) {
> +        if (nscolor color = child->GetBackgroundColor().Packed()) {
> +          EffectChain effectChain(aContainer);
> +          effectChain.mPrimaryEffect = new EffectSolidColor(Color::FromABGR(color));

You can use ToColor from gfx2DGlue.h to avoid conversion to packed and back again.

Comment 11

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

Comment on attachment 8431176 [details] [diff] [review]
Part 2 - Fill blank areas created by overscroll transform with background color

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

I don't think I'm a qualified reviewer for this, but as far as I can tell it looks fine with the comments matt made (i.e. check the alpha instead of the whole color against 0)

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 8431175 [details] [diff] [review]
Part 1 - Introduce a background color attribute on Layer

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

I wonder if we should consolidate BackgroundColor and Color from ColorLayers?

::: gfx/layers/Layers.h
@@ +1599,5 @@
>    bool mAllowResidualTranslation;
> +  /**
> +   * The solid background color for this layer, if there is one. Zero otherwise.
> +   */
> +  gfxRGBA mBackgroundColor;

Binary packing, this should be declared before bool.

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a background color attribute on Layer

Updated to address review comments. Carrying r+.

For part 2, I will wait for the remaining reviews before addressing all comments.

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8431176 [details] [diff] [review]
Part 2 - Fill blank areas created by overscroll transform with background color

After discussions with BenWa and others, we have decided to take a different approach here. Unflagging for review.

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 1 - Add a background color attribute to scrollable ContainerLayers

This patch implements a revised approach for tracking background colors, where we do it per-{scrollable ContainerLayer}, and get it from the scrollable frame's style context directly.

When we implement multi-layer-apz (bug 967844), we are likely to change the place we store the background color from ContainerLayer to elsewhere (such as Layer or FrameMetrics).

Comment 16

[Botond Ballo [:botond]]

Attachment: Part 2 - Fill blank areas created by overscroll transform with background color

This patch is substantially the same as the previous version, so I'll carry the r+ from :mattwoodrow and :nical.

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8435172 [details] [diff] [review]
Part 1 - Add a background color attribute to scrollable ContainerLayers

>+  // Also compute and set the background color on the container.
>+  // This is needed for APZ overscrolling support.
>+  if (aScrollFrame) {
>+    aRoot->SetBackgroundColor(nsCSSRendering::FindBackgroundStyleFrame(aScrollFrame)
>+          ->StyleBackground()->mBackgroundColor);
>+  }

FindBackgroundStyleFrame is badly named, it's only for finding the root background color. I think you want FindBackground.

It'll be interesting to see if this finds a background color in enough cases or if we need something different.

Comment 18

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #17)
> It'll be interesting to see if this finds a background color in enough cases
> or if we need something different.

I tested it with the whole page having a background color, and a scrollable <div> having a background color, and it seemed to work fine in both cases.

Comment 19

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #17)
> I think you want FindBackground.

This is returning (0, 0, 0, 0) as the background color for me in some cases where FindBackgroundStyleFrame returns the correct color.

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 8435172 [details] [diff] [review]
Part 1 - Add a background color attribute to scrollable ContainerLayers

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

::: gfx/layers/Layers.h
@@ +1801,5 @@
>    float mInheritedYScale;
> +  // This is currently set and used only for scrollable container layers.
> +  // When multi-layer-apz (bug 967844) is implemented, this is likely to move
> +  // elsewhere (e.g. to Layer).
> +  gfxRGBA mBackgroundColor;

IMO putting this in the Layer base class would of been fine, but I guess this is more optimal for now so no real rush to change it.

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 8435173 [details] [diff] [review]
Part 2 - Fill blank areas created by overscroll transform with background color

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +324,5 @@
> +  // over the entire composited area before drawing the container contents.
> +  if (AsyncPanZoomController* apzc = aContainer->GetAsyncPanZoomController()) {
> +    if (apzc->IsOverscrolled()) {
> +      gfxRGBA color = aContainer->GetBackgroundColor();
> +      if (color.a != 0.0) {

I feel like you could use a comment to explain what you intent to happen for a == 0? I'm assuming if the layer has no background color then you're hoping the layers behind will provide suitable content for the over-scroll effect.

Comment 22

[Botond Ballo [:botond]]

Attachment: Part 1 using FindBackground, which is not working

This is how I'm trying to use FindBackground. It is returning (0, 0, 0, 0) for the RCD-RSF. Am I using it wrong?

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8435999 [details] [diff] [review]
Part 1 using FindBackground, which is not working

Discussed this on IRC.

Comment 24

[Botond Ballo [:botond]]

Attachment: Part 1 - Add a background color attribute to scrollable ContainerLayers

Summarizing my discussion with Timothy:

  - FindBackgroundStyleFrame() happens to work for elements which are
    not transparent, but would give the body's background color for
    elements which are transparent, which is not what we want.

  - FindBackground() does what we want in general, but it does not
    handle root scroll frames properly, so we need to special-case
    those and pass in the root frame instead.

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)
> Created attachment 8436079 [details] [diff] [review]
> Part 1 - Add a background color attribute to scrollable ContainerLayers

Carrying r+ from BenWa.

Comment 26

[Botond Ballo [:botond]]

Attachment: Part 2 - Fill blank areas created by overscroll transform with background color

Updated to address review comments. Carrying r+.

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #18)
> (In reply to Timothy Nikkel (:tn) from comment #17)
> > It'll be interesting to see if this finds a background color in enough cases
> > or if we need something different.
> 
> I tested it with the whole page having a background color, and a scrollable
> <div> having a background color, and it seemed to work fine in both cases.

The case I'm thinking of is if the scrolled content has a background color (and not the scrollable element). But we go do this approach and see if it is a problem in practice.

Comment 28

[Botond Ballo [:botond]]

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

Comment 29

[Botond Ballo [:botond]]

Green Try push:
  https://tbpl.mozilla.org/?tree=Try&rev=6a8d59b67394

Landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/1436c81443f8
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f416fc171393

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1436c81443f8
https://hg.mozilla.org/mozilla-central/rev/f416fc171393