Closed Bug 411585 Opened 12 years ago Closed 12 years ago

mozillazine.org site does not print correctly (columns missing)

Categories

(Core :: Printing: Output, defect, P2, major)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: marcia, Assigned: roc)

References

()

Details

(Keywords: regression, testcase)

Attachments

(4 files, 1 obsolete file)

Seen using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008010904 Minefield/3.0b3pre.

STR:
1. Go to http://mozillazine.org/ and print the first page, as part of the Litmus smoketest. Print to the page to a Epson Stylus C88+ local printer.

Expected:

The page would print correctly, with the columns on the left and right hand side of the pages showing on the printed page.

Actual:

The page does not print correctly. The columns on the left and right are stripped from the first page, and in print preview I can only see remnants of part of the column on the top of the second page.

Safari is able to print the first page fine (including columns), and shows a total of 9 pages in preview.
Compare this with Fx2 which prints the page properly.
It seems that not only mozillazine is affected. A lot of pages are missing parts when printing them. Sounds like a bug with major or even higher severity. Asking for blocking1.9.
Severity: normal → major
Flags: blocking1.9?
I see this on Linux as well.
OS/Platform --> All/All
OS: Mac OS X → All
Hardware: PC → All
Assignee: nobody → dholbert
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
As stated in a comment within testcase 1:

<!-- Note: There are two ways you can tweak this testcase to make the text
  -- visible in print-preview:
  --  A. Reduce height of filler to 500px, so it doesn't overflow first page.
  --  B. Put filler *after* sidebar. (Note that this doesn't change the normal
  --      browser rendering, though it fixes print-prev)  -->
Status: NEW → ASSIGNED
Keywords: testcase
Here's testcase 1 again, with whitespace removed from the body for nicer framedumps.
Attachment #312932 - Attachment is obsolete: true
Attached file reference 1
This reference case just has the order of the divs switched. (tweak B from Comment 5)
Attached file semi-reference 1b
This semi-reference case uses a shorter filler div, less than the height of a page. (tweak A from Comment 5)
I think the root of the problem is this:  In testcase 1, the absolutely-positioned div's placeholder is pushed onto the second page.  However,  the absolutely-positioned div itself falls on the first page.  So we have a placeholder that points to a div on another page, which (I think) isn't supposed to happen.

(This problem doesn't happen in reference 1 because the placeholder comes first, and it doesn't happen in testcase 2 because filler isn't tall enough to push the placeholder all the way onto the second page.)
Could someone track down a regression range for when testcase 1 broke?
[ adding qawanted keyword ]
Keywords: qawanted
This bug is *old*.

I just tracked down a (large) regression range of between 2005-10-01 and 2006-02-01 for testcase1 -- have to go now, though, so I can't narrow it further at the moment.

Note that you may need to increase the height of the "filler" div (say, to 2000px) in order to reproduce the bug in older builds (near early 2007, I think).  We apparently used to be able to cram 1000 pixels onto the first page. (due to different unit-conversion rules or something)
Ok, regression range is between:
  2006-01-25
  2006-01-26
Bonsai link: http://tinyurl.com/32qj76
I'm guessing this is a regression from Bug 317375 ("Reorganize frame painting and mouse event targeting around frame display lists").
stealing
Assignee: dholbert → roc
Status: ASSIGNED → NEW
Duplicate of this bug: 426399
See also these testcases from bug 426399, which have the same regression range as this bug:
 testcase 4a:  attachment 313459 [details]
 testcase 4b:  attachment 313460 [details]
 reference 4:  attachment 313461 [details]
I've got a fix for this, just need to make some reftests. It fixes all the testcases in bug 426399 too.
Attached patch fixSplinter Review
The comments in nsLayoutUtils should explain what's going on. Basically when painting a single page in nsLayoutUtils::PaintFrame we need to traverse other pages to find placeholders through which we should paint abs-pos frames on the current page. This code does that.

PruneDisplayListForExtraPage, which is the trickiest thing in this patch, isn't *really* needed. It does two things:
1) eliminate display items that belong only to the extra page(s)
2) fix up clipping coordinates

Issue 1) doesn't really arise since we pass an empty dirty rect to create the display list for the extra pages, so the only frames for which we will create display items are ones marked NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO when we constructed the display list for the current page.

Issue 2) doesn't arise AFAIK since the only way I know of to get an element with clipping on page 2 which could still draw a clipped child on page 1 would be to use an abs-pos element with 'clip:rect(-1000px ...)' or similar, but clip only applies to abs-pos elements and bug 426909, which will not be fixed for 1.9, prevents display of abs-pos elements that don't start on the first page.

So I'm going to attach a simpler patch that does none of those things.
Comment on attachment 313485 [details] [diff] [review]
fix

On second thoughts I think we should go with this. Relying on the dirty rect would be fragile and probably would fail with container frames with borders and backgrounds that contain placeholders for frames on earlier pages.
Attachment #313485 - Attachment description: NOT the best fix → fix
Attachment #313485 - Flags: superreview?(dbaron)
Attachment #313485 - Flags: review?(dbaron)
To get this approach to work I had to prevent page-content frames from clipping everything outside the page --- that directly makes them abs-pos frames on earlier pages they contain placeholders for. So I moved the page-content clipping out and consolidated it with existing clipping in nsPageFrame.
The way I'd *like* this all to work as part of a big printing rework plan is:
-- Making printing place each printed page so that page-content edges are adjacent.
-- When printing each page, construct a display list for the whole document at once with the current page selected as the dirty rect.
-- Rework print preview as we've discussed elsewhere (not sure where :-( ) ... Make all the print-preview bling outside the page contents part of a print-preview XUL document, and each page-contents is a special XUL element that renders a page directly from the print presentation.
Whiteboard: [needs review]
dholbert, you probably want to look at this patch too.
Comment on attachment 313485 [details] [diff] [review]
fix

>+      // We may need to paint out-of-flow frames whose placeholders are
>+      // on other pages. Add those pages to our display list. Note that
>+      // out-of-flow frames can't be placed after their placeholders so
>+      // we don't have to process earlier pages. The display lists for
>+      // these extra pages are pruned so that only display items for the
>+      // page we currently care about (which we would have reached by
>+      // following placeholders to their out-of-flows) end up on the list.

I'm having trouble following this.  I can understand that an out-of-flow can be after its placeholder (e.g., when there are a lot of floats and some end up on later pages), but I don't see how it can end up before.  Did you just say this the wrong way around (i.e., s/after/before/)?  Or am I missing something?
Actually I said it the right way around.

Abs-pos frames are always children of the first-in-flow of their containing block, even though the placeholders may be pushed to be descendants of continuations of the containing block.

For floats, the placeholder and the out-of-flow's first-in-flow must always be descendants of the same continuation frame of the float containing block. For example, the code around PushTruncatedPlaceholderLine ensures that if a line has a placeholder for a below-current-line float, but the BCL float needs to be pushed to the next page, we actually split the line before that placeholder to ensure that it goes with the BCL float to the next page.
Comment on attachment 313485 [details] [diff] [review]
fix

>+ * @param aPage the page we constructed aList for

Could you call this aExtraPage to match terms used elsewhere?  (The variable too, not just the comment.)

>+        // This might clip an element which should appear on the first
>+        // page, and that element might be visible if this uses a 'clip'

s/first page/main page/ ?  (For consistent terminology.)

>+        // property with a negative top.

Is this interpretation of 'clip' across pages sanctioned by any specs?  I suppose it vaguely makes sense, but it's the kind of thing that the spec ought to describe if this is how it works.  Any chance you could raise this on www-style if it matches what other browsers do?

>+static nsIFrame*
>+GetNextPage(nsIFrame* aPageContentFrame)
>+{
>+  // XXX ugh
>+  nsIFrame* pageFrame = aPageContentFrame->GetParent();
>+  nsIFrame* nextPageFrame = pageFrame->GetNextSibling();
>+  if (!nextPageFrame)
>+    return nsnull;
>+  return nextPageFrame->GetFirstChild(nsnull);

Some assertions about frame types would be nice here.

r+sr=dbaron
Attachment #313485 - Flags: superreview?(dbaron)
Attachment #313485 - Flags: superreview+
Attachment #313485 - Flags: review?(dbaron)
Attachment #313485 - Flags: review+
(In reply to comment #24)
> Is this interpretation of 'clip' across pages sanctioned by any specs?  I
> suppose it vaguely makes sense, but it's the kind of thing that the spec ought
> to describe if this is how it works.  Any chance you could raise this on
> www-style if it matches what other browsers do?

Breaking of absolutely-positioned elements across pages is not described in the spec. I guess I can raise the 'clip' issue but this room is full of elephants.
Checked in
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Pages getting printed correctly now. So the first page contains the sidebars and text and each further page only the text => Verified.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041506 Minefield/3.0pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041504 Minefield/3.0pre ID:2008041504
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9
Depends on: 473548
You need to log in before you can comment on or make changes to this bug.