370525 - Nested tables with height=100% exceed height of parent table

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.13) Gecko/20060414] (release) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.9) Gecko/20061211 SeaMonkey/1.0.7] (release) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.2pre) Gecko/20070215 SeaMonkey/1.1] (nightly) (W2Ksp4)

No bug.


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061207 SeaMonkey/1.5a] (nightly, 2006-12-07-08-trunk) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061208 SeaMonkey/1.5a] (nightly, 2006-12-08-08-trunk) (W2Ksp4)

Regressed between these two builds.
See <http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-07+08&maxdate=2006-12-08+09&cvsroot=%2Fcvsroot>.
My first guess would be that bug 300030 is the culprit.

Bug:
The "3 Admins Techniques" area in the top table overlapses the "Équipes d'Animation" area in the middle table.


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070214 SeaMonkey/1.5a] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/2007021504 Minefield/3.0a3pre] (nightly) (W2Ksp4)

Bug still there. (FF & SM)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: Reduced testcase

This testcase fails on the current trunk

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: Reference testcase

Removing the height=100% from the table styles allows the testcase to render as it does pre-reflow branch and the same as other browsers.

Comment 4

[Jesse Ruderman]

Bug 368621 is similar and has the same regression range.  (doctor__j pointed this out in the other bug.)

Comment 5

[Bernd]

So the first question is why does the table gets a non auto height if its parent has auto height. In strict mode the pct height should collapse to auto http://www.w3.org/TR/CSS21/visudet.html#propdef-height

Comment 6

[Bernd]

Attachment: reflow log

As I suspected we have a special height reflow. IMHO this should never happen in standards mode.

Comment 7

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

Unfortunately (perhaps), we've had one particular case of special height reflow (table directly inside table cell) in standards mode for ages, and a lot of major sites depend on it (such as gmail).  Before the reflow branch landed the code was convoluted enough that it wasn't clear that this was present, but when I tried to remove it it became a problem.

Before the reflow branch there were basically three different ways to get a special height reflow:
 1) nsTableCellFrame::NotifyPercentHeight, due to a child of a table cell (quirks mode only)
 2) nsTableFrame::CheckRequestSpecialHeightReflow called from a row group, row, or cell
 3) nsTableFrame::CheckRequestSpecialHeightReflow called from a table, which magically propagated itself up into the nearest ancestor table no matter how far away that ancestor was:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.cpp&rev=3.665&mark=1875-1878#1860

On the reflow branch, I combined (3) into (1) and limited it only to the cases where (1) happens, i.e., where the table is a child of the cell.  However, it had to be done in standards mode to keep gmail and some other major sites working.  (I think gmail was one of the standards-mode sites involved.)

So now the code for (1) has a non-quirk-mode path:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableCellFrame.cpp&rev=3.396&mark=173-178#150
but we're actually less quirky than before.

But we've always had special height reflows in standards mode from (2).

Note that CVS checkin comments on the reflow branches might be more informative about which sites had problems...

Comment 8

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

Sorry, ignore the first paragraph of the previous comment -- I meant to delete it once I remembered / worked out the details.

Comment 9

[Bernd]

Attachment: fix

This fixes the issue here. This is identical to the patch for bug 305975 which got backed out by the reflow branch landing. Maybe David knows why he did back out this. I do not. And I do still not understand the reflow branch code well enough to say what is correct. Anyway I hope I isolated the issue.

Comment 10

[Bernd]

cool one can add attachments not in sync with the bug (I did not see comment 7 and 8) when attaching.

Comment 11

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

And it still wasn't quite clear.

We've always had special height reflows in standards mode due to (2) and (3), except the code for (3) is now part of the code for (1).

And, by combining (3) into (1), I mean that I removed the call to nsTableFrame::CheckRequestSpecialHeightReflow in nsTableFrame::Reflow and changed the bogus skip-tons-of-ancestors code I cited above into an assertion that we never traverse non-table-frames in nsTableFrame::RequestSpecialHeightReflow.  And then I added the code to NotifyPercentHeight that checked for table children even in standards mode (and reorganized nsTableCellFrame::NeedsToObserve so that it matched when we really do need to observe, rather than saying so much more often, and fixed it to make the notifications from inner tables work).

Comment 12

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

Does this bug cover that bug 305975 regressed, or should another bug be filed?

Comment 13

[Serge Gautherie (:sgautherie)]

From a reporter/reader point of view, it seems (that we could say) this bug covers/is that regression.

Comment 14

[Bernd]

> Does this bug cover that bug 305975 regressed, or should another bug be filed?

I don't see that bug 305975 regressed. All that I said is that I proposed the  patch before in that bug.

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> I don't see that bug 305975 regressed.

Regarding my comment 13,
it seems I misunderstood the testcase there:
I (re)tested SM 1.8.1 branch and Trunk and they behave identically.

No visible regression. Sorry.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: testcase (no cellspacing/cellpadding/borders)

Created this alternate testcase with no cellspacing/cellpadding/borders, to make the table-size calculations a bit easier to follow when tracing through the code.

Comment 18

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

So I've added a few bugs as dependencies of this one that fixing when we do and don't do special height reflows should fix as well.

I've also written some testcases at
http://dbaron.org/css/test/2006/percent-height-in-tables (for bug 359481, bug 381507, and I think this bug as well), but probably some more testcases are needed.

I think what needs to happen here is that we need to fix when we do special height reflows of the *descendants* of a table cell.  Right now we do it any time we observe a percent height on a child of the cell; IE/Windows does it only some of the time when that happens, depending on the heights on the cell, row, etc.

In quirks mode we probably want to match IE/Windows as closely as reasonable, and definitely match it in all the cases where Firefox 2 in quirks mode matched it.  In standards mode we probably want to match IE/Windows in all the cases where Firefox 2 in standards mode matched it, but follow the CSS spec in other cases.  (The combination of those requirements may or may not imply having differences between quirks mode and standards mode.)  Note that in Firefox 2 we probably had different behavior when the child of the cell was a table and when it was not (see comment 7 and comment 11); we may need to emulate that.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: reftest (as patch)

Here's a reftest for the bug (to be checked in after the fix)

Made a reftest for both quirks and standards mode.  For each of those, I also made an additional reference that should not be matched. (with a percentage-height on the cell, which should force a special height reflow.)

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: patch

This patch adds a wrapper condition to the requesting-special-height-reflow section of nsTableCellFrame::NotifyPercentHeight.  This added condition checks if the Cell, Row, RowGroup, and Table are all auto-height.  If they *are* all auto-height, then we DON'T do a special-height reflow.  Otherwise (e.g. if any have specified/percentage-height), we do.  

(This matches IE's behavior.)

This patch fixes the testcase, and it passes the layout reftest suite.

Comment 21

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

Could you reuse the AncestorsHaveStyleHeight function in nsTableFrame.cpp to do some of this work?  (You could make it a static method on nsTableFrame.  And while you're there IsPctStyleHeight and IsFixedStyleHeight should not need to null-check aStylePosition, and should be inline.)  Also, it looks like you don't need to set the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bits in NotifyPercentHeight since RequestSpecialHeightReflow does it.

Comment 22

[Daniel Holbert [:dholbert]]

I think we do still need the (In reply to comment #21)
> Also, it looks like you
> don't need to set the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bits in
> NotifyPercentHeight since RequestSpecialHeightReflow does it.

This code was already there, and I don't think it's redundant.  NotifyPercentHeight sets the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit from aReflowState.parentReflowState up to cellRS, and RequestSpecialHeightReflow picks up from there, setting the bit from cellRS up to its containing table.

So, I don't think we're repeating any work there. If you think we can/should safely remove that section anyway, though, I'm happy to do so.

Comment 23

[Daniel Holbert [:dholbert]]

In reply to comment #22)
(Ignore the dangling "I think we do still need the" at the beginning of my last comment...  I probably need to proofread my posts more. :))

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: reftest (updated)

Removed ^M characters that somehow got into the reftests, and removed unnecessary descriptive text that wasn't present in the anti-reftests (!=) and made them trivially pass.

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: patch (updated, incorporating modified AncestorsHaveStyleHeight)

Comment 26

[Daniel Holbert [:dholbert]]

The new patch (attachment 269023 [details] [diff] [review]) fixes the testcases on this bug, as well as those for the duplicate bugs.  (bug 364124, bug 366233, bug 368621)

It also passes the reftest suite, including my new reftest for this bug.

(old patch did as well, but new patch is cleaner)

Comment 27

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

Comment on attachment 269023 [details] [diff] [review]
patch (updated, incorporating modified AncestorsHaveStyleHeight)

>+  // Return true if any of aReflowState.frame's ancestors within the
>+  // containing table have non-auto height. (e.g. pct or fixed height)
>+  // The startAtParent flag indicates whether we should start checking
>+  // at aReflowState, or start checking at its parent (default behavior)
>+  static PRBool AncestorsHaveStyleHeight(const nsHTMLReflowState& aReflowState,
>+                                         PRBool startAtParent = PR_TRUE);

Instead of adding |startAtParent|, rename |aReflowState| to |aParentReflowState| and change the existing caller to pass |aReflowState.parentReflowState|.

With that, r+sr=dbaron.

Comment 28

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

But what about the second and fourth testcases in http://dbaron.org/css/test/2006/percent-height-in-tables that have the word "hello" in them?  Those have a height specified on a different cell in the row.

(Maybe that should be the subject of a followup bug, though.  But this patch does break both of those relative to both IE/Windows and Firefox 2.)

Comment 29

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

(Note that it would make sense to do that as part of bug 359481, since it becomes much simpler once heights are accumulated for rows just like widths are accumulated for columns, which is the core part of bug 359481.)

Comment 30

[Daniel Holbert [:dholbert]]

Attachment: patch (removed |startAtParent| from prev patch)

Incorporated dbaron's suggestions from comment 27

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 269044 [details] [diff] [review]
patch (removed |startAtParent| from prev patch)

(In reply to comment #28)
> But what about the second and fourth testcases in
> http://dbaron.org/css/test/2006/percent-height-in-tables that have the word
> "hello" in them?  Those have a height specified on a different cell in the row.

Yup -- it looks like I missed one case -- when determining whether do to a special height reflow, IE checks not only ancestors, but also *siblings*, for non-auto-height.  (where siblings = cells in the same row)

So, I probably should add another condition to 
nsTableCellFrame::NotifyPercentHeight, to determine whether any cells in the same row have non-auto-height.  Maybe we could add a flag on the row to indicate whether a cell in that row has non-auto-height, so that this check doesn't bog down performance too much?  (I'm worrying about the nasty case of a row that has many cells, all of which have nsTableCellFrame::NotifyPercentHeight called on them).

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: patch (correctly respond to siblings with non-auto-height)

Changes since last patch: we also trigger a special-height reflow if the cell in question (that contains a percentage-height element) has a sibling with non-auto-height.

If the cell in question has rowspan != 1, no special height reflow occurs.  Similarly, siblings with rowspan != 1 don't count towards triggering special height reflow.

(This all matches IE 7's behavior)

Also: incorporated reftests into this patch, including new reftests to test all these special cases and variations on them.

Comment 33

[Daniel Holbert [:dholbert]]

Attachment: patch (from previous, just made minor changes to reftest.list file)

The reftest.list in my previous patch (attachment 269327 [details] [diff] [review]) doesn't jive with today's trunk, because of changes in CVS near my edits.  (I also noticed that my modifications in that file weren't quite in the right alphabetical position -- I was off by one line at my insertion point)

This new patch fixes both of those issues.

Comment 34

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

So this looks pretty good, but the one change I'd make is I'd rather have the bit on the row frame than on the reflow state.  There are spare frame state bits for row frames -- only the two at the top of nsTableRowFrame.h should currently be used (out of the 12 NS_FRAME_IMPL_RESERVED bits).  So if you use that instead, you can also set it at the beginning of nsTableRowFrame::Reflow rather than in nsTableRowGroupFrame::Reflow.  This keeps the code involved a good bit more concentrated and doesn't put a table-specific concept into nsHTMLReflowState unnecessarily.

Comment 35

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

(That said, the way you did it isn't unreasonable, but I think it's cleaner as a bit on the frame.)

Comment 36

[Daniel Holbert [:dholbert]]

Attachment: patch (using bit on the frame)

Agreed, it's cleaner using a bit on the frame.  Here's the patch with that change.

Comment 37

[Daniel Holbert [:dholbert]]

(re-tested this final patch on the reftest suite, including the new reftests, and it still passes.)

Comment 38

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

Attachment: dbaron review comments

This is a diff of diffs showing my review comments.  They're all pretty minor, so I just pulled the patch in my tree and edited it myself so I can land it (after lunch).

The changes are:

 * change "multi-row-spanning" to "row-spanning" in multiple places

 * rename CheckHasCellWithStyleHeight to InitHasCellWithStyleHeight, since it initializes a bit

 * eliminate the myRowSpan variable since it's only used once, and it's slightly cheaper to not compute it if we don't need to

 * convert the else to a continue in InitHasCellWithStyleHeight, so the main flow of the logic isn't excessively indented

 * convert the 2 nested ifs to an && in InitHasCellWithStyleHeight

 * reorder the state bits in nsTableRowFrame.h so they're in numerical oder

 * rewrite the comment above InitHasCellWithStyleHeight

Comment 39

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

Checked in a few hours ago.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Is this in-testsuite+ now?

Comment 41

[Daniel Holbert [:dholbert]]

(In reply to comment #40)
> Is this in-testsuite+ now?

Yup.

Comment 43

[(no longer active)]

David, I'm wondering if the notref test cases here are correct.  I have a set of patches in bug 10209 which make the notref cases render the same way as the ref cases.  I'm not touching the table reflow code, but so far I've tracked it down to the fact that my patch changes mCBReflowState for some frame types.

I've modified my patch to restore the old behavior but I just wanted to make sure that the current behavior is the correct one.  Webkit and Opera don't seem to agree with us in the rendering.

Comment 44

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

Our current behavior is totally broken *and* has been reverse-engineered by other browsers.  Fixing that is bug 359481, which needs to happen before IE6 disappears from the Web or we'll be stuck with it.

I'd be a little uncomfortable with changing it as the side-effect of another patch, though, without thinking through exactly what the change in behavior is.

Comment 45

[(no longer active)]

(In reply to comment #44)
> I'd be a little uncomfortable with changing it as the side-effect of another
> patch, though, without thinking through exactly what the change in behavior
> is.

OK, fair enough.  :-)