Nested percentage-height tables grow endlessly on any window resize

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: dholbert, Assigned: dbaron)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
Created attachment 265574 [details]
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)
(Reporter)

Comment 1

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

Comment 2

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

Comment 3

12 years ago
Is there a bug for that separation?
(Assignee)

Comment 4

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

Updated

12 years ago
Depends on: 359481
(Assignee)

Comment 5

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

(Assignee)

Comment 6

12 years ago
Created attachment 265855 [details]
testcase with inner div instead of inner table

This one shows oscillation instead of continuous increase.
(Assignee)

Comment 7

12 years ago
(And this means that bug 359481 actually wouldn't fix this bug.)
(Assignee)

Updated

12 years ago
Blocks: 363496
No longer depends on: 359481
(Assignee)

Comment 8

12 years ago
Created attachment 265979 [details] [diff] [review]
patch

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
Status: NEW → ASSIGNED
Attachment #265979 - Flags: superreview?(roc)
Attachment #265979 - Flags: review?(dholbert)
(Reporter)

Comment 9

12 years ago
Comment on attachment 265979 [details] [diff] [review]
patch

Looks good to me.
Attachment #265979 - Flags: review?(dholbert) → review+
(Assignee)

Updated

12 years ago
Blocks: 368621

Comment 10

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

Updated

12 years ago
Blocks: 372048
It will, but only to reflow down to the table, so it doesn't seem so bad.
(Reporter)

Comment 13

12 years ago
Created attachment 267610 [details] [diff] [review]
reftest (as patch)

Here's a reftest for this bug.  Fails pre-patching, passes post-patching.
Attachment #267610 - Flags: review?(dbaron)
(Reporter)

Comment 14

12 years ago
Created attachment 267618 [details] [diff] [review]
reftest (as patch)

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)
Created attachment 268284 [details] [diff] [review]
patch

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

Updated

12 years ago
Attachment #267618 - Flags: review?(dbaron) → review+
(Reporter)

Updated

12 years ago
Attachment #268284 - Flags: review?(dholbert) → review+
Patch and reftest checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: in-testsuite+

Updated

12 years ago
Blocks: 382779
You need to log in before you can comment on or make changes to this bug.