Closed
Bug 1014280
Opened 11 years ago
Closed 10 years ago
Show correct background color in overscrolled state
Categories
(Core :: Panning and Zooming, defect)
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 | ||
Updated•11 years ago
|
Assignee: nobody → botond
Assignee | ||
Comment 1•11 years ago
|
||
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).
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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
Assignee | ||
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
green try |
Comment 9•11 years ago
|
||
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 10•11 years ago
|
||
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 11•11 years ago
|
||
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 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8431176 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 14•11 years ago
|
||
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)
Updated•11 years ago
|
Blocks: apz-overscroll
Assignee | ||
Comment 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
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 17•11 years ago
|
||
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.
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
(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•10 years ago
|
||
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 21•10 years ago
|
||
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+
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8435999 [details] [diff] [review]
Part 1 using FindBackground, which is not working
Discussed this on IRC.
Attachment #8435999 -
Flags: feedback?(tnikkel)
Assignee | ||
Comment 24•10 years ago
|
||
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)
Assignee | ||
Comment 25•10 years ago
|
||
(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.
Assignee | ||
Comment 26•10 years ago
|
||
Updated to address review comments. Carrying r+.
Attachment #8435173 -
Attachment is obsolete: true
Attachment #8436080 -
Flags: review+
Comment 27•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8436079 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 28•10 years ago
|
||
try |
Assignee | ||
Comment 29•10 years ago
|
||
green landing try |
Comment 30•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1436c81443f8
https://hg.mozilla.org/mozilla-central/rev/f416fc171393
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•