414075 - Little or no dead space between pages in Print Preview

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Daniel Holbert [:dholbert]]

In Trunk on Linux, there's no visible dead space above the first page, between pages, and after the last page.   (There *are* page shadows in between the pages and after the last page, but no dead white space.)

On Windows, there's some dead white space, but it's very little -- much less than there was in Firefox 2.

I'm pretty sure there should be *some* dead white space, because
 a) there was in Firefox 2
 b) there is a small amount on Windows
 c) the DocumentViewerImpl::PrintPreviewNavigate code seems to assume that there is -- it jumps to the y-position of the given page, minus the deadSpaceGap, to give the page a whitespace border above it.*

*Note -- the "minus the deadSpaceGap" part of DocumentViewerImpl::PrintPreviewNavigate may be temporarily disabled as part of the fix for bug 389359, because of this bug here. (see proposed patch, attachment 299323 [details] [diff] [review])  When this bug here is fixed, we should re-enable this subtraction.

Comment 1

[Daniel Holbert [:dholbert]]

I'm assuming that the intended behavior is for there to be as much dead white space as there was in Firefox 2 -- correct me if that's wrong.

Comment 2

[Daniel Holbert [:dholbert]]

Actually, in Trunk, there's also no dead space on the sides of the page, either.  (Firefox 2 had some space to the sides, for a nice padding effect.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screenshot of trunk PrintPreview

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of branch PrintPreview

Comment 5

[Daniel Holbert [:dholbert]]

On linux, the dead space disappeared between these builds:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112004 Minefield/3.0b2pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112105 Minefield/3.0b2pre

--> regression from bug 404519, probably.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> --> regression from bug 404519, probably.

Yup -- backing out the bug 404519 patch from current trunk fixes this bug here, reinstating some dead space on linux in PrintPreview.

Also, I found one more reason there should definitely be visible dead space- -- nsSimplePageSequenceFrame::GetDeadSpaceValue returns NS_INCHES_TO_TWIPS(0.25), and has since 2002. (see URL below)
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsSimplePageSequence.h#106

So we should be seeing 1/4 inch of dead space.

Comment 7

[Daniel Holbert [:dholbert]]

Requesting blocking, since this is a pretty visible regression from Firefox 2.0. (see screenshots)

Comment 8

[Jeremy Lea]

Attachment: Fix of patch from Bug 404519

Please try this patch before backing out Bug 404519...

Comment 9

[Jeremy Lea]

Attachment: Better fix...

Actually, this is a better fix.  Reuse gapInTwips.

Comment 10

[Daniel Holbert [:dholbert]]

I can confirm that "better fix" reinstates the 1/4 inch of dead white space.  Yay! Thanks for fixing this, Jeremy.

One thing you should add as part of this bug's patch, though: Now that deadSpaceGap is correct again, can you subtract it inside the parenthesis in the computation of "newYPosn" at nsDocumentViewer.cpp:3669, and remove the XXXdholbert comment there about this issue?

It's at this point in the code
http://lxr.mozilla.org/mozilla/source/layout/base/nsDocumentViewer.cpp#3669

and we need to change
  3667 nscoord newYPosn = 
  3668   nscoord(mPrintEngine->GetPrintPreviewScale() * 
  3669           float(fndPageFrame->GetPosition().y));

to
  3667 nscoord newYPosn = 
  3668   nscoord(mPrintEngine->GetPrintPreviewScale() * 
  3669           float(fndPageFrame->GetPosition().y - deadSpaceGap));

and remove the comment above that chunk.
(this is new code as of yesterday, checked in as part of bug 389359)

Comment 11

[Daniel Holbert [:dholbert]]

Actually, maybe cancel my last comment -- after making the change I mentioned above, we're not quite lining up correctly on Jump-To-Page -- a little bit of of the previous page is visible, in addition to the (expected) dead space.

Investigating this a bit...

Comment 12

[Daniel Holbert [:dholbert]]

Ok -- I think there are more issues involved with the deadSpaceGap subtraction issue in print-preview jump-to-page functionality, so we should leave that disabled for now, and I'll file a separate bug for that.

This bug's patch should land as-is. (ignoring the changes I suggested in comment 10)

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 300540 [details] [diff] [review]
Better fix...

Requesting approval for this patch to go into beta 3.

Justification:
 - Makes print-preview prettier, fixes visible regression. (see screenshots)
 - Minimal change, with little (or no) risk
 - The replaced code was clearly a typo, and the added code does what was clearly intended to be done

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> Ok -- I think there are more issues involved with the deadSpaceGap subtraction
> issue in print-preview jump-to-page functionality, so we should leave that
> disabled for now, and I'll file a separate bug for that.

Filed as bug 415012.

Comment 15

[Mike Schroepfer]

Comment on attachment 300540 [details] [diff] [review]
Better fix...

Go for it

Comment 16

[Daniel Holbert [:dholbert]]

Patch checked in.  Thanks for the patch, Jeremy!

Checking in nsSimplePageSequence.cpp;
/cvsroot/mozilla/layout/generic/nsSimplePageSequence.cpp,v  <--  nsSimplePageSequence.cpp
new revision: 3.163; previous revision: 3.162
done