Closed Bug 438987 Opened 12 years ago Closed 11 years ago

root element table background not propagated to viewport

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached file testcase
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.
Attached patch fix (obsolete) — Splinter Review
Fix as described.
Attachment #324905 - Flags: superreview?(dbaron)
Attachment #324905 - Flags: review?(dbaron)
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)
Attachment #330298 - Flags: superreview?(dbaron)
Attachment #330298 - Flags: superreview?
Attachment #330298 - Flags: review?(roc)
Attachment #330298 - Flags: review?
Blocks: 418454
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.)
(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+
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
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?
I think so, because eventually the frame will be reflowed which calls SyncFrameViewProperties again.
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?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
I backed this out, it crashed in crashtests, and in reftests locally.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch better patchSplinter Review
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.
Relanded, ca7bb3f59be1.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Blocks: 314585
Depends on: 450322
You need to log in before you can comment on or make changes to this bug.