Closed Bug 430150 Opened 14 years ago Closed 14 years ago

Print-selection output too high (or off of page) due to print scaling

Categories

(Core :: Printing: Output, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(6 files)

When we're doing a print-selection, we shift the selected area up to the top of the printed page.  However, when print scaling[1] is involved, it looks like we don't correctly scale our units when we shift the selected area.  Consequently, the selected area gets shifted too far up, possibly all the way off the page.

This is demonstrated by the upcoming testcases.

STEPS TO REPRODUCE:
 1. Load testcase
 2. Select the line of text
 3. Print selection

EXPECTED RESULT:
 Selected text is at top of page, inside the margins.
(and inside the headers, if you've got the patch for bug 429337 applied and can see print headers)
 
ACTUAL RESULT:
 Varies by testcase:
   - in testcase 1, the selected text appears at the very top edge of the page (outside the print header, if you've got the patch for bug 429337 applied.)
   - in testcase 2, the selected text isn't visible. (it's off the page)

[1] "print scaling" is when our content is too wide to fit on the page, and so we shrink the whole page by some scaling factor in order to make it fit.
Attached file testcase 1
This testcase uses a 9in-wide div to trigger print-scaling.

The div is 1in tall, which means that (ignoring scaling) the selected text will need to be shifted up 1in to be at the top of the page, when printed with print-selection.

However, the text actually ends up being placed near the absolute top edge of the page, in the margin region, which is higher than it should be.
Attached file reference 1
This reference case uses a 4in wide div, instead of 9in wide, so it doesn't trigger print scaling.

As a result, we get expected behavior.
Attached file testcase 2
This is like testcase 1, but it uses a div that's much taller, which means the selected text needs to be shifted by a lot more.

This means our scaling error has a much bigger effect, shifting the text all the way off of a printed page.
Attached file reference 2
This reference case just uses a skinnier div than testcase 2, so that we don't need to do any print scaling.
This bug is a regression WRT Firefox 2. (Firefox 2 gives expected output when I apply the steps from comment 0.)
Assignee: nobody → dholbert
Blocks: 402264
Keywords: regression, testcase
Duplicate of this bug: 429934
Duplicate of this bug: 428417
Attached patch fix v1Splinter Review
This patch fixes the bug by appropriately scaling the selection position & height, in the call to  pageSequence->SetSelectionHeight.

This fixes all testcases on this bug and its duplicates.  I verified it on the following testcases:
 - testcases 1 and 2 here
 - oxid-esales.com, per bug 428417 comment 0 (duplicate of this bug)
 - factcheck.org, per bug 429934 comment 4 (duplicate of this bug)
 - bugzilla.mozilla.org, per bug 429934 comment 5 (duplicate of this bug)
Note that the patch also moves the declaration of selectionHgt up a few lines, instead of recomputing that value twice, as we were before.
Status: NEW → ASSIGNED
Attachment #316907 - Flags: superreview?(roc)
Attachment #316907 - Flags: review?(roc)
Also: fwiw, the patch *only* affects print-selection behavior -- the modified code is all within a "if (nsIPrintSettings::kRangeSelection == printRangeType)" clause.
Attachment #316907 - Flags: superreview?(roc)
Attachment #316907 - Flags: superreview+
Attachment #316907 - Flags: review?(roc)
Attachment #316907 - Flags: review+
Comment on attachment 316907 [details] [diff] [review]
fix v1

Requesting approval. Justification:
 - Low-risk
 - Scope is limited to print selection behavior (per comment 10)
 - Fixes print-selection issues on multiple sites (per comment 8)
Attachment #316907 - Flags: approval1.9?
FWIW, this comment in the patch
 +                // XXXdholbert does this num-pages calculation need to take
 +                // aPO->mZoomRatio into consideration?
can't be addressed until bug 428339 ("Print Selection output is limited to one page in trunk") is fixed.

(Once this patch lands, I'll mention this added comment on that bug's page.)
Comment on attachment 316907 [details] [diff] [review]
fix v1

a1.9+=damons
Attachment #316907 - Flags: approval1.9? → approval1.9+
fix v1 landed.

Checking in nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.180; previous revision: 1.179
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Here's a second testcase, just like the first, but simplified by removing the margin from the body.

With fix v1, PDF-printouts of this testcase both with and without print-selection line up exactly in their positioning of the first pages' content.  
(They don't quite line up for testcase 1 because it has the default body margin, and print-selection ignores this body margin.)
Comment on attachment 317147 [details]
[-- ignore me -- posted wrong file on wrong bug :) ]

Oops... Ignore my last comment.  I meant to post a different testcase, and to post it on bug 430357, not here. :)
Attachment #317147 - Attachment description: testcase 2 (no margin on body) → [-- ignore me -- posted wrong file on wrong bug :) ]
Blocks: 431588
You need to log in before you can comment on or make changes to this bug.