438987 - root element table background not propagated to viewport

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Fix as described.

Comment 2

[Rob Arnold [:robarnold]]

Attachment: Unbitrotted patch

The patch no longer applies cleanly and mq had trouble with the reftests.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is it OK that FrameHasTransparency does the wrong thing in that case?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think so, because eventually the frame will be reflowed which calls SyncFrameViewProperties again.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out, it crashed in crashtests, and in reftests locally.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better patch

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Relanded, ca7bb3f59be1.