219220 - Table overflow is not visible

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Mats Palmgren (inactive)]

** This bug occurs in a build with bug 173277 fixed **

Spawned off from bug 197236.

STEPS TO REPRODUCE:
1. load http://2cvfriend.org/dck-midt.html
2. click on "Arrangementer" in the top row
3. click on "Forårstur" in the left column

This is 100% reproducible but you have to clear the cache between each try.

ACTUAL RESULTS:
The table overflow (half the image and the text below) is not visible.
No vertical scrollbar.

EXPECTED RESULTS:
Visible overflow and vertical scrollbar.

BUILDS & PLATFORMS TESTED:
Bug occurs in Mozilla nightly trunk build 2003-09-13-22 on Linux

ADDITIONAL INFORMATION:
The scrollbar does not appear sometimes even when the overflow is visible,
that is probably a duplicate of bug 211641

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Mats Palmgren (inactive)]

*** Bug 222840 has been marked as a duplicate of this bug. ***

Comment 3

[José Jeria]

*** Bug 226842 has been marked as a duplicate of this bug. ***

Comment 4

[Mats Palmgren (inactive)]

*** Bug 221355 has been marked as a duplicate of this bug. ***

Comment 5

[Mats Palmgren (inactive)]

*** Bug 224553 has been marked as a duplicate of this bug. ***

Comment 6

[Mats Palmgren (inactive)]

*** Bug 227104 has been marked as a duplicate of this bug. ***

Comment 7

[Bernd]

Mats, the testcase is wfm winxp 2003112508, can you modify it?

Comment 8

[Mats Palmgren (inactive)]

I can still reproduce with 2003-11-10-05 on Linux.
Unfortunately, I am blocked by bug 227063 from testing later builds...
I'll see if I can distill the latest duplicates into a testcase though.

Are you seeing the red dashed top/bottom border in the testcase?

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #2

This is distilled from various product listings at http://www.dabs.com

Comment 10

[José Jeria]

Do not see any green block (attachment 136569 [details]) with build 20031128, Windows XP.

Comment 11

[David Brittain]

I am seeing the problem at dabs. And also don't see the green block. Should OS
be all.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031207
Firebird/0.7+

Comment 12

[David Brittain]

*** Bug 228037 has been marked as a duplicate of this bug. ***

Comment 13

[Jason Barnabe (np)]

*** Bug 229181 has been marked as a duplicate of this bug. ***

Comment 14

[Bernd]

Attachment: patch

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 137876 [details] [diff] [review]
patch

>Index: nsTableFrame.cpp

In the changes to IR_TargetIsChild -- what if the child has much less overflow
than before?  It looks like you'd now be reporting too much overflow.  Isn't
the right thing to do to recover the overflow from all the other children?  (Or
is that too expensive?	Is there someplace where you already iterate over the
other children?)


I haven't looked at the other files yet...

Comment 16

[Bernd]

Comment on attachment 137876 [details] [diff] [review]
patch

>Isn'tthe right thing to do to recover the overflow from all the other children? 
>(Or is that too expensive?

Thats correct, I was afraid to recover the overflow area in the IR case. I
considered  the too large overflow area during an incr. reflow a transient
phenomena, as if there is enough happening on the table cells it will trigger a
rebalance or even layout strategy init which will then cause a resize reflow on
all table children. 

On the other side, I think my fear is a typical case of premature optimization.
The usual table has between one and three row group children, so we are
speaking of typically one or two recovery operations. They can't be that
expensive.

Comment 17

[Bernd]

Attachment: revised patch

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For the future, it would be helpful if you used diff -p (which shows diff's
(usually correct) guess at the function name on the @@ line), and probably more
context as well.  In my ~/.cvsrc, I have the line "diff -pNu12".

Comment 19

[José Jeria]

*** Bug 230208 has been marked as a duplicate of this bug. ***

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 139136 [details] [diff] [review]
revised patch

>Index: nsTableFrame.cpp
>+    desiredSize.mOverflowArea = nsRect(0, 0, mRect.width, mRect.height);
>+    nsVoidArray rowGroups;
>+    PRUint32 numRowGroups;
>+    OrderRowGroups(rowGroups, numRowGroups, nsnull);
>+    for (PRUint32 rgX = 0; rgX < numRowGroups; rgX++) {
>+      nsIFrame* kidFrame = (nsIFrame*)rowGroups.ElementAt(rgX);
>+      nsTableRowGroupFrame* rgFrame = GetRowGroupFrame(kidFrame);
>+      ConsiderChildOverflow(aPresContext, desiredSize.mOverflowArea, rgFrame);
>+    }

Is there a good reason for doing the extra work of |OrderRowGroups| rather than
just walking through the child list?  If so, leave it, but if not, I'd prefer
if you just walked the child list (as OrderRowGroups does).

>Index: nsTableRowFrame.cpp
>@@ -1220,6 +1220,7 @@
>     nscoord oldCellDesDescent   = oldCellDesSize.height - oldCellDesAscent;
>+
>     
>     // Reflow the cell passing it the incremental reflow command. We can't 

seems extraneous, but fine if you really want it

>@@ -1319,12 +1320,25 @@
>         Invalidate(aPresContext, dirtyRect);
>       }
>     }

I really don't understand what's going on here.  Why do we do work only if the
heights are equal?  Is there something else that does it if they're unequal? 
Where?	(I looked, and couldn't find it.)

Also, inside that code, can you get rid of the ConsiderChildOverflow call,
since you're doing it below?  (The cell should have called StoreOverflow,
right?)

>+    else {
>+      nsIFrame* cellKidFrame = cellFrame->GetFirstChild(nsnull);
>+      if (cellKidFrame) {
>+        cellFrame->ConsiderChildOverflow(aPresContext, cellMet.mOverflowArea, cellKidFrame);
>+        cellFrame->StoreOverflow(aPresContext, cellMet);
>+       }
>+    }

Shouldn't the cell frame's reflow method be doing this?

>Index: nsTableRowGroupFrame.cpp
>-        ConsiderChildOverflow(aPresContext, aDesiredSize.mOverflowArea, aNextFrame);

I assume something else does this, but I don't see where.

>@@ -1746,6 +1745,13 @@
>       // rect of what changed. Or whether anything moved or changed size...
>       nsRect  dirtyRect(0, 0, mRect.width, mRect.height);
>       Invalidate(aPresContext, dirtyRect);
>+    }
>+    else {
>+      // need to recover the  OverflowArea
>+      for (nsTableRowFrame* rowFrame = GetFirstRow(); rowFrame; rowFrame = rowFrame->GetNextRow()) {
>+        ConsiderChildOverflow(aPresContext, aDesiredSize.mOverflowArea, rowFrame);
>+      }
>+      StoreOverflow(aPresContext, aDesiredSize);
>     }
>   }

What about the else for the if one level out?  Doesn't that need the same code?

Comment 21

[Bernd]

Attachment: rev 3

>Is there a good reason for doing the extra work of |OrderRowGroups| rather
than
>just walking through the child list?  If so, leave it, but if not, I'd prefer
>if you just walked the child list (as OrderRowGroups does).

Changed, no there is no good reason, it was simply a copy operation, but there
is a good reason to use the kidFrame instead as rowgroup frames may be
encapsulated into scrollable frames.

>seems extraneous, but fine if you really want it

fixed

>I really don't understand what's going on here.  Why do we do work only if the

>heights are equal?  Is there something else that does it if they're unequal? 
>Where? (I looked, and couldn't find it.)

>Also, inside that code, can you get rid of the ConsiderChildOverflow call,
>since you're doing it below?  (The cell should have called StoreOverflow,
>right?)

No, this is done in cellframe::VerticalAlignChild

>Shouldn't the cell frame's reflow method be doing this?


if (!aDesiredSize.mNothingChanged) {
      aTableFrame.CellChangedWidth(*cellFrame, oldCellMinWidth,
oldCellMaximumWidth); 
} 

This innocent looking line is contains the trigger logic for adapting the table
to the changing cells parameters. If the cell widths or the MEW changes a
strategy or rebalance will be issued. If this happens we can stop to bother
about the computations because everything will be reflown anyway. However if
the height changes this go through smooth and then special love for the table
cell is necessary.

>I assume something else does this, but I don't see where.

 if (needToCalcRowHeights) {

then CalculateRowHeights is called  which calles DidResizeRows which stores the
overflow area

>What about the else for the if one level out?	Doesn't that need the same
code?

Then the table frame needs a reflow anyway, so all computations will be
overwritten anyway.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 139802 [details] [diff] [review]
rev 3

>+    else { // we dont reallign vertical but we need to update the overflow area

s/dont reallign vertical/don't realign vertically/

Comment 23

[Bernd]

fix checked in

Comment 24

[Andrew Schultz]

*** Bug 233957 has been marked as a duplicate of this bug. ***

Comment 25

[PikeUK]

*** Bug 243404 has been marked as a duplicate of this bug. ***