Fix deadSpaceGap computation and restore deadSpaceGap subtraction in PrintPreviewNavigate

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 years ago
I discovered this while testing the patch for bug 414075 (attachment 300540 [details] [diff] [review]).  

I was checking whether we can re-enable subtraction of deadSpaceGap in the PrintPreview jump-to-page functionality, after applying that fix.  (We can't -- we end up subtracting too much space right now)

I think it boils down to a difference that I can't explain between the value of presContext.mDeviceContext.mAppUnitsPerInch.


In nsSimplePageSequenceFrame::Reflow around line 250, these values are set:

   aPresContext->mDeviceContext.mAppUnitsPerInch = 5760
   gapInTwips = 360
   deadSpaceGap = gapInTwips / 72 / 20 * mAppUnitsPerInch = 1440

... which means we put 1440 app units of dead space between each page.

In DocumentViewerImpl::PrintPreviewNavigate around line 3660, these values are set:
 
   mPresContext->mRawPtr->mDeviceContext.mAppUnitsPerInch = 7740
   deadSpaceGapTwips = 360
   deadSpaceGap = deadSpaceGapTwips / 72 / 20 * mAppUnitsPerInch = 1935

... which means we're expecting 1935 app units of dead space above each page.  (and we subtract that much space)  

Due to this discrepancy, we end up with 1935 - 1440 = 495 app-units of the previous page visible in a jump-to-page operation.


At first I thought the discrepancy might be due to the print-preview scale, but it's not, AFAICT -- the value of that scale is:
   mPrintEngine->mRawPtr->GetPrintPreviewScale() = 1.79166663

whereas the ratio of the mAppUnitsPerInch values is
   1935/1440 = 1.34375

So, I'm not sure where the difference in mAppUnitsPerInch is coming from, but I think it's the source of the problem.

Comment 1

11 years ago
7740/5760 = 1935/1440 = 129/96 = 72*1.7916663/96.  Printing is done to a 72dpi surface, and displays are normally around 96dpi.  So your scaled printing DPI is 129dpi and your display is 96dpi.

Basically, one of them is using the printing context and one is using the display device context.
(Assignee)

Comment 2

11 years ago
(In reply to comment #1)
> So your scaled printing DPI
> is 129dpi and your display is 96dpi.
> 
> Basically, one of them is using the printing context and one is using the
> display device context.

Ah, interesting...  Yeah, my laptop's display is 129 dpi, which has caused other issues in the past (bug 403511).

FWIW, here are two hacks that both fix the problem on my machine:
  1. Manually forcing deadSpaceGap to be 1440 in PrintPreviewNavigate

  2. Setting layout.css.dpi to 96 in about:config (which changes mAppUnitsPerInch in PrintPreviewNavigate to 5760, and hence makes deadSpaceGap come out to 1440)
(Assignee)

Comment 3

11 years ago
Created attachment 300714 [details] [diff] [review]
patch v1

This patch fixes the issue, and re-enables deadSpaceGap subtraction in PrintPreviewNavigate, by using the nsSequenceFrame's  PresContext rather than the nsDocumentViewer's one for the TwipsToAppUnits conversion.

This change ensures that 'deadSpaceGap' will be in the same units in PrintPreviewNavigate as it was when the nsSimplePageSequenceFrame was laid out. These are the units we need it to be in, if we want to reliably scroll to a particular position on the seqFrame's view.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #300714 - Flags: superreview?(roc)
Attachment #300714 - Flags: review?(roc)
Attachment #300714 - Flags: superreview?(roc)
Attachment #300714 - Flags: superreview+
Attachment #300714 - Flags: review?(roc)
Attachment #300714 - Flags: review+
(Assignee)

Updated

11 years ago
Summary: In Print Preview, deadSpaceGap & mAppUnitsPerInch have different values during reflow vs during jump-to-page → Fix deadSpaceGap computation and restore deadSpaceGap subtraction in PrintPreviewNavigate
(Assignee)

Comment 4

11 years ago
Comment on attachment 300714 [details] [diff] [review]
patch v1

Requesting approval for inclusion in b3.

Justification:

 - Minimal change, Print-preview-only, little (or no) risk

 - Improves Print-Preview usability / prettiness.  (restores inclusion of the dead-space above page in PrintPreview's NextPage/PrevPage/Jump-to-Page functionality)
Attachment #300714 - Flags: approval1.9b3?
Comment on attachment 300714 [details] [diff] [review]
patch v1

This can wait until the next beta, IMO.
Attachment #300714 - Flags: approval1.9b3?
Attachment #300714 - Flags: approval1.9b3-
Attachment #300714 - Flags: approval1.9+
(Assignee)

Comment 6

11 years ago
Ok, approval1.9+ WFM. :)
(Assignee)

Updated

11 years ago
Whiteboard: [needs landing]
(Assignee)

Comment 7

11 years ago
Created attachment 302014 [details] [diff] [review]
patch v1a [checked in]

Here's the patch, as landed -- removed an obsolete comment and improved whitespace.

Checking in nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.570; previous revision: 1.569
done
(Assignee)

Updated

11 years ago
Attachment #300714 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.