429337 - Print Selection doesn't show headers / footers

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Daniel Holbert [:dholbert]]

Steps to reproduce:
 1. Open attachment 314434 [details]
 2. Select first line
 3. Print (or print-to-file) with "Print Selection" ticked

Actual Results: 
  Output contains the first line, but has no headers/footers

Expected results:
  Output should contain first line *and* headers/footers (looking similar to normal printed output)

FF2 shows Expected Results.

Tested using:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008041004 Minefield/3.0pre

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to bug 429330 comment #6)
> I think that those bugs (428339 and 429337) should block final release of
> firefox 3.

I'd lean more towards putting this as wanted-1.9.0.x.

I think we'd take a patch if we can get one before closing for final release, but it's not crucial enough for me to think that we should absolutely block on it.  (plus, it should be easy / safe enough to fix in a dot-release.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch v1

This patch fixes this bug.
(tested using the steps described in comment 0)
(Note: This is just an un-bitrotted version of attachment 287280 [details] [diff] [review], Mats' first patch on bug 402264.)

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 316281 [details] [diff] [review]
patch v1

(In reply to comment #2)
> (Note: This is just an un-bitrotted version of attachment 287280 [details] [diff] [review], Mats' first
> patch on bug 402264.)

Though as Eli suggested in 402264 comment 9, this patch breaks printing of selections beyond the first page of content.

e.g. if I try this experiment...
  - Load first chapter of Alice's Adventures in Wonderland, at http://www.sabian.org/alicech1.htm
 - Scroll to bottom, and highlight final line of the chapter ("So she set to work...")
 - Print selection

... I get this output:

 [TRUNK]: one page w/ selected line of text (and nothing else)

 [TRUNK with patch v1]: one blank page

So, patch v1 needs to be improved upon.

Comment 4

[Daniel Holbert [:dholbert]]

Note that, because current trunk uses an unconstrained GetPageSize().height during print selection, that makes us do evil nscoord_MAX arithmetic at the three spots linked below.  (In each case, we get pagesize.height and perform division / addition / subtraction with it.)

http://mxr.mozilla.org/seamonkey/source/layout/generic/nsSimplePageSequence.cpp#235
235   nsSize pageSize = aPresContext->GetPageSize();
...
242   nscoord extraThreshold = PR_MAX(pageSize.width, pageSize.height)/10;
...
269   nsSize availSize(pageSize.width + shadowSize.width + extraMargin.LeftRight(),
270                    pageSize.height + shadowSize.height +
271                    extraMargin.TopBottom());

http://mxr.mozilla.org/seamonkey/source/layout/generic/nsSimplePageSequence.cpp#494
494     nscoord height = aPresContext->GetPageSize().height;
...
504         rect.height = height;
506         y += rect.height + mMargin.top + mMargin.bottom;

http://mxr.mozilla.org/seamonkey/source/layout/generic/nsSimplePageSequence.cpp#594
594     height = PresContext()->GetPageSize().height;
595     height -= mMargin.top + mMargin.bottom;

Comment 5

[Daniel Holbert [:dholbert]]

This is the exact code where an NS_UNCONSTRAINEDSIZE page-height prevents us from getting headers / footers:

http://mxr.mozilla.org/seamonkey/source/layout/generic/nsSimplePageSequence.cpp#494

494     nscoord height = aPresContext->GetPageSize().height;
...
504         rect.height = height;
505         page->SetRect(rect);

If I manually use GDB to set rect.height to 63360 (the original page height), then I get headers & footers printed.

Moreover, this manual tweak doesn't break multi-page printing in the way that patch v1 does.

So, I think we need to do something like patch v1, but in some* of the places where GetPageSize() is called, we want to override the page height with NS_UNCONSTRAINEDSIZE.

*In particular, we'd _not_ want to use an unconstrained page-height at nsSimplePageSequence.cpp#494 (the code pasted above). 

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: patch v2

> So, I think we need to do something like patch v1, but in some* of the places
> where GetPageSize() is called, we want to override the page height with
> NS_UNCONSTRAINEDSIZE.

patch v2 does exactly this.  As it turns out, there's only one place where we need to use an unconstrained height in order to get print-selection working from arbitrary pages.

This patch:
 - Fixes this bug (headers/footers are visible on print-selection output)
 - Successfully prints from selections beyond the first page, using the experiment described in comment 3
 - Prevents all the cases of evil nscoord_MAX arithmetic mentioned in comment 4

FWIW, based on bug 402264 comment 9, it sounds like this patch would be fine by Eli.  (He was asked to review Mats' earlier version of this patch, which was posted in that bug)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 316519 [details] [diff] [review]
patch v2

Requesting approval. Justification:
 - Limited scope (only affects print-selection behavior)
 - Fixes this bug & also prevents evil nscoord_MAX arithmetic
 - Low risk, IMHO

Comment 8

[d]

will the patch be included in rc1?
are there "patched" binaries?

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> will the patch be included in rc1?
That's what the patch approval request in comment 8 is for.  If that's approved, then yes.

> are there "patched" binaries?
Not currently.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316519 [details] [diff] [review]
patch v2

a1.9=beltzner

Comment 11

[Daniel Holbert [:dholbert]]

Patch checked in.

Checking in generic/nsSimplePageSequence.cpp;
/cvsroot/mozilla/layout/generic/nsSimplePageSequence.cpp,v  <--  nsSimplePageSequence.cpp
new revision: 3.169; previous revision: 3.168
done
Checking in printing/nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.177; previous revision: 1.176
done

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> will the patch be included in rc1?
> are there "patched" binaries?

Unless the just-checked-in patch gets backed out for some reason, it will be included in rc1, as well as tomorrow morning's nightly binary build. (available in http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008/04/ )

Comment 13

[Daniel Holbert [:dholbert]]

(Canceling 'wanted1.9.0.x' request, as this has now landed with approval)

Comment 14

[Daniel Holbert [:dholbert]]

Backed out patch, as it seems to have broken some print reftests.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1208820838.1208824557.13774.gz

Checking in generic/nsSimplePageSequence.cpp;
/cvsroot/mozilla/layout/generic/nsSimplePageSequence.cpp,v  <--  nsSimplePageSequence.cpp
new revision: 3.170; previous revision: 3.169
done
Checking in printing/nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.178; previous revision: 1.177
done

Basically it looks like the 
   mPageData->mReflowSize.height = NS_UNCONSTRAINEDSIZE;
line is being run for some of these testcases... which would mean that
   nsIPrintSettings::kRangeSelection == mPrintRangeType
must be evaluating to true.

This must be because we don't initialize nsSimplePageSequenceFrame::mPrintRangeType correctly in reftest-print mode, or something like that.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: bustage fix

(In reply to comment #14)
> Backed out patch, as it seems to have broken some print reftests.
> ...
> This must be because we don't initialize
> nsSimplePageSequenceFrame::mPrintRangeType correctly in reftest-print mode, or
> something like that.

Yeah -- the bustage was due to the fact that nsPrintSettingsGTK's default constructor never initializes mPrintSelectionOnly, so its value was unpredictable.  (As a result, nsPrintSettingsGTK::GetPrintRange was sometimes returning kRangeSelection, which triggered the print-selection-only block that I added)

mPrintSelectionOnly should be initialized to false (to match the copy constructor's initializer list).  This bustage fix adds that.

Comment 16

[Daniel Holbert [:dholbert]]

Re-landed with bustage fix:

Checking in layout/generic/nsSimplePageSequence.cpp;
/cvsroot/mozilla/layout/generic/nsSimplePageSequence.cpp,v  <--  nsSimplePageSequence.cpp
new revision: 3.171; previous revision: 3.170
done
Checking in layout/printing/nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.179; previous revision: 1.178
done
Checking in widget/src/gtk2/nsPrintSettingsGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.cpp,v  <--  nsPrintSettingsGTK.cpp
new revision: 1.10; previous revision: 1.9
done

Comment 17

[Gabriele Best [:Gabri]]

Attachment: Printed PDF file

Comment 18

[Daniel Holbert [:dholbert]]

Per comment 17 & attachment 317080 [details], it looks like this bug's patch partially fixed multi-page printing, though that's still not completely  working.  (talked with Mrtb in IRC a bit about this)

Let's direct further discussion on that to bug 428339.

Comment 19

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5281 added to Litmus for both FF 2 and 3.