326551 - text in table cells not painted when visibility goes from collapsed to visible, part 2

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

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.
"

Comment 1

[kmike]

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

Comment 2

[Bernd]

Robert could  this be a missing invalidate?

Comment 3

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

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.

Comment 4

[Bernd]

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.

Comment 5

[Bernd]

Attachment: patch that fixes the reflow

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.

Comment 6

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

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?

Comment 7

[Bernd]

Attachment: testcase

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.

Comment 8

[Bernd]

Attachment: fix for the paint error

Comment 9

[Bernd]

Attachment: fix for the overflow area

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

Comment 10

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

(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 11

[Bernd]

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.

Comment 12

[Bernd]

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.

Comment 13

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

(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.)

Comment 14

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

Attachment: testcase with a cell spanning through a collapsed row

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

Comment 15

[Bernd]

Attachment: patch

Comment 16

[Bernd]

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.

Comment 17

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

+  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.

Comment 18

[Bernd]

Attachment: cellspacing illustration

Comment 19

[Bernd]

Attachment: revised patch

> +  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

Comment 20

[Bernd]

>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 21

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

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?

Comment 22

[Bernd]

>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.

Comment 23

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

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 24

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

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.

Comment 25

[Bernd]

fix checked in