380227 - Tables with percentage height don't shrink when browser is resized.

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase

Tables of the form 
<table height="80%" bgcolor="009999">
will grow as the browser size increases, but don't shrink as the browser is shrunk.

I'm pretty sure I localized the problem in the code -- it's in  nsTableFrame::CalcDesiredHeight.  Here's what happens in my test case:

CalcDesiredHeight starts out by setting desiredheight to the *old* height of the row group, at line 3007:
    desiredHeight += rg->GetSize().height + cellSpacingY;
Then, it gets the *updated* height, tableSpecifiedHeight, from the reflow state.  It checks if this new height is greater than desiredHeight, and if it is, it distributes the difference amongst the table's rows with DistributeHeightToRows.  This is all good so far.  However, we don't handle the case where tableSpecifiedHeight < desiredHeight.  We don't update desiredHeight, or shrink any of the rows, which is why the table doesn't shrink.

It seems that, in this (unhandled) tableSpecifiedHeight < desiredHeight case, we should do something similar to the tableSpecifiedHeight > desiredHeight case.  The only difference is that, instead of distributing extra space amongst the rows, we need to distribute shrinkage to the rows.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Testcase 2: nested tables -- more complex, and more bad.

Bug is worse in this test case -- just about any resizing at all makes the tables grow in size, stretching them the page, even though the outermost table has height = 10%.

I'm assuming this is caused by the same bug (tables never getting shrunk correctly), although there may be another bug interacting.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> Tables of the form 
> <table height="80%" bgcolor="009999">
> will grow as the browser size increases, but don't shrink as the browser is
> shrunk.

Correction --  tables only grown then the browser is resized *diagonally*.  A pure vertical resize doesn't trigger the table reflow, for some reason.

So, this can be thought of as two issues:
 1. Vertical window resizes aren't triggering percentage-height table reflows.
 2. When I trigger a table reflow by resizing the window diagonally, the table's size can only be increased, and is never decreased. (except by shrinking the browser to be smaller than the table.)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> (In reply to comment #0)
> > Tables of the form 
> > <table height="80%" bgcolor="009999">
> > will grow as the browser size increases, but don't shrink as the browser is
> > shrunk.
> 
> Correction --  tables only grown then the browser is resized *diagonally*.  A
> pure vertical resize doesn't trigger the table reflow, for some reason.
>

Note: The diagonal-resize-only issue, mentioned in comment #2, is fixed by my patch to bug 380004.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Fix using "GeometryDirty" flag

Fixed by adding a SetGeometryDirty(), in cases of vertical and horizontal resizes, and then adding one IsGeometryDirty() check to make sure the cell resize code gets triggered correctly.

Also added some related code-commenting suggested by dbaron.

Comment 5

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

Comment on attachment 264952 [details] [diff] [review]
Fix using "GeometryDirty" flag

>+  // Note: For tables, NS_FRAME_CONTAINS_RELATIVE_HEIGHT specifically means that
>+  // there's a sub-block with a specified *percentage* height. It doesn't
>+  // correspond to the trivial case where the table's rows are auto-height and
>+  // scale to fill the table. (even though this technically could be viewed as a
>+  // "relative height")

Could you move this comment up into the test you're adding (inside the {}, I think).

s/sub-block/inner table frame/
s/even though this technically could be viewed as a "relative height"/even though this would normally mean NS_FRAME_CONTAINS_RELATIVE_HEIGHT should be set/

>+  /** The GeometryDirty flag is intended to allow efficient resizing
>+   *  of a table.  It indicates that the cells' resizing code should
>+   *  be triggered, but the cells' content won't necessarily be marked
>+   *  dirty.

I'd replace the second sentence with:

The NS_FRAME_IS_DIRTY bit implies that all descendants are dirty.  The GeometryDirty flag differs from NS_FRAME_IS_DIRTY by implying that all the parts of the table are dirty, but that resizing optimizations should still apply to the contents of the individual cells.

r+sr=dbaron, but Bernd should also review

Comment 6

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

Actually, in the second part of the nsTableFrame.cpp change, you should just change the definition of the |reflowAllKids| variable instead.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Added dbaron's suggestions

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> I'm pretty sure I localized the problem in the code -- it's in 
> nsTableFrame::CalcDesiredHeight.  Here's what happens in my test case:
> 
> CalcDesiredHeight starts out by setting desiredheight to the *old* height of
> [etc]
> we need to distribute shrinkage to the rows.

Note: My initial diagnosis was somewhat incorrect, because the buggy behavior misled me about what the code was supposed to do.  

In actuality, we're not missing any "shrinkage-distribution" code.  CalcDesiredHeight is supposed to start with each row at its *minimum* height, and then distribute any additional height among the rows.

With the patch, this is what it does.  (But before the patch, it would sometimes start out each row at its *previous* height, which is why it looked like we needed to distribute shrinkage  when the table shrunk)

Comment 9

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

Comment on attachment 265022 [details] [diff] [review]
Added dbaron's suggestions

>+  // Note: For tables, NS_FRAME_CONTAINS_RELATIVE_HEIGHT specifically means that
>+  // there's an inner table frame with a specified *percentage* height. It
>+  // doesn't correspond to the trivial case where the table's rows are
>+  // auto-height and scale to fill the table. (even though this would normally
>+  // mean NS_FRAME_CONTAINS_RELATIVE_HEIGHT should be set

Could you move this up to where you make the SetGeometryDirty call?

Comment 10

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

Although, per bug 380550 comment 2, Bernd is busy, so maybe we should just land this without waiting for him (he can always comment later if he wants to).

Comment 11

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

Although I'm beginning to wonder if the correct condition is that we want SetGeometryDirty if the table has a specified height -- since if there's a change inside a cell that also requires the same type of rebalance.  That could even be related to the problem on gmail with the contacts list.

Comment 12

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

(Or, alternatively, we could remember the information that DistributeHeightToRows needs.)

Comment 13

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

Then again, tables with specified heights probably don't have a lot of rows, so it should be cheap to just SetGeometryDirty().  (We also need to check for row groups with specified heights, if we support those.)

Comment 14

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

(I also wonder if SetGeometryDirty should have a vertical/horizontal distinction, since most of the things that call it either require vertical recalculation or horizontal recalculation, but not both.  In fact, they may even all be vertical.)

Comment 15

[Bernd]

Comment on attachment 265022 [details] [diff] [review]
Added dbaron's suggestions

Just go ahead with checking in as said in comment 10, I can nag later.

>+  // Note: For tables, NS_FRAME_CONTAINS_RELATIVE_HEIGHT specifically means that
>+  // there's an inner table frame with a specified *percentage* height. It
>+  // doesn't correspond to the trivial case where the table's rows are
>+  // auto-height and scale to fill the table. (even though this would normally
>+  // mean NS_FRAME_CONTAINS_RELATIVE_HEIGHT should be set

I am not very happy with the "inner table frame" notion. We have a outer table frame and usually refer to the table frame itself as the inner table frame 
(http://lxr.mozilla.org/seamonkey/search?string=inner%20table%20frame). 

More, I do not really understand what this comment means. For me the description
at http://lxr.mozilla.org/seamonkey/source/layout/generic/nsIFrame.h#166 is very clear and understandable. However does this comment mean it supersedes the description there or does it expand the meaning for a special case. 

My guess is that only if a ancestor of the table frame is pct based this flag should be set. 

Is this flag also set when a on nested table the pct is set on any ancestor

r+ for the code, but IMHO the comment should be better worded

Comment 16

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

(In reply to comment #15)
> I am not very happy with the "inner table frame" notion.

Oh, I guess we usually call the things inside the table the *internal* table frames.

The comment about NS_FRAME_CONTAINS_RELATIVE_HEIGHT needs updating -- for things other than tables, it means has a height that depends on this frame's height, which is usually a percentage, but also needs to be set for a few other frame types that always fill their parent (viewport, frameset).  But rows and row groups also (collectively) fill their parent's height, but we don't set it for them.  It doesn't really matter much which way it is, and in fact it's fine to differ from one case to the other.  Maybe I should switch viewport and frameset back to doing what the comment in nsIFrame.h said (although the parent of nsFrameSetFrame sometimes contains a frameset and sometimes contains something else, I think).

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: Updated patch

From previous patch, changed the condition for calling SetGeometryDirty() to be
 if(aReflowState.mComputedHeight != NS_UNCONSTRAINEDSIZE)


I also took out of the code a check for "NS_UNCONSTRAINEDSIZE != aReflowState.availableWidth", prior to calling DistributeHeightToRows.  (dbaron says the reflow branch made this check unnecessary.)

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: Patch

(same as prev. patch -- except that before, I'd forgotten to take out one of my testcases for another bug from the reftest.list file)

Comment 19

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

Attachment: patch with comments tweaked

I tweaked the comments a bit; this is what I'll land.

Comment 20

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

Above patch checked in to trunk.

Comment 21

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/reftests/bugs&command=DIFF_FRAMESET&file=reftest.list&rev1=1.67&rev2=1.68&root=/cvsroot

Comment 22

[Daniel Holbert [:dholbert]]

The second test case (with the nested tables) is a separate issue, and I've filed a separate bug for that case: Bug 381507

Comment 23

[robinpelgrim]

Attachment: Test case not shrinking table

Test case not shrinking table height due to td width:100% set

Comment 24

[robinpelgrim]

Attachment: Test case not shrinking table height

Test case not shrinking table height (100%) due to long description in td

Comment 25

[robinpelgrim]

Attachment: Test case 'correct' shrinking table

Table shrinks as expected (when window not too small)

Comment 26

[Daniel Holbert [:dholbert]]

Filed bug 402629 to cover these newly-posted testcases.