This is the attachment from bug 242253 (which basically regressed again when the frame display lists patch landed, I believe). See bug 317375, comment 100: " (In reply to comment #99) > https://bugzilla.mozilla.org/attachment.cgi?id=147434 from bug 242253 should > show "foo" as text, this doesn't happen anymore in my debug build with the > patch. This appears to be an incremental reflow issue. The table cell frame seems to be positioned outside the table; forcing a reflow (e.g., by resizing the window) makes the text come back. Not sure why this doesn't show up on trunk. Let's file a separate bug about that, perhaps after this lands. "

I see this in the Linux builds too (OS -> All)

Robert could this be a missing invalidate?

Bernd: I don't think so. Repainting the page doesn't help. It's a reflow issue, the cell gets positioned outside the table. Forcing an incremental reflow fixes it.

Initial reflow: VP 034AF950 r=1 a=14460,15420 c=14460,15420 cnt=163 scroll 034AFB30 r=1 a=14460,15420 c=14460,15420 cnt=164 canvas 034AF9E4 r=1 a=14460,UC c=14460,UC cnt=165 area 034D3AA8 r=1 a=14460,UC c=14460,UC cnt=166 block 034D3C2C r=1 incr. (Dirty) a=14460,UC c=14220,UC cnt=167 text 034D41A0 r=0 a=14220,UC c=UC,UC cnt=168 text 034D41A0 d=0,0 tblO 034D42DC r=0 a=14220,UC c=0,UC cnt=169 tbl 034D4430 r=0 a=14220,UC c=UC,UC cnt=170 rowG 034AFCD8 r=0 a=UC,UC c=UC,UC cnt=171 row 034DD108 r=0 a=UC,UC c=UC,UC cnt=172 cell 034DD390 r=0 a=UC,UC c=UC,UC cnt=173 block 034DD3F0 r=0 a=UC,UC c=UC,UC cnt=174 text 034DD598 r=0 a=UC,UC c=UC,UC cnt=175 text 034DD598 d=300,285 me=300 block 034DD3F0 d=300,300 me=300 cell 034DD390 d=330,330 me=330 row 034DD108 d=UC,330 rowG 034AFCD8 d=UC,330 colG 034D4618 r=0 a=UC,UC c=UC,UC cnt=176 col 034DD83C r=0 a=0,0 c=0,UC cnt=177 col 034DD83C d=0,0 colG 034D4618 d=0,0 rowG 034AFCD8 r=2 a=330,UC c=330,UC cnt=178 row 034DD108 r=2 a=330,UC c=330,UC cnt=179 cell 034DD390 r=2 a=330,UC c=300,UC cnt=180 block 034DD3F0 r=2 a=300,UC c=300,UC cnt=181 block 034DD3F0 d=300,300 me=300 cell 034DD390 d=330,330 me=330 row 034DD108 d=330,330 rowG 034AFCD8 d=330,330 colG 034D4618 r=2 a=330,UC c=330,UC cnt=182 col 034DD83C r=0 a=0,0 c=0,UC cnt=183 col 034DD83C d=0,0 colG 034D4618 d=0,0 tbl 034D4430 d=390,60 o=(0,0) 390 x 360 tblO 034D42DC d=390,60 o=(0,0) 390 x 360 text 034DD8A0 r=0 a=14220,UC c=UC,UC cnt=184 text 034DD8A0 d=0,0 block 034D3C2C d=14220,60 o=(0,0) 14220 x 360 area 034D3AA8 d=14460,300 o=(0,0) 14460 x 480 canvas 034AF9E4 d=14460,480 scroll 034AFB30 d=14460,15420 VP 034AF950 d=14460,15420 incr. reflow: VP 034AF950 r=1 a=14460,15420 c=14460,15420 cnt=187 scroll 034AFB30 r=1 a=14460,15420 c=14460,15420 cnt=188 canvas 034AF9E4 r=1 a=14460,UC c=14460,UC cnt=189 area 034D3AA8 r=1 a=14460,UC c=14460,UC cnt=190 block 034D3C2C r=1 a=14460,UC c=14220,UC cnt=191 tblO 034D42DC r=1 a=14220,UC c=0,UC cnt=192 tbl 034D4430 r=1 a=14220,UC c=UC,UC cnt=193 rowG 034AFCD8 r=1 a=330,UC c=330,UC cnt=194 row 034DD108 r=1 incr. (Style) a=330,UC c=330,UC cnt=195 cell 034DD390 r=3 a=330,UC c=300,UC cnt=196 block 034DD3F0 r=3 a=300,UC c=300,UC cnt=197 text 034DD598 r=3 a=UC,UC c=UC,UC cnt=198 text 034DD598 d=300,285 me=300 text 034DD598 r=3 a=300,UC c=UC,UC cnt=199 text 034DD598 d=300,285 me=300 block 034DD3F0 d=300,300 me=300 m=300 cell 034DD390 d=330,330 me=330 m=330 row 034DD108 d=330,330 rowG 034AFCD8 d=330,330 rowG 034AFCD8 r=2 a=330,UC c=330,UC cnt=200 row 034DD108 r=2 a=330,UC c=330,UC cnt=201 row 034DD108 d=330,330 rowG 034AFCD8 d=330,330 colG 034D4618 r=2 a=330,UC c=330,UC cnt=202 col 034DD83C r=0 a=0,0 c=0,UC cnt=203 col 034DD83C d=0,0 colG 034D4618 d=0,0 tbl 034D4430 d=390,390 tblO 034D42DC d=390,390 block 034D3C2C d=14220,390 area 034D3AA8 d=14460,630 canvas 034AF9E4 d=14460,630 scroll 034AFB30 d=14460,15420 During the incr. reflow the reflowis correct, but during the initial reflow it is not.

The patch fixes the reflow issue but not the painting, we call in both reflows the Invalidate at http://lxr.mozilla.org/seamonkey/source/layout/tables/nsTableFrame.cpp#2128 with the full table area so it should paint in my old style understanding. The cell offset after the style change reflow is zero.

Hmm, is it correct to just subtract from the overflow area? Does it include chilren at this point? (If so, the overflow areas of children might the outer overflow area doesn't change due to collapsing...) How does this fix the bug?

attachment 211497 [details] [diff] [review] fixes only the reflow, it does not fix the painting. The attached testcase shows that we render the cell and its border at its correct position but not the content. The overflow only contains the accumulated child overflow areas.

I think this is a correct overflow computation in the the collapse case.

(In reply to comment #9) > Created an attachment (id=211589) [edit] > fix for the overflow area The overflow area recomputation for rows and groups in the case of column collapsing should be separated out into two helper functions. I think the collapsed-column overflow recomputation needs to ensure that the rowgroup's rect and the row's rect are included in their overflow areas. Why are we ignoring the overflow area contribution of any cell whose spanned cells intersect a collapsed column? Some of the cell still gets painted, right? I still don't understand why fixing the overflow areas fixes the layout bug.

Comment on attachment 211589 [details] [diff] [review] fix for the overflow area The patch still has errors in the row group collapsing. This patch just fixes a hole in the overflow computation that I discovered when looking at the bug. The bug fix is the first patch. Your idea that the cell has a offset was just to the point.

Hey Robert, you did not tell that you touched that code !!! :-) I was today wondering why the following code looks so alien in nsTableCellFrame.cpp nsPoint collapsedOffset; GetCollapseOffset(collapsedOffset); kidOrigin += collapsedOffset; But bonsai is my my friend. I guess you royally horked visibility collapse. So to clean up the mess I will: 1. remove rants in the code like // Clip off content to the left of the x=0 line. This is bogus really, // but the whole handling of collapsed-offset cells is bogus. because rants are not an excuse for breaking things that previously worked. 2. Remove the collapsed offset at all. 3. There is no need to shift cell in y direction if the row is collapsed so is the cell. If it spans out it will be collapsed anyway. If it spans in we will shorten the cell like before. 3a. As there is no cell spanning out of a rowgroup this will be as before rowgroupwise. 4. If we need to collapse columns or colgroups, we will move the cells together with their views in x direction. 5. We will reorder the cell position like we do it in row reflow and resize the row and rowgroup too.

(In reply to comment #12) > because rants are not an excuse for breaking things that previously worked. The old approach could have only worked if we're lucky. It tried to draw the cell contents at a different location simply by adding a translation to the current rendering context. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableCellFrame.cpp&rev=3.370#422 This could never work in general because it means the frame tree does not accurately reflect the displayed positions of content. Put anything nontrivial (e.g., anything with a view) in the cell, and it breaks. The new code I wrote was simply an attempt to place the cell frame where the old code would have drawn it. I apologize for getting it wrong. > 3. There is no need to shift cell in y direction if the row is collapsed so > is the cell. If it spans out it will be collapsed anyway. Okay, I thought part of the cell would still be displayed, and that's what http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableCellFrame.cpp&rev=3.370#422 was trying to achieve. I was wrong. I agree that the cell doesn't need to move, so I don't understand what the heck that code is doing. > If it spans in we > will shorten the cell like before. I'm not sure how this is supposed to work, especially if the cell spans *through* a collapsed row. (I'll attach a testcase in a minute.)

Surprisingly, we get the same results on 1.8 branch and trunk, although I can't say the result is a good one...

Comment on attachment 213259 [details] [diff] [review] patch Robert, this is essentially a rewrite of the collapse code. The core method went to the row frame where it handles row and cols in one sweep.

+ nscoord width = cellSpacingX; Why do you add one unit of cell spacing at the beginning? It would be easier to review if the GetTableFrame change had been made separately. + void CollapseRowGroupIfNecessary(const nscoord& aYTotalOffset, Just 'nscoord'. + nscoord& aYGroupOffset, I think this should be the regular return value of the function. + * @param aYGroupOffset shift due to rows and this rowgroup being collapsed You mean the shift upward that should be applied to subsequent rowgroups? + groupRect.height -= PR_MIN(groupRect.height, aYGroupOffset); Shouldn't this be PR_MAX? We never want to make groupRect.height negative. But if aYGroupOffset is greater than groupRect.height, hasn't something gone horribly wrong? + void CollapseRowIfNecessary(nscoord& aYGroupOffset, I think this should be split into an input-only parameter --- the amount to shift this row upward within the row group --- and an output, which should be the regular result of this function, the amount to shift up subsequent rows within this row group. + while (cellFrame) { + nsRect cRect = cellFrame->GetRect(); + cRect.height -= rowRect.height; I thought you said that cells originating in collapsed rows are totally collapsed, even if they span out. So why are we allowing them to have nonzero height here? Other than that, this looks really great. Much much better.

> + nscoord width = cellSpacingX; > > Why do you add one unit of cell spacing at the beginning? See the illustration attached. The first column is inset by one cellSpacingX, then the code adds for every real column a cellSpacingY after it. The other comments are addressed

>It would be easier to review if the GetTableFrame change had been made > separately. Yes, but after all it was *you*, who drilled me not to tolerate these signatures. ;-)

Comment on attachment 213368 [details] [diff] [review] revised patch + * @return this row causes an additional shift up of the following rows Something like "@return the amount to shift up all following rows" One more question: after we reset the cell frame bounds, are we guaranteed to reflow the cell contents again? How do we know that setting the cell height to zero will actually hide anything, won't the contents just overflow?

>One more question: after we reset the cell frame bounds, are we guaranteed to >reflow the cell contents again? No, the opposite, we are past the cell reflow (thats what I think also is required by CSS 2.1) >How do we know that setting the cell height to >zero will actually hide anything, won't the contents just overflow? The row/rowgroup will not paint and it will not paint its children regardless of the overflow. It might be that we could even skip the sizing to 0.

Then why do we bother changing the heights of cells that span through collapsed rows? That won't really have any effect without reflowing the cell contents. Although I guess it won't have any effect even if we do reflow the cell contents, in most cases.

Comment on attachment 213368 [details] [diff] [review] revised patch I'm not really sure about the cell height changes, but this patch is still a huge win over the current approach.

fix checked in