403669 - print/print preview only prints first of many pages, then gives up

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P2)

Description

[chris hofmann]

Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-IN; rv:1.9a9pre) Gecko/2007110705 Minefield/3.0a9pre

load URL http://visitpuertovallarta.com/puertovallarta/entertainment/restaurants/index.html

try to print or print preview.

page header prints on first page, then one page of restaurant listings, then a mostly blank final page gets printed.   

several pages of restaurant listings should be printed or shown.

Comment 1

[chris hofmann]

this seems to work fine in 2.0.0.x

Comment 2

[Mike Schroepfer]

Moving to blocking . If we fix it do we get to go there?

Comment 3

[Mackenzie]

Can this be marked "all" instead of Windows?  I've had this problem on OSX (10.4) and Linux (Ubuntu 7.10).

Comment 4

[Ori Avtalion (salty-horse)]

Another test-case, with less images: http://www.dribin.org/dave/blog/archives/2007/12/30/why_mercurial/

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> Another test-case, with less images:
> http://www.dribin.org/dave/blog/archives/2007/12/30/why_mercurial/
>

I think that URL is a different issue -- it's broken in FF2 as well.

I can confirm that the visitpuertovallarta site is broken in FF3 but prints fine in FF2, though.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase (should be 6+ pages)

Here's a testcase demonstrating the problem.

 Steps to reproduce:  Print or Print-Preview the testcase

 Expected Behavior:   6+ pages of output

 Actual behavior:     2 pages of output (with no a's crossing beyond first page)

FF2 shows expected behavior.  FF3 b2 shows actual behavior.


The basic problem here is this:  content in a small-specified-height table isn't being allowed to extend beyond the page in which the table starts.

Comment 7

[Daniel Holbert [:dholbert]]

Testcase works in nightly from 2006-12-01, but is buggy in 2006-12-08. Looks like this is from the reflow branch.

Comment 8

[chris hofmann]

Daniel,  hope you noticed schrep's comment #2.  I'd say yes ;-)  It will be great to see this fixed...

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> Daniel,  hope you noticed schrep's comment #2.  I'd say yes ;-)  

Haha. :)  It should be a Gran Paradiso-themed trip.

Comment 10

[Ori Avtalion (salty-horse)]

(In reply to comment #5)
> (In reply to comment #4)
> > Another test-case, with less images:
> > http://www.dribin.org/dave/blog/archives/2007/12/30/why_mercurial/
> >
> 
> I think that URL is a different issue -- it's broken in FF2 as well.

You're right. It's bug 129941.

Comment 11

[Daniel Holbert [:dholbert]]

So basically, we're not correctly handling overflow of the table contents when the table has a fixed height.

Comment 12

[Daniel Holbert [:dholbert]]

Here's some analysis of what's going wrong...

So, for specified-height tables in paginated layouts, we set ourselves up to do a special-height reflow, at line 1873:

1869     if (isPaginated && !GetPrevInFlow() && (NS_UNCONSTRAINEDSIZE != aReflowState.availableHeight)) {
1870       nscoord tableSpecifiedHeight = CalcBorderBoxHeight(aReflowState);
1871       if ((tableSpecifiedHeight > 0) && 
1872           (tableSpecifiedHeight != NS_UNCONSTRAINEDSIZE)) {
1873         needToInitiateSpecialReflow = PR_TRUE;
1874       }
1875     }


And as a result, on the initial table reflow, we allow ourselves unconstrained height:

1887     nscoord availHeight = needToInitiateSpecialReflow 
1888            ? NS_UNCONSTRAINEDSIZE : aReflowState.availableHeight;
1889 
1890     ReflowTable(aDesiredSize, aReflowState, availHeight,
1891                 lastChildReflowed, aStatus);

Now, on our SECOND call to ReflowTable (at line 1912), we're passing in the actual page height, and I think we intend to figure out that we need multiple pages at this point.  

During that call, we do end up hitting SplitRowGroup, which means we're on the right track, but we end up here:

http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowFrame.cpp#827
#0  nsTableRowFrame::ReflowChildren     at nsTableRowFrame.cpp:827
#1  nsTableRowFrame::Reflow             at nsTableRowFrame.cpp:1042
#2  nsContainerFrame::ReflowChild       at nsContainerFrame.cpp:755
#3  nsTableRowGroupFrame::SplitRowGroup at nsTableRowGroupFrame.cpp:1081
#4  nsTableRowGroupFrame::Reflow        at nsTableRowGroupFrame.cpp:1294
#5  nsContainerFrame::ReflowChild       at nsContainerFrame.cpp:755
#6  nsTableFrame::ReflowChildren        at nsTableFrame.cpp:2954
#7  nsTableFrame::ReflowTable           at nsTableFrame.cpp:2000

... at which point, we set "doReflowChild" to PR_FALSE, which prevents us from doing another reflow on the table cell. (and I think we *need to do* that reflow on the table cell)

If I keep doReflowChild set to PR_TRUE, then we get the correct printed output.  So I think we might need to tweak one of the flags that we're checking there, so that we don't clear doReflowChild in this situation. (e.g. maybe we need to add a SetGeometryDirty() call somewhere?)

Comment 13

[Daniel Holbert [:dholbert]]

One more thing --

The context of clearing doReflowChild is this:

 821     // See if we should only reflow the dirty child frames
 822     PRBool doReflowChild = PR_TRUE;
 823     if (!aReflowState.ShouldReflowAllKids() &&
 824         !aTableFrame.IsGeometryDirty() &&
 825         !NS_SUBTREE_DIRTY(kidFrame)) {
 826       if (!aReflowState.mFlags.mSpecialHeightReflow)
 827         doReflowChild = PR_FALSE;
 828     }

Now,  you'd think that the (!aReflowState.mFlags.mSpecialHeightReflow) check would fail, because we're in the middle of a special height reflow, after all.

However, it passes, because we clear the mSpecialHeightReflow bit for the duration of SplitRowGroup, as shown here (in nsTableRowGroupFrame.cpp)

1292     PRBool specialReflow = (PRBool)aReflowState.mFlags.mSpecialHeightReflow;
1293     ((nsHTMLReflowState::ReflowStateFlags&)aReflowState.mFlags).mSpecialHeightReflow = PR_FALSE;
1294 
1295     SplitRowGroup(aPresContext, aDesiredSize, aReflowState, tableFrame, aStatus);
1296 
1297     ((nsHTMLReflowState::ReflowStateFlags&)aReflowState.mFlags).mSpecialHeightReflow = specialReflow;

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: workaround

Simple workaround, but probably not the right solution

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: patch v1 [checked in]

I think this patch is a bit better than the previously-posted workaround.

It just sets the table geometry to be dirty inside of SplitRowGroup, which I think makes sense because we really are messing with the table geometry...  (at least, we're changing available heights)  In stack frames below SplitRowGroup, optimizations that depend on the table geometry being clean don't really apply if we're in the middle of splitting rows.

dbaron, does this make sense to you?

Comment 16

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

Comment on attachment 298579 [details] [diff] [review]
patch v1 [checked in]

Makes sense to me; r+sr=dbaron.

Please add some regression tests.  Hopefully you can do it using -moz-column-* (with fixed column heights?).  (Was there printing reftest stuff in the works at some point?)

Comment 17

[Daniel Holbert [:dholbert]]

I don't think I can use -moz-column for testing this...  SplitRowGroup only works on paged media, per this precondition at the beginning:

1034   NS_PRECONDITION(aPresContext->IsPaginated(), "SplitRowGroup currently supports only paged media"); 

There is a "layout/reftests/printing" directory, though, which looks like it emulates printing via <html class="reftest-print">.  I'll see if that works as desired.

Comment 18

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

Ah, right, it's a class -- I was looking for something in the manifest.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: reftest [checked in]

Here's a printing reftest.

However, this reftest fails even after patch v1 -- the contents of the first page are shifted down by a few pixels, and subsequent pages are shifted down by smaller amounts.

This shifting doesn't occur before patch v1, so this bug's patch probably still needs a bit more work.

Comment 20

[Daniel Holbert [:dholbert]]

Here are data-URLs for the reftest's output so far.

(note: I'm using preview.tinyurl.com because the auto-redirect from www.tinyurl.com doesn't seem to work for data-URLs this long.)

Testcase pre-patching:  http://preview.tinyurl.com/37f8nr
Testcase post-patch v1: http://preview.tinyurl.com/2kguon
Reference case:         http://preview.tinyurl.com/2sqfok

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to comment #19)
> However, this reftest fails even after patch v1 -- the contents of the first
> page are shifted down by a few pixels, and subsequent pages are shifted down by
> smaller amounts.

I'm not sure why this was failing before, but it passes on today's build using patch v1.  I've also verified that the print-preview framedump structures & geometries are identical.  So, I think it's good to land, once the tree reopens.

(In reply to comment #2)
> If we fix it do we get to go there?

Puerto Vallarta, here we come!

Comment 22

[Daniel Holbert [:dholbert]]

patch v1 and reftest checked in.

Checking in tables/nsTableRowGroupFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableRowGroupFrame.cpp,v  <--  nsTableRowGroupFrame.cpp
new revision: 3.392; previous revision: 3.391
done
RCS file: /cvsroot/mozilla/layout/reftests/printing/403669-1-ref.html,v
done
Checking in reftests/printing/403669-1-ref.html;
/cvsroot/mozilla/layout/reftests/printing/403669-1-ref.html,v  <--  403669-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/printing/403669-1.html,v
done
Checking in reftests/printing/403669-1.html;
/cvsroot/mozilla/layout/reftests/printing/403669-1.html,v  <--  403669-1.html
initial revision: 1.1
done
Checking in reftests/printing/reftest.list;
/cvsroot/mozilla/layout/reftests/printing/reftest.list,v  <--  reftest.list
new revision: 1.5; previous revision: 1.4
done