User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 Build Identifier: If the body element of a root document specifies a background color with rgba(r,g,b,a) with a < 1 (ie (semi-)transparent) then the background drawn will be the color compose(rgba(r,g,b,a), compose(rgba(r,g,b,a), default_background_color)) where compose(foreground_color, background_color) composes two colors with alpha values. A reasonable thing to do would be to draw the background as the color compose(rgba(r,g,b,a), default_background_color). This is what Firefox 3 and Chrome do. (Although in Firefox 3 if the page loses focus the background will become a much darker shade of red). This is caused by the code in nsPresShell.cpp PresShell::Paint and nsLayoutUtils.cpp nsLayoutUtils::PaintFrame. PresShell::Paint computes an opaque color to use for a background color, nsLayoutUtils::PaintFrame adds a display item underneath everything else that draws this color that is only meant as a fallback, and then the regular draw code adds a display item for the background that is (semi-)transparent. Reproducible: Always

Maybe the simplest approach would be to simply not draw the viewport background color at all, assuming that the solid-color fallback ins PresShell::Paint and nsSubdocumentFrame::BuildDisplayList takes care of it?

Sorry, I'm not quite sure what you are suggesting. The problem is that nsDisplayCanvasBackground will draw its semi-transparent background on top of whatever is drawn by the solidcolor item added by PresShell::Paint/nsLayoutUtils::PaintFrame. Are you suggesting that we make the drawing of the background color the sole responsibility of the solidcolor item and change nsDisplayCanvasBackground (and hence nsCSSRendering::PaintBackground) so that it only draws the rest of the background (ie background-image)?

Yes.

So this was ready a long time ago, but I had to read a bunch of code just to figure out that I didn't need to make any more changes.

+ const nsStyleBackground *aBackground; Don't use 'a' for local variable names. DetermineBackgroundColorInt should be DetermineBackgroundColorInternal In PaintBackgroundWithSC, can't you just set drawBackgroundColor to false in the IsCanvasFrame case, instead of checking an extra variable? + PRBool& drawBackgroundImage, + PRBool& drawBackgroundColor, aDrawBackgroundImage/Color. And I'd prefer you use pointers so in the caller it's clear the variable can be modified. Instead of returning drawBackgroundColor as an out parameter, you can just return NS_RGBA(0,0,0,0) and have the caller check if NS_GET_A is zero. - nsIWidget* widget = view->GetNearestWidget(nsnull); - if (widget && widget->GetTransparencyMode() == eTransparencyOpaque) { Why are you taking this out? + if (NS_GET_A(bgcolor) == 255 && !nsCSSRendering::IsCanvasFrame(mFrame)) Why do we need the IsCanvasFrame check here? + * bounds aBounds. If aBounds is an empty rect (the default) then the + * bounds will be derived from the frame. aBackstopColor is composed behind + * the background color of the canvas, it is transparent by default. I don't think we should treat an empty rect as magic. Maybe make aBounds a pointer and treat null as magic? + // For printing, the paint function for nsPageFrame calls + // nsLayoutUtils::PaintFrame on its child which is nsPageContentFrame. + // We only want to add the canvas background color item once, for the + // nsPageContentFrame. + if (NS_SUCCEEDED(rv) && aFrame->GetType() != nsGkAtoms::pageFrame) { + // Add the canvas background color. + rv = aFrame->PresContext()->PresShell()->AddCanvasBackgroundColorItem( + builder, list, aFrame, nsRect(), aBackstop); + } This is a bit ugly. Can't we just stop nsPageFrame from painting its background? + PRBool aBottomLayerOnly = PR_FALSE); It's best to try to avoid optional parameters, let's make this non-optional. + if (NS_SUCCEEDED(rv)) { + // Add the canvas background color. + rv = rootFrame->PresContext()->PresShell()->AddCanvasBackgroundColorItem( + builder, list, rootFrame); + } Can we change this to add the canvas background color first? It's easier to read when items are consistently added from back to front. + // Make sure that the frame that a solidcolor item is based on gets + // invalidated for loads of the bottom layer image so that we draw the + // correct color (either mFallbackBackgroundColor or mBackgroundColor). + mPresContext->SetupBackgroundImageLoaders(aFrame, bgStyle, PR_TRUE); Actually, does it matter if we just set up all the background image loaders here? I don't think it does, what you're doing here is just a small optimization which really isn't worth it. + // Page frames don't get a solid color item. + if (aFrame->GetType() != nsGkAtoms::pageFrame) { This check is also just an optimization we don't really need. + if (!mDocument) + return; Not needed. + // If the root element of the document (ie html) has style 'display: none' + // then the document's background color does not get drawn; cache the + // color we actually draw. Can't you just check if the root element has a frame? If it's display:none, it won't. + nscolor ComputeBackstopColor(nsIView* aView); Document this. Thanks!!

I took all your changes except as noted below. (In reply to comment #7) > In PaintBackgroundWithSC, can't you just set drawBackgroundColor to false in > the IsCanvasFrame case, instead of checking an extra variable? This was my first thought too, but its a little more complicated then that. drawBackgroundColor is used in the logic and gets modified later on in the function -- its not just set at the start and used as a simple flag to tell us to draw the background color or not at the end of the function. In the end I settled on this approach because I wanted to follow the exact same path except skip the actual drawing of the background color. > Instead of returning drawBackgroundColor as an out parameter, you can just > return NS_RGBA(0,0,0,0) and have the caller check if NS_GET_A is zero. The only user of the drawBackgroundColor out parameter is PaintBackgroundWithSC and it uses it in multiple places, each one would have to be replaced by the NS_GET_A != 0 check which is a waste and would hinder readability. So PaintBackgroundWithSC still needs a drawBackgroundColor variable, and DetermineBackgroundColorInternal needs a drawBackgroundColor variable, may as well keep the symmetry with drawBackgroundImage and share them both in the same way. > - nsIWidget* widget = view->GetNearestWidget(nsnull); > - if (widget && widget->GetTransparencyMode() == eTransparencyOpaque) > { > > Why are you taking this out? Previously PresShell::Paint could handle a transparent widget but PresShell::PaintDefaultBackground could not. So I factored out the transparency logic from PresShell::Paint into ComputeBackstopColor, and used that in PresShell::PaintDefaultBackground. > + if (NS_GET_A(bgcolor) == 255 && !nsCSSRendering::IsCanvasFrame(mFrame)) > > Why do we need the IsCanvasFrame check here? Because if the item is based on a canvas frame then the item doesn't draw the background color, so it won't be opaque. > + // For printing, the paint function for nsPageFrame calls > + // nsLayoutUtils::PaintFrame on its child which is nsPageContentFrame. > + // We only want to add the canvas background color item once, for the > + // nsPageContentFrame. > + if (NS_SUCCEEDED(rv) && aFrame->GetType() != nsGkAtoms::pageFrame) { > + // Add the canvas background color. > + rv = aFrame->PresContext()->PresShell()->AddCanvasBackgroundColorItem( > + builder, list, aFrame, nsRect(), aBackstop); > + } > > This is a bit ugly. Can't we just stop nsPageFrame from painting its > background? The problem isn't that nsPageFrame is painting a background, its that nsLayoutUtils::PaintFrame will be called on nsPageFrame and its child nsPageContentFrame in the same painting of the document. nsPageFrame::BuildDisplayList adds a PageContent item and doesn't descend the frame tree at all. The Paint method of the PageContent item (nsPageFrame::PaintPageContent) calls nsLayoutUtils::PaintFrame on the child of the nsPageFrame, which is a nsPageContentFrame. So there are two calls on the stack to nsLayoutUtils::PaintFrame for one painting of the document and two solid color items get added (not to mention EnterPresShell will add the same PresShell to the mPresShellStates twice); besides screwing up translucent backgrounds, the nsPageFrame solid color item covers the entire page right to the edge of the paper except for the header area. > + if (NS_SUCCEEDED(rv)) { > + // Add the canvas background color. > + rv = > rootFrame->PresContext()->PresShell()->AddCanvasBackgroundColorItem( > + builder, list, rootFrame); > + } > > Can we change this to add the canvas background color first? It's easier to > read when items are consistently added from back to front. I assume you want this same order in the other places we do this. > + // If the root element of the document (ie html) has style 'display: none' > + // then the document's background color does not get drawn; cache the > + // color we actually draw. > > Can't you just check if the root element has a frame? If it's display:none, it > won't. Don't know why I didn't think of that.

Accidentally left "gDumpPaintList = true" in patch v2. It will be removed in the final patch.

Refresh patch due to conflicts with bug 494667. The AddCanvasBackgroundColorItem in nsLayoutUtils::PaintFrame has to be added at the end otherwise it mucks up printing.

Comment on attachment 381425 [details] [diff] [review] patch v3 + if (!FrameConstructor()->GetRootElementFrame()) + mCanvasBackgroundColor = mPresContext->DefaultBackgroundColor(); {} around this statement, since it's not break/return/continue. Thanks!!!

This needs to land after bug 494664 (otherwise the tests in this patch will fail), which is still waiting for a review.

After bug 496721 landed this became bitrotted. Refresh this patch once again.

Don't want any one to get confused by pending reviews and "needs-landing". I'm just looking for a once over from Robert on the final patch that merges with bug 496721 (removing fallback background color support). No rush.

http://hg.mozilla.org/mozilla-central/rev/23942ec68af5 Thanks Timothy!!