389619 - Duplicate frames for content in nested fixed-position divs, when on second page in print-preview

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase

Found this bug while modifying testcases for bug 363729.

To reproduce, do a print-preview of the testcase.  The second page should have only one row of "___" and only one row of "+++" below that.  But instead, we get an extra copy of the "+++", superimposed on top of the "___".

This bug is also present in Firefox 2.0 and Firefox 1.5.  Haven't tested Firefox 1.0.  IE7 displays this correctly. (with just one copy of each row)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fancy testcase

Here's a similar testcase with a few more levels of nesting, to show an exaggerated version of the bug.  On this one, we get several rows of superimposed _, +, /, and ! characters.

Comment 2

[Daniel Holbert [:dholbert]]

Figured out the problem.

When we first construct the PrintPreview frametree, we do it all on one page. This page's framedump looks something like this:

    http://pastebin.mozilla.org/177644

Notice that the actual page content only has **ONE** placeholder, even though there are two elements in the Fixed-list.  This is because the placeholder for the second fixed element is **in** the first fixed element.

Now, to create the second page, there's a point where we call CreateContinuingFrame on the first page's Area frame.  Towards the end of that CreateContinuingFrame call (at nsCSSFrameConstructor.cpp:10678), we iterate across and copy **every element in the first page's fixed-list*** and the resulting list of copies becomes the new PageContent's frame list.  

This is bad -- we shouldn't copy everything in the first page's fixed-list.  Only the first placeholder belongs in the second page's PageContent (the way it is on the first page), so we should only copy the first element of fixed-content.

In fact, just copying that first fixed element will automagically copy the second element, because the second element is a child of the first in the content tree.  When we *explicitly* copy the second fixed element at the end of CreateContinuingFrame, we create a duplicate **erroneous** copy, in addition to the automagical copy.

This is why we get the two lines on the second page.  We're seeing both the automagical copy and the erroneous copy.

SO: I'd propose that at the end of CreateContinuingFrame, we only copy those fixed-elements whose placeholders are descendants of the frame that we're copying.

Will post a patch to that effect soon. (if it works)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: initial patch

This patch implements the fix described at the end of comment 2.
(at the end of CreateContinuingFrame, only copy those fixed-elements whose placeholders are descendants of the frame that we're copying.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: initial patch (no whitespace changes)

here's the no-whitespace-changes version of the previously posted patch

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 273917 [details] [diff] [review]
initial patch

Note that this patch fixes Bug 363729 (blocking1.9) as well.  If this patch is the right thing to do, I'll mark the bugs as duplicates.

Comment 6

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

Use nsLayoutUtils::IsProperAncestorFrame. And in your comment mention that aFrame is the page content frame's sole normal child (and maybe assert that).

Comment 7

[Daniel Holbert [:dholbert]]

Weird... In my fresh trunk checkout from 11:53am today, I'm can't get *any* fixed text to appear on the second page.

I think that's the reason my patch seemed to fix bug 363729 -- when I apply the patch to an earlier build, I still get bug 363729 showing itself.  And when I try to reproduce bug 363729 using this morning's fresh build, I see no crash.

I'm filing this new issue as a separate bug, which will need to be resolved before either this bug or bug 363729 can be closed, because neither bug can be reproduced until the new issue is addressed.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> I'm filing this new issue as a separate bug

bug 389767, to be exact.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch v2 (using IsProperAncestorFrame and an assertion)

Incorporating roc's comments.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch v2 (no whitespace changes)

cvs diff -w version of patch v2

Comment 11

[Daniel Holbert [:dholbert]]

Note: Currently, as described in bug 389767, one needs to back out the final patch for bug 379349 in order to get any fixed-position content on the second page.  

Without backing that patch out, this bug's patches won't do anything, and the testcases will have no fixed content on their second page.

Comment 12

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

+  NS_ASSERTION(prevPageContentFrame->GetFirstChild(nsGkAtoms::normal) == aFrame
+               && aFrame->GetNextSibling() == nsnull,
+              "aFrame should be the prevPageContentFrame's sole normal child");

You pass nsnull to GetFirstChild to get the normal children. Doesn't this assert always fire?

             && aFrame->GetNextSibling() == nsnull,

You could write !aFrame->GetNextSibling().

+    if (origPlaceholder) {
+      if (nsLayoutUtils::IsProperAncestorFrame(aFrame, origPlaceholder)) {

Use && so you only need one if.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: patch v3 (w/ roc's suggestions)

Thanks roc for the comments.  You're right about the assertion -- it triggered all the time because I was using nsGkAtoms::normal instead of nsnull.  My bad -- I should've caught that.

Here's a new version, incorporating roc's suggestions.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: patch v3 (no whitespace changes)

Comment 15

[Daniel Holbert [:dholbert]]

roc, thanks for the review.

Adding checkin-needed to keywords.

Comment 16

[fantasai]

I'll check it in in a couple hours, when I'll be able to watch the tree cycle. :)

Comment 17

[fantasai]

Rather, I'll check it in when the tree reopens.

Comment 18

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

Comment on attachment 274520 [details] [diff] [review]
patch v3 (no whitespace changes)

a1.9=dbaron

Comment 19

[fantasai]

Attachment: patch v3, updated to fix conflicts with checkin for 389767

Comment 20

[Daniel Holbert [:dholbert]]

fantasai's amended patch looks good to me.  I tested it, too, and it fixes the issue with no problems.

Comment 21

[fantasai]

Fix checked in. Thanks dholbert!