Closed
Bug 438987
Opened 16 years ago
Closed 16 years ago
root element table background not propagated to viewport
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(3 files, 1 obsolete file)
257 bytes,
text/html
|
Details | |
13.54 KB,
patch
|
roc
:
review+
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
16.26 KB,
patch
|
Details | Diff | Splinter Review |
The code in FindBackground/FindCanvasBackground/FindElementBackground is clumsy. Essentially it pokes around the frame tree trying to find the root element frame whose background should be propagated to the viewport --- both to draw that background in the canvas frame, and to avoid drawing that background in the root element's frame. We can simplify the code and speed it up by just storing in nsCSSFrameConstructor the frame whose background should be propagated --- the frame for the root element whose style context is the primary style context for the element. This will let us fix a bug where if the root element is a table, we look for the background on the outer table frame, which doesn't have the background.
Assignee | ||
Comment 1•16 years ago
|
||
Fix as described.
Attachment #324905 -
Flags: superreview?(dbaron)
Attachment #324905 -
Flags: review?(dbaron)
Comment 2•16 years ago
|
||
The patch no longer applies cleanly and mq had trouble with the reftests.
Attachment #324905 -
Attachment is obsolete: true
Attachment #330298 -
Flags: superreview?
Attachment #330298 -
Flags: review?
Attachment #324905 -
Flags: superreview?(dbaron)
Attachment #324905 -
Flags: review?(dbaron)
Updated•16 years ago
|
Attachment #330298 -
Flags: superreview?(dbaron)
Attachment #330298 -
Flags: superreview?
Attachment #330298 -
Flags: review?(roc)
Attachment #330298 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #330298 -
Flags: review?(roc) → review+
So how does mRootElementStyleFrame get set to the inner table when the root element is a table? It looks more like it will end up being null. Or do we call ConstructDocElementFrame twice? The way you set mRootElementStyleFrame also doesn't seem very future-proof; what if we end up with other transformations in the future? (This doesn't affect the code that propagates scrollbar styles, does it? Since we have to decide whether they're propagated before we decide what frame to construct. The propagation of scrollbar styles should work pretty much the same way, but I guess that's ok.)
Assignee | ||
Comment 4•16 years ago
|
||
(In reply to comment #3) > So how does mRootElementStyleFrame get set to the inner table when the root > element is a table? It looks more like it will end up being null. Or do we > call ConstructDocElementFrame twice? contentFrame->GetParentStyleContextFrame will set it to the inner table because that's the outer table frame's style context parent. > The way you set mRootElementStyleFrame also doesn't seem very future-proof; > what if we end up with other transformations in the future? How much more future-proof could it be? I presume that any such transformations would have to update GetParentStyleContextFrame. At least I've moved the logic out from nsCSSRendering to frame construction. > (This doesn't affect the code that propagates scrollbar styles, does it? Since > we have to decide whether they're propagated before we decide what frame to > construct. The propagation of scrollbar styles should work pretty much the > same way, but I guess that's ok.) That's unaffected. It will work correctly already, because it uses styleSet->ResolveStyleFor(docElement) instead of looking at frames so it's not affected by table frame weirdness.
Ah, I missed that you were assigning to mRootElementStyleFrame in the call to GetParentStyleContextFrame; I thought it was only being used for the isChild output.
Comment on attachment 330298 [details] [diff] [review] Unbitrotted patch >+ // This can be called even when there's no root element yet, during frame >+ // construction. >+ if (!aRootElementFrame) >+ return PR_TRUE; How could we be painting a body element if there's no root element?
Comment on attachment 330298 [details] [diff] [review] Unbitrotted patch r+sr=dbaron if you: 1) remove the snippet I quoted above or explain better (in comments) why it needs to be there 2) preferably add some reftests for the print case as well (and include a != test to make sure that backgrounds work at all)
Attachment #330298 -
Flags: superreview?(dbaron) → superreview+
Attachment #330298 -
Flags: review+
It might also be good to add a comment in the place that confused me, something like: // else we already assigned the correct value in the GetParentStyleContextFrame call above
Assignee | ||
Comment 9•16 years ago
|
||
We're not painting, we're doing something during frame construction that results in FindBackground being called. I forget the exact call chain, but it was probably nsCSSFrameConstructor calling nsHTMLContainerFrame::CreateViewForFrame calling nsContainerFrame::SyncFrameViewProperties calling SyncFrameViewGeometryDependentProperties calling nsLayoutUtils::FrameHasTransparency calling nsCSSRendering::FindBackground. I'll add those comments and tests.
Is it OK that FrameHasTransparency does the wrong thing in that case?
Assignee | ||
Comment 11•16 years ago
|
||
I think so, because eventually the frame will be reflowed which calls SyncFrameViewProperties again.
Assignee | ||
Comment 12•16 years ago
|
||
I pushed 9628f4628ed4. However, I haven't added reftests for printing. Currently reftest-print honors the preference to print backgrounds (or not). Should we should always enable printing of backgrounds for reftest-print?
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•16 years ago
|
||
I backed this out, it crashed in crashtests, and in reftests locally.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•16 years ago
|
||
This patch fixes the crashes. We need to null out mRootElementStyleFrame when an XBL binding is applied to the root element and we call ReconstructDocElementHierarchy, because we go into a temporary state where there is no root element frame and we don't want to have dangling frame pointers in that state.
Assignee | ||
Comment 15•16 years ago
|
||
Relanded, ca7bb3f59be1.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•