Closed
Bug 235189
Opened 21 years ago
Closed 3 years ago
canvas background color should not be painted if the root element has an appearance.
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: p_ch, Unassigned)
References
Details
Attachments
(1 file, 2 obsolete files)
|
27.53 KB,
patch
|
bzbarsky
:
review-
|
Details | Diff | Splinter Review |
this is a follow up of bug 119735 comment 8.
the patch I will attach makes FindBackground return a Frame instead of a style
background, so that we can paint the appearance of the root element for the canvas.
FindBackground is also used by nsPresContext::FindFrameBackground, but this is
dealt in bug 235187.
In some case, I wasn't sure if I should do more changes to be more consistent
with the background frame (the real frame used to get the style background and
display) returned by FindBackground. I then just added a comment, since I think
the way we paint the canvas should be much more simplified.
I've also left the style background argument in PaintBackgroundWithSC, since
there is a subtelty in the non translucent case: the canvas background color is
derived from the presContext DefaultBackgroundColor.
|
Reporter | |
Comment 1•21 years ago
|
||
|
|
Reporter | |
Comment 2•21 years ago
|
||
|
|
Reporter | |
Updated•21 years ago
|
Attachment #141944 -
Flags: superreview?(roc)
Attachment #141944 -
Flags: review?(bzbarsky)
|
||
Comment 3•21 years ago
|
||
I won't be able to get to this in the foreseeable future (a week or so at least).
|
|
Reporter | |
Comment 4•21 years ago
|
||
Comment on attachment 141944 [details] [diff] [review]
patch v1.0
bz: np.
Cancelling reviews... I forgot to update the comments in nsCSSRendering.h and
eventually in other places. I'll update that shortly this week
Attachment #141944 -
Flags: superreview?(roc)
Attachment #141944 -
Flags: review?(bzbarsky)
|
|
Reporter | |
Comment 5•21 years ago
|
||
Here is the patch with comments. I removed back a return in the middle of
FindCanvasBackground, to avoid destructor duplication.
reviewers, be careful about the use of the input frame or the returned bgFrame
that that is used to paint the background. I hope I haven't done mistakes.
|
|
Reporter | |
Updated•21 years ago
|
Attachment #141944 -
Attachment is obsolete: true
Attachment #141945 -
Attachment is obsolete: true
|
|
Reporter | |
Updated•21 years ago
|
Attachment #142650 -
Flags: superreview?(roc)
Attachment #142650 -
Flags: review?(bzbarsky)
Comment on attachment 142650 [details] [diff] [review]
patch v1.1
+ const nsStyleBackground* scrollFrameBG =
scrollFrame->GetStyleBackground();
Should be "= bgScrollFrame->" I think
+ PaintBackgroundWithSC(aPresContext, aRenderingContext, bgFrame,
So this doesn't really matter because I think bgFrame == aForFrame here, but I
think this should be passing aForFrame since it's aForFrame's geometry that is
relevant to the painting here.
+ PaintBackgroundWithSC(aPresContext, aRenderingContext, bgFrame,
Ditto here. But here it *is* relevant. I think as it stands, a repeating
background image on <BODY> propagated up to <HTML> is going to be aligned
incorrectly. on the other hand, I guess sending the bgFrame here is the point
of this patch. Hmm... evil. Does it work if you pass in aForFrame to
PaintBackgroundWithSC, but pass in a separate appearance style as an extra
parameter, so you can pass in bgFrame's appearance?
+ * to paint the background of the running frame.
+ * Returns true if the background of the running frame should be painted.
Just say "aForFrame" instead of "the running frame".
Attachment #142650 -
Flags: superreview?(roc)
|
|
||
Comment 7•21 years ago
|
||
Comment on attachment 142650 [details] [diff] [review]
patch v1.1
>Index: layout/html/base/src/nsContainerFrame.cpp
> SyncFrameViewGeometryDependentProperties(nsIPresContext* aPresContext,
>+ const nsStyleBackground* bg;
Init that to nsnull, please.
> PRBool viewHasTransparentContent =
> !(hasBG && !(bg->mBackgroundFlags & NS_STYLE_BG_COLOR_TRANSPARENT) &&
> bg->mBackgroundClip == NS_STYLE_BG_CLIP_BORDER &&
>- aFrame->CanPaintBackground() &&
>+ bgFrame->CanPaintBackground() &&
> !HasNonZeroBorderRadius(aStyleContext));
Why is that using bgFrame instead of aFrame? (I doubt they're ever different
in the cases when this returns false, but this deserves a comment in the code
at the very least to explain the reasoning).
>+ const nsStyleBackground* scrollFrameBG = scrollFrame->GetStyleBackground();
What roc said.
>Index: layout/html/style/src/nsCSSRendering.cpp
>+ * to paint the background of the running frame. It is responsible for handling
Again, what roc said.
>+FindCanvasBackground(nsIPresContext* aPresContext, nsIFrame* aForFrame,
You changed the logic here, no? It used to be that the firstchild background
would get returned even if it was transparent, as long as there was no HTML
<body> around. With your change, the aBGFrame is aForFrame in cases when the
first child background is transparent and there is no HTML body. I'm not sure
that's such a good idea...
> nsCSSRendering::PaintBackground(nsIPresContext* aPresContext,
>+ PaintBackgroundWithSC(aPresContext, aRenderingContext, bgFrame,
>+ aDirtyRect, aBorderArea,
>+ *bgFrame->GetStyleBackground(), aBorder,
> aPadding, aUsePrintSettings);
roc is right. This will screw up positioning of background images. It will
also screw up canvas invalidation (since we will invalidate bgFrame instead of
aForFrame as the background image loads, and hence won't invalidate a big
enough area).
You really want to be passing in both of the frames here, I would say.
>+ PaintBackgroundWithSC(aPresContext, aRenderingContext, bgFrame,
> aDirtyRect, aBorderArea, canvasColor,
> aBorder, aPadding, aUsePrintSettings);
Same.
>Index: layout/xul/base/src/nsBoxFrame.cpp
>+ const nsStyleBackground* bg;
Init to null, please.
Attachment #142650 -
Flags: review?(bzbarsky) → review-
|
||
Comment 9•3 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months.
:dholbert, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee: p_ch → nobody
Flags: needinfo?(dholbert)
|
||
Comment 10•3 years ago
|
||
Skimming the 18-year-old comments here, it's not clear to me what this change was about or whether there's still anything to be fixed here (likely not).
Let's close as INCOMPLETE and we can reopen or file a new bug if there's anything still valid here.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(dholbert)
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•