Closed Bug 414075 Opened 13 years ago Closed 13 years ago

Little or no dead space between pages in Print Preview

Categories

(Core :: Print Preview, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: reg)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.
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.
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.)
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.
Blocks: 404519
(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.
Requesting blocking, since this is a pretty visible regression from Firefox 2.0. (see screenshots)
Flags: blocking1.9?
Keywords: regression
Attached patch Fix of patch from Bug 404519 (obsolete) — Splinter Review
Please try this patch before backing out Bug 404519...
Attachment #300538 - Flags: superreview?(roc)
Attachment #300538 - Flags: review?(roc)
Status: NEW → ASSIGNED
Assignee: nobody → reg
Status: ASSIGNED → NEW
Attached patch Better fix...Splinter Review
Actually, this is a better fix.  Reuse gapInTwips.
Attachment #300538 - Attachment is obsolete: true
Attachment #300540 - Flags: superreview?(roc)
Attachment #300540 - Flags: review?(roc)
Attachment #300538 - Flags: superreview?(roc)
Attachment #300538 - Flags: review?(roc)
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)
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...
Attachment #300540 - Flags: superreview?(roc)
Attachment #300540 - Flags: superreview+
Attachment #300540 - Flags: review?(roc)
Attachment #300540 - Flags: review+
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 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
Attachment #300540 - Flags: approval1.9b3?
Blocks: 415012
(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 on attachment 300540 [details] [diff] [review]
Better fix...

Go for it
Attachment #300540 - Flags: approval1.9b3? → approval1.9b3+
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.