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

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: jtd, Assigned: dholbert)

Tracking

({regression})

Trunk
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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
Flags: blocking1.9?
(Reporter)

Updated

11 years ago
Blocks: 389360
Flags: blocking1.9? → blocking1.9+

Updated

11 years ago
Keywords: regression
Priority: -- → P2

Updated

11 years ago
Assignee: nobody → Olli.Pettay

Comment 1

11 years ago
This is worksforme. Can anyone reproduce?

Comment 2

11 years ago
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 :(
Assignee: Olli.Pettay → nobody
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

11 years ago
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.
(Assignee)

Comment 5

11 years ago
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
(Assignee)

Comment 6

11 years ago
Looks like a regression from Bug 369834.
Blocks: 369834
(Assignee)

Comment 7

11 years ago
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.
(Assignee)

Updated

11 years ago
OS: Windows XP → All
Summary: Clicking on next page button of print preview moves several pages ahead → jump-to-page in print preview goes to wrong position
(Assignee)

Comment 8

11 years ago
(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.
Assignee: nobody → dholbert
(Assignee)

Comment 9

11 years ago
Created attachment 299323 [details] [diff] [review]
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.
Attachment #299323 - Flags: review?(dbaron)
(Assignee)

Comment 10

11 years ago
(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
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
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?
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.
(Assignee)

Comment 13

11 years ago
(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)
(Assignee)

Comment 14

11 years ago
Created attachment 300066 [details] [diff] [review]
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.)
Attachment #299323 - Attachment is obsolete: true
Attachment #300066 - Flags: review?(roc)
Attachment #299323 - Flags: review?(dbaron)
(Assignee)

Comment 15

11 years ago
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.)
(Assignee)

Comment 16

11 years ago
Also, this patch works during zoomed print-preview (using ctrl+mousewheel) as well.  (Thanks to smaug for telling me that zoomed print-preview exists. :))
Attachment #300066 - Flags: superreview+
Attachment #300066 - Flags: review?(roc)
Attachment #300066 - Flags: review+
(Assignee)

Comment 17

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.