Closed Bug 381507 Opened 15 years ago Closed 15 years ago

Nested percentage-height tables grow endlessly on any window resize


(Core :: Layout: Tables, defect)

Not set





(Reporter: dholbert, Assigned: dbaron)




(4 files, 2 obsolete files)

Attached file testcase
Under certain nested-table conditions, the tables will grow whenever the window is resized in any way.

the certain conditions are: 
 - both tables have percentage height
 - Outer table contains some content above the inner table, in the same cell
 - inner table has its percentage large enough to extend outside the outer table.

(e.g. see the test case)
The problem is basically a positive feedback loop between the outer cell's block and the interior table.  It goes as follows:

- Initial state: 
  Outer Table height = 3000
  Row Group height = 3000
  Cell Height = 3000
  Block Height = 4000
  Inner Table height = 3000
(the Block is larger than everything else because it has some other content, in addition to the interior table. For example, this content is the upper '+' symbol in the test case.  I'm assuming this content is 1000 units high.)

- First reflow of cell:
  Block's height gets propagated up to the nsCellFrame and then the nsRowGroupFrame, so those both become 4000 units high

- Second reflow of cell:
  * Below nsBlockFrame::ReflowDirtyLines:
   Block's height is used to recalculate the height of the inner table, so the inner table becomes 4000 units high
  * When nsBlockFrame::Reflow completes, the Block has recalculated its height based on the (new) height of the inner table.  Now, block height = 4000 + 1000 = 5000

- At some later stage of reflow, the Row Group height gets propagated up to the Outer Table, so the Outer table ends up 4000 units high.

So we end up with everything 1000 units taller:
  Outer Table height = 4000
  Row Group height = 4000
  Cell Height = 4000
  Block Height = 5000
  Inner table height = 4000

This process repeats on each reflow, including those triggered by for horizontal resizes.
dbaron says that this is partly caused by the fact that we're using the same flag for two very different kinds of special-reflow:
 1. when there's percentage height on cells and rows
 2. when there's percentage height on children of a cell (e.g. this bug's testcase)

These two types of special reflow need to be separated out in a better way, and until that separation happens, dbaron says it's probably not worth trying to fix this bug.
Is there a bug for that separation?
Not directly caused by that distinction, but probably hard to fix without cleaning it up (although maybe not).  We want to handle (2) based on a first-pass calculation of heights, and we want to get NS_FRAME_CONTAINS_RELATIVE_HEIGHT set in that case (which I realize we could perhaps do in NotifyPercentHeight).

The messier part is that we want to set heights differently during the first pass and then do a second pass with the real heights.  This may just involve undoing some changes I did on the reflow branch (bug 300030).

(In reply to comment #3)
> Is there a bug for that separation?

Bug 359481.
So the problem here is that using a vertical resize isn't really sufficient.  In the later first pass, before the special height reflow, we give the inner table a reflow with mVResize set.  But the inner table now has an auto mComputedHeight (although it didn't before).  But since it has an auto height, it just determines its height from its children, and thus basically returns its own height.

This one shows oscillation instead of continuous increase.
(And this means that bug 359481 actually wouldn't fix this bug.)
Blocks: 363496
No longer depends on: table-height-rewrite
Attached patch patch (obsolete) — Splinter Review
So this fixes this bug (and hopefully a bunch of others; I'll test after lunch).

The basic problem is that the vertical resizing optimizations I made assume that a frame doesn't switch between 'auto' and non-'auto' heights without a style change.  Special height reflows, where the first pass is 'auto' and the second pass is non-'auto', break that assumption.  This works around the problem in two places.
Assignee: nobody → dbaron
Attachment #265979 - Flags: superreview?(roc)
Attachment #265979 - Flags: review?(dholbert)
Comment on attachment 265979 [details] [diff] [review]

Looks good to me.
Attachment #265979 - Flags: review?(dholbert) → review+
Blocks: 368621
The testcase HTML has a typo: "</tr></td>".
+       // outer table for such a child
+       frame->GetType() == nsGkAtoms::tableOuterFrame ||

You're not really testing "such a child" ... you're testing for any table outer frame whether the inner table depends on the containing block height or not. That actually seems kinda bad since this will be triggered for any directly nested table, right?
Blocks: 372048
It will, but only to reflow down to the table, so it doesn't seem so bad.
Attached patch reftest (as patch) (obsolete) — Splinter Review
Here's a reftest for this bug.  Fails pre-patching, passes post-patching.
Attachment #267610 - Flags: review?(dbaron)
oops -- first version of reftest still had the typo Jesse mentioned in Comment 10.  Fixed in this version.
Attachment #267610 - Attachment is obsolete: true
Attachment #267618 - Flags: review?(dbaron)
Attachment #267610 - Flags: review?(dbaron)
Attached patch patchSplinter Review
Here's a somewhat more conservative patch; it's also somewhat more symmetric between the changing to special-height and changing from special-height transitions.
Attachment #265979 - Attachment is obsolete: true
Attachment #268284 - Flags: superreview?(roc)
Attachment #268284 - Flags: review?(dholbert)
Attachment #265979 - Flags: superreview?(roc)
Attachment #268284 - Flags: superreview?(roc) → superreview+
Attachment #267618 - Flags: review?(dbaron) → review+
Attachment #268284 - Flags: review?(dholbert) → review+
Patch and reftest checked in to trunk.
Closed: 15 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Blocks: 382779
Blocks: 234964
Depends on: 438509
You need to log in before you can comment on or make changes to this bug.