Closed Bug 1014280 Opened 6 years ago Closed 6 years ago

Show correct background color in overscrolled state

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(2 files, 9 obsolete files)

9.70 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.45 KB, patch
botond
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → 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).
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.
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).
Attachment #8428938 - Attachment is obsolete: true
Updated patch to allow Layers in general, and not just ThebesLayers, to have a background color. Currently ThebesLayers and ColorLayers actually have one.
Attachment #8428937 - Attachment is obsolete: true
Attachment #8431175 - Flags: review?(matt.woodrow)
Attachment #8431175 - Flags: review?(bgirard)
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.
Attachment #8429617 - Attachment is obsolete: true
Attachment #8431176 - Flags: review?(nical.bugzilla)
Attachment #8431176 - Flags: review?(matt.woodrow)
Attachment #8431176 - Flags: review?(bugmail.mozilla)
Attachment #8431176 - Flags: review?(bgirard)
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.
Attachment #8431175 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8431176 - Flags: review?(matt.woodrow) → review+
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)
Attachment #8431176 - Flags: review?(bugmail.mozilla) → feedback+
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.
Attachment #8431175 - Flags: review?(bgirard) → review+
Updated to address review comments. Carrying r+.

For part 2, I will wait for the remaining reviews before addressing all comments.
Attachment #8431175 - Attachment is obsolete: true
Attachment #8431698 - Flags: review+
Attachment #8431176 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8431176 - Flags: review?(bgirard)
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).
Attachment #8431698 - Attachment is obsolete: true
Attachment #8435172 - Flags: review?(tnikkel)
Attachment #8435172 - Flags: review?(mstange)
Attachment #8435172 - Flags: review?(bgirard)
This patch is substantially the same as the previous version, so I'll carry the r+ from :mattwoodrow and :nical.
Attachment #8431176 - Attachment is obsolete: true
Attachment #8435173 - Flags: review?(bgirard)
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.
(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.
(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 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.
Attachment #8435172 - Flags: review?(bgirard) → review+
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.
Attachment #8435173 - Flags: review?(bgirard) → review+
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?
Attachment #8435999 - Flags: feedback?(tnikkel)
Comment on attachment 8435999 [details] [diff] [review]
Part 1 using FindBackground, which is not working

Discussed this on IRC.
Attachment #8435999 - Flags: feedback?(tnikkel)
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.
Attachment #8435172 - Attachment is obsolete: true
Attachment #8435999 - Attachment is obsolete: true
Attachment #8435172 - Flags: review?(tnikkel)
Attachment #8435172 - Flags: review?(mstange)
Attachment #8436079 - Flags: review?(tnikkel)
(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.
Updated to address review comments. Carrying r+.
Attachment #8435173 - Attachment is obsolete: true
Attachment #8436080 - Flags: review+
(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.
Attachment #8436079 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/1436c81443f8
https://hg.mozilla.org/mozilla-central/rev/f416fc171393
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1037624
You need to log in before you can comment on or make changes to this bug.