430389 - Multi-page print selection has content streaming off end of page and duplicated on next page.

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Daniel Holbert [:dholbert]]

Testing with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008042204 Minefield/3.0pre

(Note: Don't try testing this with a build before this one -- multi-page print-selection only started semi-working today, due to bug 428339 being fixed.)

STEPS TO REPRODUCE:
 1. Load attachment 317148 [details]
 2. Select-All with Ctrl-A
 3. File | Print, with "print selection" checked in options tab.

ISSUES IN PRINTED OUTPUT:
 A. First page's content overflows past the footer, off the page's lower edge.
 B. Second page's content starts at the very top edge, above the header
 C. Content in these areas is duplicated on pages 1 and 2.

I'm pretty sure that the content is actually all positioned correctly -- it's just that we should be cropping what's actually displayed to the correct page-content-area.

(Note: FF2 doesn't show these issues -- hence the 'regression' keyword.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: ff2 print-selection output

Here's an example of Firefox 2's output.

I tweaked the bottom margin slightly, to change the imageable area such that FF2 chopped the number "51" in half.  This *only* happens in print-selection -- normal printing is more graceful, and would push the 51st line to the next page.

This chopping illustrates what FF2 is really doing here -- it lays out the selection on a "mega-page" with unconstrained height, and then it shows a page-sided slice of the "mega-page" on each actual page.

AFAIK, FF3 does the exact same thing, except that it needs to cover up the page-margin regions, so that only the page-content-sized slice is visible on each page.  That's what I meant by this part of comment 0:
> I'm pretty sure that the content is actually all positioned correctly -- it's
> just that we should be cropping what's actually displayed to the correct
> page-content-area.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: ff3 print-selection output (broken)

Comment 3

[Daniel Holbert [:dholbert]]

Looking at the code, I'm pretty sure this has been broken since bug 371528 (" Stop creating views for pages") landed.

That patch added this chunk of code in nsPageFrame::PaintPageContent:

549   // Make sure we don't draw where we aren't supposed to draw, especially
550   // when printing selection
551   nsRect clipRect(nsPoint(0, 0), pageContentFrame->GetSize());
552   aRenderingContext.SetClipRect(clipRect, nsClipCombine_kIntersect);

The sentiment of that comment is correct, but the code right after it completely disobeys it.  Currently, in print-selection, this code creates a clipRect that's positioned at (0,0) with height equal to the full length of the document (multiple pages long -- 120000 app units on attachment 317148 [details]).  This same clip rect is used on each page that we print.  So, nothing is really being clipped.

I can get desired output by manually tweaking clipRect as follows:
  - set clipRect.height to be 52936 (one page-content-area's height)
  - set clipRect.y = 52936 on page 2,  52936*2 on page 3, etc.

I think I just need to change how we set up the clipRect there, so that it ends up matching these manual settings in print-selection situations (and actually clipping to the region that we want to show).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch v1

This patch fixes the issue.

I tested it on zoomed page-content, too, using a testcase with a wide div (which I'll post shortly).

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (200 lines)

Here's a simple testcase with 200 lines.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (200 lines, scaled to fit wide div )

Here's that same 200-line testcase, with a wide div added, to force us to scale the output.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch v2

I improved the patch slightly with a s/GetSize().height/mPD->mReflowSize.height.

AFAIK, this shouldn't behave any differently from the previous patch.  It just makes the calculation of expectedPageContentHeight make more sense.

In particular, this added chunk...
+  // Note: this computation matches how we compute maxSize.height
+  // in nsPageFrame::Reflow
+  nscoord expectedPageContentHeight = NSToCoordCeil(
+    (mPD->mReflowSize.height - mPD->mReflowMargin.TopBottom()) / scale);

... exactly matches the computation in this existing chunk...

107       avHeight = mPD->mReflowSize.height - mPD->mReflowMargin.TopBottom();
...
109     nsSize  maxSize(mPD->mReflowSize.width - mPD->mReflowMargin.LeftRight(),
110                     avHeight);
...
114       maxSize.height = NSToCoordCeil(maxSize.height / scale);

(which is important because maxSize.height is what will become pageContentFrame.GetSize().height)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch v3 (added an assertion)

Requesting review.
 Summary of patch:
  -- Fixes the problem as described in Comment #3 by..
    * detecting print-selection by checking for taller-than-expected pageContentFrames
    * during print-selection, clip to the current page's "slice" of the pageContentFrame

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch v4

(In reply to comment #7)
> I improved the patch slightly with a
> s/GetSize().height/mPD->mReflowSize.height.
> 
> AFAIK, this shouldn't behave any differently from the previous patch.

Oops, I was wrong -- GetSize().height returns what I want (the unscaled full page height), but mPD->mReflowSize.height is actually unconstrained during print-selection runs.  So, patch v4 returns us to using GetSize().height in that computation.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch v5

This revision just fixes an assertion to be slightly more strict.  mPageNum should be > 0, not >= 0.

Comment 11

[Daniel Holbert [:dholbert]]

Requesting approval
 - Benefit: Fixes major rendering issue w/ multipage print-selection (see attachment 317165 [details])
 - Risk: low
 - Passes existing reftests  (test not included because we can't yet reftest print-selection)

Comment 12

[Damon Sicore (:damons)]

Comment on attachment 318696 [details] [diff] [review]
patch v5

a1.9+=damons

Comment 13

[Daniel Holbert [:dholbert]]

Patch v5 landed.
Checking in layout/generic/nsPageFrame.cpp;
/cvsroot/mozilla/layout/generic/nsPageFrame.cpp,v  <--  nsPageFrame.cpp
new revision: 3.181; previous revision: 3.180
done