If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
Layout: Tables
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Bernd)

Tracking

({regression, testcase})

Trunk
x86
Windows XP
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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.
"
(Reporter)

Updated

12 years ago
Summary: https://bugzilla.mozilla.org/attachment.cgi?id=147434 → text in table cells not painted when visibility goes from collapsed to visible, part 2
Blocks: 317375

Comment 1

12 years ago
I see this in the Linux builds too (OS -> All)
(Assignee)

Comment 2

12 years ago
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.
(Assignee)

Comment 4

12 years ago
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.
(Assignee)

Comment 5

12 years ago
Created attachment 211497 [details] [diff] [review]
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.
Attachment #211497 - Flags: superreview?(roc)
Attachment #211497 - Flags: review?(roc)
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?
(Assignee)

Comment 7

12 years ago
Created attachment 211515 [details]
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.
(Assignee)

Comment 8

12 years ago
Created attachment 211588 [details] [diff] [review]
fix for the paint error
Assignee: nobody → bernd_mozilla
Attachment #211497 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #211497 - Flags: superreview?(roc)
Attachment #211497 - Flags: review?(roc)
(Assignee)

Comment 9

12 years ago
Created attachment 211589 [details] [diff] [review]
fix for the overflow area

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

Comment 11

12 years ago
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.
Attachment #211589 - Attachment is obsolete: true
(Assignee)

Comment 12

12 years ago
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.)
Created attachment 212135 [details]
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...
(Assignee)

Comment 15

12 years ago
Created attachment 213259 [details] [diff] [review]
patch
Attachment #211588 - Attachment is obsolete: true
(Assignee)

Comment 16

12 years ago
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.
Attachment #213259 - Flags: superreview?(roc)
Attachment #213259 - Flags: review?(roc)
+  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.
(Assignee)

Comment 18

12 years ago
Created attachment 213354 [details]
cellspacing illustration
(Assignee)

Comment 19

12 years ago
Created attachment 213368 [details] [diff] [review]
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
Attachment #213259 - Attachment is obsolete: true
Attachment #213368 - Flags: superreview?(roc)
Attachment #213368 - Flags: review?(roc)
Attachment #213259 - Flags: superreview?(roc)
Attachment #213259 - Flags: review?(roc)
(Assignee)

Comment 20

12 years ago
>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?
(Assignee)

Comment 22

12 years ago
>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.
Attachment #213368 - Flags: superreview?(roc)
Attachment #213368 - Flags: superreview+
Attachment #213368 - Flags: review?(roc)
Attachment #213368 - Flags: review+
(Assignee)

Comment 25

12 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Updated

12 years ago
Depends on: 331344
(Reporter)

Updated

11 years ago
Depends on: 362348
You need to log in before you can comment on or make changes to this bug.