383195 - Repeatable footers don't work on trunk in Print/Print-Preview

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

The testcase in the URL bar fails on trunk.  It seems to work as of right before 1.8 branched.  I have no idea when it broke; I tried to find a range, but printing seems to have been completely busted on trunk for most of 2006.  Or at least the nightly builds I have from that time don't even paginate this testcase in print preview, and don't print it when I try to print to file.

Someone who's not on Linux might want to look into this, in the hope that things are better elsewhere.

Comment 1

[Bernd]

Martijn, I suspect this is mine, do you have time to find a regression range?

Comment 2

[dolphinling]

I actually have this breaking between 20061207 and 20061208, which would point to the reflow branch or maybe bug 362708

bonsai query:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-07&maxdate=2006-12-08+04&cvsroot=%2Fcvsroot

(This is also on linux, by the way)

Comment 3

[Martijn Wargers (dead)]

Thanks for finding the regression range, dophinling.
This doesn't seem to be a problem on branch.
However, on print preview, when I change to Landscape format, I don't get a footer on the second page. I can see that problem even on branch. I guess I should file a new bug on that?

Comment 4

[Bernd]

>This doesn't seem to be a problem on branch.

This rules out bug 362708 as it landed on the branches.

Comment 5

[Bernd]

note to myself: verify where the functionality that got removed here http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/tables&command=DIFF&root=/cvsroot&file=nsTableFrame.cpp&rev1=3.665&rev2=3.666#87
is now

Comment 6

[Daniel Holbert [:dholbert]]

During Print-Preview, I'm just seeing one footer (on the last page) on trunk builds for *both* Windows and Linux.

(This is instead of one footer per page, which would be correct.)

Marking as OS: All.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> when I change to Landscape format, I don't get a
> footer on the second page. I can see that problem even on branch. I guess I
> should file a new bug on that?

Filed bug 389748 for this separate branch issue.

Comment 8

[Bernd]

David do you remember why you removed the footer frame stuff?

Comment 9

[Bernd]

I had a discussion with bz on IRC (#developers)

The agreement was that the removed code needs to be reimplemented but the reflow state should not be used to keep track of the necessary pointers and sizes. Local variables should be used instead.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll take this since we should do it soon, there will probably be regressions or other followup work.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsTableFrame::Reflow contains

  if (isPaginated && !GetPrevInFlow() && (NS_UNCONSTRAINEDSIZE == aReflowState.availSize.height)) {
    nscoord height = presContext->GetPageSize().height;
    // don't repeat the thead or tfoot unless it is < 25% of the page height
    if (thead && height != NS_UNCONSTRAINEDSIZE) {
      thead->SetRepeatable(IsRepeatable(*thead, height));
    }
    if (tfoot && height != NS_UNCONSTRAINEDSIZE) {
      tfoot->SetRepeatable(IsRepeatable(*tfoot, height));
    }
  }

I can't figure out how this ever got executed. When did we do an unconstrained-height reflow with pagination enabled? I thought perhaps nsTableFrame::Reflow did an extra unconstrainted-height reflow to make this happen, but I can't find any such thing in the pre-reflow-branch-landing code.

Comment 12

[Boris Zbarsky [:bzbarsky]]

aReflowState is not an nsHTMLReflowState there.  It's a nsTableReflowState.

See http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableFrame.cpp&rev=3.616.6.12&mark=1937-1940,1944-1946#1924 for the place this reflow is coming from.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch v1

My first go at getting this working.

The approach is a bit different from what we used to do. I'm reflowing headers and footers at the start of ReflowChildren to determine whether we should repeat them, and if a footer should be repeated, how much height it needs.

Seems to work OK in my limited testing.

I'm assuming that reflowing them "out of order" is OK, if suboptimal, because we reorder rowgroups etc anyway. The risky part here I think is that we reflow with unconstrained height and then reflow again with constrained height. I wonder if there's something I need to do to make sure the second reflow happens correctly.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The special-height reflow stuff does an unconstrained height reflow followed by a constrained height reflow, for the entire table, which suggests what I'm doing should be OK.

Comment 15

[Damon Sicore (:damons)]

Moving this to P2, but there's no reason why we won't take this during beta 2.  We will accept this if the patch is ready.

Comment 16

[Bernd]

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

> // XXXroc shouldn't we add a repeated footer here?

I think yes, it would otherwise not create a footer for a table where every row is encapsulated in a row group or at least the row that does not fit, is such a encapsulated row.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

I'll file a followup bug about repeating a footer in that case, rather than fix it here now.

Comment 18

[Bernd]

Attachment: testcase for comment 16

Comment 19

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

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

sr=dbaron, although I suspect isPaginated isn't really the right test and we'll eventually want to do this for multi-column as well.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Getting this to work in dynamic contexts is a really big nasty project. I don't know when if ever we'll get around to doing that with our current continuation architecture.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in