Closed
Bug 422678
Opened 15 years ago
Closed 14 years ago
style height on table cells not honored during printing
Categories
(Core :: Layout: Tables, defect, P2)
Tracking
()
VERIFIED
FIXED
People
(Reporter: bernd_mozilla, Assigned: roc)
References
(Depends on 1 open bug)
Details
(Keywords: testcase)
Attachments
(6 files)
289 bytes,
text/html
|
Details | |
293 bytes,
text/html
|
Details | |
171 bytes,
text/html
|
Details | |
1.35 KB,
patch
|
Details | Diff | Splinter Review | |
4.54 KB,
patch
|
Details | Diff | Splinter Review | |
16.92 KB,
patch
|
bernd_mozilla
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
the testcase worked in the build from 2006-12-0704 and fails already in 2006-12-08-04
Updated•15 years ago
|
Flags: blocking1.9?
Comment 3•15 years ago
|
||
Seems somewhat related to bug 422589.
> 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.
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....
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])
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
Assignee | ||
Updated•15 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Updated•15 years ago
|
Attachment #309634 -
Attachment is patch: true
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•14 years ago
|
||
Proposing bernd as an owner for this bug...
Assignee: nobody → bernd_mozilla
Reporter | ||
Comment 10•14 years ago
|
||
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
Assignee: bernd_mozilla → nobody
Reporter | ||
Comment 12•14 years ago
|
||
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.
Assignee | ||
Comment 13•14 years ago
|
||
> 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.
Assignee | ||
Comment 14•14 years ago
|
||
> 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?
Reporter | ||
Comment 15•14 years ago
|
||
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.
Assignee | ||
Comment 16•14 years ago
|
||
Let me try to fix comment #13. If that works OK then we'll see what's fixed and whether we can ship that.
Assignee | ||
Comment 17•14 years ago
|
||
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.
Assignee | ||
Comment 18•14 years ago
|
||
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.
Assignee | ||
Comment 19•14 years ago
|
||
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.
Assignee | ||
Comment 20•14 years ago
|
||
Looks like the problem in bug85738.html is just a border painting problem, should be easy to fix...
Assignee | ||
Comment 21•14 years ago
|
||
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.
Assignee | ||
Comment 22•14 years ago
|
||
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.
Attachment #313606 -
Flags: superreview?(dbaron)
Attachment #313606 -
Flags: review?(bernd_mozilla)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
Reporter | ||
Comment 23•14 years ago
|
||
// 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)
Reporter | ||
Comment 24•14 years ago
|
||
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
Attachment #313606 -
Flags: review?(bernd_mozilla) → review+
Comment 25•14 years ago
|
||
Comment on attachment 313606 [details] [diff] [review] fix sr=dbaron
Attachment #313606 -
Flags: superreview?(dbaron) → superreview+
Assignee | ||
Comment 26•14 years ago
|
||
Checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
Comment 27•14 years ago
|
||
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
Status: RESOLVED → VERIFIED
Keywords: testcase
You need to log in
before you can comment on or make changes to this bug.
Description
•