389359 - jump-to-page in print preview goes to wrong position

Status: RESOLVED FIXED

(Core :: Print Preview, defect, P2)

Description

[John Daggett (:jtd)]

Steps:

1. Open URL
2. File > Print Preview (with default Page Settings, portrait, shrink to fit)
3. Click on "Next Page" button

Result: the last of five pages is displayed, page indicator shows 2.

Entering "2" for the page value has the same effect, the last of five pages is shown.  Dragging on the scroll bar correctly shows all five pages.

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This is worksforme. Can anyone reproduce?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Um, I've been told that is not WFM.
Bad thing is that I can't reproduce this on latest nightly/windows, nor
on Linux. So fixing this is pretty much impossible for me :(

Comment 3

[Carsten Book [:Tomcat]]

i can reproduce this bug on  Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b2pre) Gecko/2007111204 Minefield/3.0b2pre with the steps to reproduce from comment #0

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Argh, sorry, my mistake. Did something stupid on windows.
I can reproduce, but only on windows, which still makes this pretty impossible for
me to fix.

Comment 5

[Daniel Holbert [:dholbert]]

Regression range is between the nightlies of Feb 13 and 14, 2007.

Build IDs:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a3pre) Gecko/20070213 Minefield/3.0a3pre
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a3pre) Gecko/20070214 Minefield/3.0a3pre

Comment 6

[Daniel Holbert [:dholbert]]

Looks like a regression from Bug 369834.

Comment 7

[Daniel Holbert [:dholbert]]

This is actually a multi-platform bug -- it's not Windows only.  We're just off by different amounts on different platforms.

I tried this on both Windows and Linux:
 * Go to http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/
 * Go into print-preview  (which shows the document to be 7 pages long)
 * Type in a page number and press enter

On Windows, if you try to jump to page 2, it takes you to the end of page 7. (i.e. it goes way too far)

On Linux, if you try to jump to page 7, it takes you in between pages 5 and 6. (i.e. it doesn't go far enough)

I'm guessing this has to do with the GetPrintPreviewScale() function added in Bug 369834 -- there's probably some place we need to use that function and scale our coordinates up or down.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> I'm guessing this has to do with the GetPrintPreviewScale() function

Yup -- if I tweak this call to SetPrintPreviewScale in nsPrintEngine.cpp:1933

  aPO->mPresContext->SetPrintPreviewScale(screenDPI / printDPI);

and set the scale to be exactly 1.0 instead of the DPI ratio, then the next-page and jump-to-page actions work perfectly.  (The page is at the wrong zoom factor, though, because of the changed scale.)

So, that suggests there's something that needs to be scaled up/down by the PrintPreviewScale.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch

This fixes the issue by multiplying the argument of scrollableView->ScrollTo
by the printPreviewScale factor.

I've temporarily disabled subtracting the dead-space above the page, because on my linux box, deadSpaceGap is 1440 and yet there's no visible dead space there at all -- just the shadow of the previous page, and then the current page starts immediately where that shadow ends.  (FWIW, on windows, there is a tiny amount of dead space, but it's much much less than in FF2, so that may also be incorrect.)

I'll file a separate bug about this, with a note that we should reinstate deadSpaceGap-subtraction when that bug is fixed.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> I've temporarily disabled subtracting the dead-space above the page
> ...
> I'll file a separate bug about this, with a note that we should reinstate
> deadSpaceGap-subtraction when that bug is fixed.

Filed as bug 414075

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the deadSpaceGap subtraction didn't work even when there was no scale?  And it doesn't work either inside or outside the parentheses for the multiplication?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that roc is really a better reviewer for this, since I haven't followed all the scaling stuff much, but the patch looks fine to me other than that.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to comment #11)
> So the deadSpaceGap subtraction didn't work even when there was no scale?  

It did work at that point.  However, Print-Preview had visible dead space on Linux at that point. (both before and after scale was added).  Now, no dead space is visible on Linux.

> And
> it doesn't work either inside or outside the parentheses for the
> multiplication?

Correct.  (I just re-tested both inside & outside, to make sure.)  There's *zero* visible dead space in print-preview on Linux right now, so any subtraction there at all will make a chunk of the previous page visible.  (This is filed as bug 414075, as mentioned in comment 10)

(In reply to comment #12)
> Note that roc is really a better reviewer for this, since I haven't followed
> all the scaling stuff much, but the patch looks fine to me other than that.

Ok -- CCing roc, and I'll ask him for review on the final patch. (gonna investigate bug 414075 a little bit first, to see if I can resolve the no-deadspace issue before landing this patch)

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: patch v2 (improved comment)

This patch just updates the previous patch's comment slightly to indicate where deadSpaceGap *should* be subtracted -- inside the parenthesis, before scaling -- if bug 414075 weren't broken.

(I determined this by backing out the patch that caused bug 414075, and trying both positionings.)

Comment 15

[Daniel Holbert [:dholbert]]

Quick summary of patch:
 - Scale page-position by the PrintPreviewScale, so that we actually jump to the correct place.
 - Temporarily disable subtraction of deadSpaceGap in this jump, so that we jump to the exact top of the page, because deadSpaceGap is currently broken (bug 414075.)

Comment 16

[Daniel Holbert [:dholbert]]

Also, this patch works during zoomed print-preview (using ctrl+mousewheel) as well.  (Thanks to smaug for telling me that zoomed print-preview exists. :))

Comment 17

[Daniel Holbert [:dholbert]]

Thanks for the review, roc!  Patch v2 checked in.

Checking in layout/base/nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.565; previous revision: 1.564
done
Checking in layout/printing/nsPrintEngine.h;
/cvsroot/mozilla/layout/printing/nsPrintEngine.h,v  <--  nsPrintEngine.h
new revision: 1.28; previous revision: 1.27
done