422678 - style height on table cells not honored during printing

Status: VERIFIED FIXED

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

Description

[Bernd]

Attachment: look the testcase during in print preview

the testcase worked in the build from 2006-12-0704 and fails already in 2006-12-08-04

Comment 1

[Bernd]

Attachment: reftest

Comment 2

[Bernd]

Attachment: reference

Comment 3

[David Baron :dbaron:]

Seems somewhat related to bug 422589.

Comment 4

[Bernd]

> Seems somewhat related to bug 422589

no, the behaviour does not change with the backout patch https://bugzilla.mozilla.org/attachment.cgi?id=308945 with respect to current trunk.

Comment 5

[Bernd]

Attachment: patch to restore the old behaviour

surprise surprise, Davids guess was correct, mine was not.

now comes the whole regression history of bug 373400 again, arggh.
OK this time with reftests....

Comment 6

[Bernd]

Whole regression history means the test cases in bug 231823 and bug 212984 will be broken again with this patch (attachment 309634 [details] [diff] [review])

Comment 7

[Bernd]

David changed in bug 231823 the way SplitRowGroup works to a way that corresponds to what reflow usual expects.  However this change was not in sync with SplitSpanningCells which leads to crashes when printing tables with rowspans (tinderbox for instance).

To fix the crashes in bug 373400  basically happened a backout of the patch with one "minor" modification 

- if (rowMetrics.height >= availSize.height) {
+ if (rowMetrics.height > availSize.height) {

This triggered a question from Eli

>Is there some reason why the containing if statement shouldn't be "if
>(rowMetrics.height > availSize.height)" rather than "if (rowMetrics.height >=
>availSize.height)"?

with my response:

"Yes, if rowMetrics.height == availSize.height it still fits (perfectly) so why
should it be pushed?"

This sentence which should be fine in normal layout code is however not true in
table land.

rowMetrics.height == availSize.height is here a special condition that shows that row did not fit due to style constrains.

How does that work:

We reflow first the rowgroup 
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableRow
GroupFrame.cpp&rev=3.395&mark=1317,1325,1330#1315

we do this with unconstrainend height 

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.395&mark=401-403#390

If the rowgroup desired height exceeds the available height we split the rowgroup

Then we loop over the rows and look which row has a YMost larger than then available size.

Once we exceed this we reflow the row again with remaining available space that is aReflowstate.availableHeight - lastRowYmost

There are several possibilities 
1. this is the first row on page - we need to place it

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.395&mark=1090-1094#1080

2. The row minimum height exceeds the available space.

Please note that minimum here is not minimum in a usual layout sense but rather the historic MEW style, so even if a fixed style height is supplied for a row or cell in the row, this would not increase the minimum height, only content inside the table cells influences this.

In this case one would expect the cells to create a continuation.

3. The style height exceeds the available space. As outlined above the row will not yet have honored this information. It will be clamped by the available space

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableRowFrame.cpp&rev=3.408&mark=1015-1020#1010

Now with the information that the row did not fit with when reflowed with an unconstrained height we know that the row got clamped when it is all over sudden complete and does fit into the small space.
 Thats is the magical function of the = sign that I removed.

The next scenario is that it contains rowspanning cells that spill out of the row.

Those cells should be split but here occurs the problem with this algorithm that the original bug 231823 revealed.

Given a structure like

<table>
<tr><td rowspan="2"> a lot of content with multiple lines that would be smaller than 1000px</td></tr>
<tr style="height:1000px"></tr>
</table>

The unconstrained reflow will make the first <tr> as small as possible and all the space will go into the second row. If this row is larger than a page height. 
We will try to split in between the first and the second row
We will reflow the cell with a 0px available height and it will split the cell after the first line. Further it will create a new continuation row that continues the first row and push this continuation together with the second row.

On the next page this will repeat as long as not all text lines are each on a separate page.

I don't have a good idea how fix this in the 1.9 time frame, especially given the lack of regression testing that we have for table pagination.

One option would be to place the continuation of the first split cell into the next row that will be pushed but with a reduced rowspan. Its unclear how to realize this as we will look up the rowspan from the content: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableCellFrame.cpp&rev=3.409&mark=658-673#650

Comment 8

[Bernd]

Just to state the obvious:
I have *no* idea how to fix bug 231823 once the back out gets in, gentle slaps with a clue stick are welcome.

The only thing that I can offer are reftests...

Comment 9

[Brian Crowder]

Proposing bernd as an owner for this bug...

Comment 10

[Bernd]

Is there anything in comment 8 that indicates that I have an idea how to fix this.
And this is properly described by the previous ownership of the bug 

Comment 11

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

I'll see what I can do

Comment 12

[Bernd]

Robert if you can come up with an idea how to deal with this, that would good enough to get me started again. Its just that I feel like to be between a rock and a hard place.

Comment 13

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

> Now with the information that the row did not fit with when reflowed with an
> unconstrained height we know that the row got clamped when it is all over
> sudden complete and does fit into the small space.
> Thats is the magical function of the = sign that I removed.

In this case I think the row should NOT be marked complete. Blocks mark themselves incomplete in this situation:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.942&mark=1327#1310

Do you see any problem with marking the row incomplete here? I'll try it.

Comment 14

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

> We will try to split in between the first and the second row
> We will reflow the cell with a 0px available height

Why reflow the cell with 0 available height? Why not the true available height?

Comment 15

[Bernd]

The 0 is  the height the row asked for in a unconstrained height reflow. Basically comment 14 is what David has done in bug 231823 which leads to a conflict with the SplitSpanningCell routine, which result in a crash (bug 373400).

But maybe we should go on bug 373400 the other way and adapt SplitSpanningCells (http://mxr.mozilla.org/mozilla/source/layout/tables/nsTableRowGroupFrame.cpp#947) to the sane way of business. I am not sure that is feasible.

Comment 16

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

Let me try to fix comment #13. If that works OK then we'll see what's fixed and whether we can ship that.

Comment 17

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

Attachment: proof of concept

This patch for comment #13 fixes the test in this bug. It's super-simple. I haven't done any more testing yet, but you might want to give it a try yourself ... I'm travelling to Canberra today so I just wanted to get this out. I'll do some more tests and try to create some reftests.

Comment 18

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

It sort of fixes http://mxr.mozilla.org/mozilla/source/layout/html/tests/table/printing/bug85738.html ... the layout is OK but the second page doesn't print a cell border. I'll look into that.

Comment 19

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

It does fix the testcase in bug 422249. Well, the vertical-align gets applied to the cell contents on each page, but that's probably been our behaviour forever and perhaps is not a bug.

Comment 20

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

Looks like the problem in bug85738.html is just a border painting problem, should be easy to fix...

Comment 21

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

Yeah, the border problem is just that continuation cells think they're empty if they have no children:
  SetContentEmpty(0 == kidSize.height);
We need a better way to determine whether the cell is empty and it should return the same value for all cell frames in a flow chain.

Comment 22

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

Attachment: fix

Okay here it is.

The change to nsTableCellFrame fixes the issue of continuation cells appearing empty.

There's a change to rename 'paginatedHeight' to the better name 'cellMaxHeight'.

The core of the patch is to reorganize the row height so that if the style height exceeds the available height, we mark the row incomplete.

There's a test for this bug, and I also added reftests for the testcases in bugs 231823 and 422249.

Comment 23

[Bernd]

    // XXX this is a bad way to check for empty content. There are various	
    // ways the cell could have content but the kid could end up with zero
    // height.

There is an old bug about it: bug 76331. previously we did check for both height and width (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableCellFrame.cpp&rev=3.364.10.1&mark=871-873#865)

Comment 24

[Bernd]

Comment on attachment 313606 [details] [diff] [review]
fix

I would like to see that the comment about empty cell handling is expanded:
1. referencing what should be done by providing a link to http://www.w3.org/TR/CSS21/tables.html#empty-cells
2. referencing the bug about the issue: bug 76331

Comment 25

[David Baron :dbaron:]

Comment on attachment 313606 [details] [diff] [review]
fix

sr=dbaron

Comment 26

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

Checked in

Comment 27

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041217 Minefield/3.0pre ID:2008041217 and the testcase.

--> Verified fixed