389767 - Regression: Fixed-position items are missing on 2nd page of print-preview

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase

The patch for bug 379349 seems to have broken something with showing fixed-position items on page 2 of Print-Preview.  (confirmed this by trying testcase on current trunk, vs. current trunk with that patch backed out)

Bonsai: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-25++21%3A03%3A00&maxdate=2007-07-25++21%3A03%3A59&cvsroot=%2Fcvsroot

This issue blocks bug 389619 and bug 363729 -- both those bugs need to be able to put fixed-position items on the second page in order for their test cases to reproduce correctly.

Steps to reproduce:
-------------------
 - Load testcase
 - Do Print-Preview
 - Go to 2nd page of preview, and check for "**Hello World**".  (should be there)

Comment 1

[Daniel Holbert [:dholbert]]

Issue seems to be this:

Up until the patch to bug 379349, we would copy fixed content from the first page to the second page during a call to CreateContinuingFrame, with aFrame = the first page's AreaFrame and with aParentFrame = the **second** page's PageContentFrame.

(The idea being, aFrame = frame to continue, and aParentFrame = parent for continuation frame)

That patch changed where and how we call CreateContinuingFrame to build the second page's AreaFrame, however.  It looks like we now make that call with aParentFrame == the **first** page's PageContentFrame (instead of the second page's).  The newly-built AreaFrame ends up as overflow content on the first page, and then is later moved to the second page.

This call happens via this stacktrace:
   #0  nsCSSFrameConstructor.cpp:10394 
          nsCSSFrameConstructor::CreateContinuingFrame
   #1 nsHTMLContainerFrame.cpp:350
          nsHTMLContainerFrame::CreateNextInFlow
   #2 nsPageContentFrame.cpp:97
          nsPageContentFrame::Reflow
     (The nsPageContentFrame in question is from the first page)

(The code at stack level #2 is in a new section added in the patch.)

As a result, we return early from that CreateContinuingFrame call, at nsCSSFrameConstructor.cpp:10659.  
mxr link: http://mxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#10659 
We return there because CreateContinuingFrame **expects** aParentFrame to be the **new** page's nsPageContentFrame.  It's expecting that the previous page's content frame is at aParentFrame->GetParent()->GetPrevInFlow()->GetFirstChild().  (these are called between lines 10649 and 10664, to eventually set the variable prevPageContentFrame).  But while it's trying to look up that previous page, it sees that aParentFrame->GetParent() doesn't have a previous page, so it gives up.

And as a result, we never make it to the end of CreateContinuingFrame, and we never copy fixed content from page 1 to page 2.

I see two classes of solutions (but either/both may be naive, because I'm not too familiar with bug 379349):

  1. Build the second page's PageContentFrame first, so that we can call CreateContinuingFrame with aParentFrame == 2nd Page's PageContentFrame (the way that call originally worked, before the patch)

  2. Move the fixed-content-copying code out of CreateContinuingFrame to somewhere else, to be called **after** the second page's area frame has been moved off the first page's overflow and onto the second page.

fantasai, roc, or anyone else -- do you know if either of those options make sense, or if there's something else we should do?

Comment 2

[fantasai]

I think we should do #2. The code doesn't really belong in CreateContinuingFrame anyway. I'll work on this tonight. Thanks for the report/analysis!

Comment 3

[Daniel Holbert [:dholbert]]

Thanks for taking care of this.

Before moving the fixed-content-copying code, grab the patch for bug 389619, which modifies that code a bit.  (That patch will be checked in to trunk soon after trunk re-opens, but in the meantime, you'd need to apply it locally.)

Comment 4

[fantasai]

Attachment: testcase

Actually, I'm not convinced that test is correct. I think any fixed-positioned element whose position is outside the initial containing block should not be rendered at all. That's what happens in continuous media. We probably need the specs to clarify on this point, though.

Anyway, here's the testcase I'm using. It avoids the above issue.

Comment 5

[fantasai]

Attachment: patch

So, this conflicts with dholbert's patch for bug 389619. I'm not sure what to do about the situation: the bug dholbert's patch fixes isn't visible without this. 

Maybe I'll apply both patches, fix conflicts, and test, then just check in his patch alone and create a new patch for this bug...

Comment 6

[fantasai]

Attachment: patch -up12w

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275273 [details] [diff] [review]
patch

So the key here is that you're changing the parent of the fixed-pos placeholders from the page content frame to the "docRootFrame", right?

>Index: layout/base/nsCSSFrameConstructor.cpp
>+  if (nextInFlow) {
>+    nextInFlow->SetPrevInFlow(newFrame);
>+    newFrame->SetNextInFlow(nextInFlow);

So what frames does this change behavior for?  Just the in-flow child of the page content frame (so the frame corresponding to the document element)?

>+  nsIFrame* prevPageContentFrame = static_cast<nsPageContentFrame*>
>+                                   (aParentFrame->GetPrevInFlow());

Why do you need the cast?  You shouldn't need it.

>+  nsIFrame* docRootFrame = aParentFrame->GetFirstChild(nsnull);

It'd be worth documenting what this is (some continuation of the frame for the document's root element).

It's not in this diff because of small context, but shouldn't the frame constructor state use docRootFrame (not mInitialContainingBlock) as the float/absolute containing block?  Not that it matters much, I guess...

>+  docRootFrame->SetInitialChildList(nsnull, fixedPlaceholders.childList);

Did you test this with documents where the root element has "display: table"?  Wouldn't docRootFrame be a nsTableOuterFrame in that case?  Should be easy to check: if it is, I think this will assert.

Then again, I guess in that case we wouldn't have had a fixed-list to start with...

Out of curiousity, why is being in a block now required for the placeholders?

>Index: layout/generic/nsPageContentFrame.cpp
>-  nsPageContentFrame* prevPageContentFrame = static_cast<nsPageContentFrame*>
>-                                             (GetPrevInFlow());
>-  if (prevPageContentFrame) {
>+  if (mFrames.IsEmpty() && GetPrevInFlow()) {
>+    nsPageContentFrame* prevPageContentFrame = static_cast<nsPageContentFrame*>
>+                                               (GetPrevInFlow());

Why add the extra GetPrevInFlow() call?  And why the mFrames.IsEmpty() check?  Worth documenting the latter.

>+    nsresult rv = aPresContext->PresShell()->FrameConstructor()
>+                    ->ReplicateFixedFrames(this);

Why does this have to happen at reflow time?  The answer doesn't depend on when we do it, right?  all fixed-pos frames will appear on all pages... or is that not the case?

Comment 9

[fantasai]

> So the key here is that you're changing the parent of the fixed-pos
> placeholders from the page content frame to the "docRootFrame", right?

Well, the key is that I'm fixing the problem dholbert noted, which is that the patch for bug 379349 changed the way CreateContinuingFrame was called so that the wrong pageContentFrame was the parent of the new fixed frames. The placeholders were being are added to the doc root frame already.

> So what frames does this change behavior for?  Just the in-flow child of the
> page content frame (so the frame corresponding to the document element)?

Yes. Effectively this doesn't change anything because I can't think of a case where we'd insert a new pageContentFrame between two existing pageContentFrames, but I figured if we ever did we should be hooking it up properly. The code for connecting the in-flows was added in bug 140948. I can't see any reason why it shouldn't apply to the docRootFrame.

> Why do you need the cast?  You shouldn't need it.

Good point.

> It's not in this diff because of small context, but shouldn't the frame
> constructor state use docRootFrame (not mInitialContainingBlock) as the
> float/absolute containing block? 

I don't know. I didn't change that part. (The -up12w patch has more context.)

> Did you test this with documents where the root element has "display: table"? 

Just did. I'm not hitting any assertions that I don't hit on other pages, but I'm also not seeing the fixed frames replicate correctly. I can file a bug for this. The fix will probably interact with bug 320865.

> Out of curiousity, why is being in a block now required for the placeholders?

see bug 200774. You made that change. :)

> Why add the extra GetPrevInFlow() call?

Mainly to tuck prevPageContentFrame's scope inside the brackets. I can undo that.

> And why the mFrames.IsEmpty() check?

A PageContentFrame must always have one child: the doc root element's frame.
We only need to get overflow frames if we don't already have that child; otherwise the call is a nop. It's nice to avoid that extra call, but it's also *necessary* to avoid the extra call to ReplicateFixedFrames.

> Why does this have to happen at reflow time?

I tried doing all this in the Init function, or in ConstructPageFrame, but something always crashed or asserted (or both). So I don't really understand why, but this is where we've always done it, and it works.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Well, the key is that I'm fixing the problem dholbert noted

Ah.  I should have read comment 1, clearly.  Would have made reviewing a lot easier...  ;)

> I don't know. I didn't change that part.

I know.  That was just a general observation.  As I said, it probably doesn't matter much, since everything we'll work with will be fixed-pos.  Might be worth documenting that in a comment.

> but I'm also not seeing the fixed frames replicate correctly.

Hmmm...  I guess the XXXbz comment in nsCSSFrameConstructor::ConstructDocElementTableFrame sort of covers this case... ok, you're off the hook.  :)

> see bug 200774. You made that change. :)

Hoist by my own petard, eh?  I don't even recall touching this code.  :(

> I can undo that.

Please.  It's worth avoiding the extra virtual call, I think.

> We only need to get overflow frames if we don't already have that child;

This should be commented in the code.

> So I don't really understand why, but this is where we've always done it

Story of the life of nsCSSFrameConstructor.  :(

One last question.  When we pull the one child frame from the previous nsPageContentFrame* (which we now do only once with this patch, right?), are we guaranteed that it has no child frames?  The code in ReplicateFixedFrames() certainly assumes that...

Comment 11

[fantasai]

Attachment: patch 2

Yes, we're guaranteed that. The pageContentFrame isn't generated unless the docRootElement continues into this page, and that continuation doesn't get reflowed until we pull it into this pageContentFrame and reflow it. Anyway, I added an assertion about that. :)

Comment 12

[fantasai]

Filed bug 391153 for the table docroot issue.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275494 [details] [diff] [review]
patch 2

r+sr=bzbarsky

Comment 14

[fantasai]

Fix checked in.