Closed Bug 572613 Opened 10 years ago Closed 10 years ago

Move fallback SolidColor display item into the canvas background display item

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(1 file)

Attached patch fixSplinter Review
The layers system gets confused when we have a nonscrollable solid color behind the scrollable canvas background. I need to fuse the nonscrollable viewport fallback background color with the canvas background when possible. The attached patch does this by giving nsCanvasBackgroundDisplayItem the ability to paint an extra solid color, and by making AddCanvasBackgroundColorItem locate a suitable nsCanvasBackgroundDisplayItem and poke the solid color directly into that item if possible. This means we don't need to build the SolidColor display item at all in most cases.
Attachment #451832 - Flags: review?(tnikkel)
Whiteboard: [needs review]
Comment on attachment 451832 [details] [diff] [review]
fix

Should probably add a comment to all the other AddCanvasBackgroundColorItem call sites that it is called after the main display list is constructed for a reason like you have in nsFrameFrame.cpp (and check that it is indeed called after in all places).

Can you update the comment in nsCSSRendering::PaintBackgroundWithSC above "PRBool isCanvasFrame = ..." saying that the background color is drawn by the canvas background item or solid color item?

AddCanvasBackgroundColor returns a bool, I think you should check that in AddCanvasBackgroundColorItem, and use the regular path if it returns false.


+  virtual PRBool IsUniform(nsDisplayListBuilder* aBuilder, nscolor* aColor)
+  {
+    nscolor background;
+    if (!nsDisplayBackground::IsUniform(aBuilder, &background))
+      return PR_FALSE;
+    *aColor = NS_ComposeColors(mExtraBackgroundColor, background);
+    return PR_TRUE;
+  }

If everything is working right then I think mExtraBackgroundColor should equal background, and we should be drawing just one, not the composition of both.

r=tnikkel if you agree with these comments.
Attachment #451832 - Flags: review?(tnikkel) → review+
(In reply to comment #1)
> If everything is working right then I think mExtraBackgroundColor should equal
> background, and we should be drawing just one, not the composition of both.

Oops, I didn't see how nsDisplayBackground::IsUniform works. Can you just assert that background is completely transparent and not compose?
Whiteboard: [needs review]
(In reply to comment #1)
> Should probably add a comment to all the other AddCanvasBackgroundColorItem
> call sites that it is called after the main display list is constructed for a
> reason like you have in nsFrameFrame.cpp (and check that it is indeed called
> after in all places).

Done. There's only one other, in nsLayoutUtils::PaintFrame.

> Can you update the comment in nsCSSRendering::PaintBackgroundWithSC above
> "PRBool isCanvasFrame = ..." saying that the background color is drawn by the
> canvas background item or solid color item?

Done.

> AddCanvasBackgroundColor returns a bool, I think you should check that in
> AddCanvasBackgroundColorItem, and use the regular path if it returns false.

Yes indeed. Done.

> Oops, I didn't see how nsDisplayBackground::IsUniform works. Can you just
> assert that background is completely transparent and not compose?

Yes. Done.

Still sr+ with that?
Sounds good, the r=tnikkel is still valid!
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/34d7e0561e5b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Depends on: 582146
You need to log in before you can comment on or make changes to this bug.