{inc}table inside fixed-height div with fixed-height tbody loses its height on incremental reflow

RESOLVED FIXED

Status

()

Core
Layout: Tables
--
major
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: John Duperon, Assigned: Bernd)

Tracking

({regression, testcase})

Trunk
regression, testcase
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

This bug is really odd and annoying.  If a table is within a div and it has a table body and the body has links within it, the rows are spaced widely apart and, when the link is clicked, they move closer together.  When the rows touch, the link is fired.  It only happens in Firefox 3, so far confirmed on Windows and Mac.  In Firefox 2, the rows are shown close together as they should be and clicking on the link the first time fires it.

Reproducible: Always

Steps to Reproduce:
View this code in Firefox 3 and try to click a link.  You will immediately know what I mean.

<html>
<head>
<title>Table Test</title>
</head>

<body bgcolor="#FFFFFF" text="#000000"  style="background: white;">
<div id="mainlayer"  style="height:550px;">

<div id="div2">
        <table border=0 width=375px align="center" cellspacing=0 class="smalltext" cellpadding=0>
         <tbody style="overflow-x: hidden; overflow-y: scroll; border-bottom: solid 1px #97B8C5;border-left: solid 1px #97B8C5;" height="100px">
        
        <tr>
        <td width=1px></td>
        <td align="center"><a href=javascript:alert("Clicked!">click_me</a></td>
        <td align="center">thingy1</td>
        <td align="center" style="border-right: solid 1px #97B8C5;">+/-</td></tr>
        
        <tr>
        <td width=1px></td>
        <td align="center"><a href=javascript:alert("Clicked!")>click_me</a></td>
        <td align="center">thingy2</td>
        <td align="center" style="border-right: solid 1px #97B8C5;">+</td></tr>
        
        <!-- <tr> -->
        <tr><td width=1px></td><td>&nbsp;</td><td></td><td style="border-right: solid 1px #97B8C5;">&nbsp;</td></tr>
        </tbody>
        </table>
</div>  <!-- div2 -->

</div>  <!-- mainlayer -->

</body>
</html>



I was not using any special themes or anything else out of the ordinary.  This bug is a pretty big one as it will break many people's web sites (including some of our software, which is how I found it).

Comment 1

10 years ago
Confirming on Mac: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008061304 Minefield/3.0pre

I don't see any duplicate bugs, but I could be missing something....
Status: UNCONFIRMED → NEW
Component: General → Layout: Tables
Ever confirmed: true
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → layout.tables
Hardware: PC → All
Version: unspecified → Trunk

Comment 2

10 years ago
Created attachment 325451 [details]
testcase

This bug is only in quirksmode.

Please add keywords "testcsase" and "regression".

Dup of ??

Updated

10 years ago
Attachment #325451 - Attachment mime type: application/octet-stream → text/html
Regression range is 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1196822580&maxdate=1196824139
so it was caused by Bug 375304 or Bug 406568.
Blocks: 406568
Keywords: regression, testcase
Flags: wanted1.9.1?
Summary: Tables with table body's in div's have strange spacing/link behavior → {inc}table inside fixed-height div with fixed-height tbody loses its height on incremental reflow
Created attachment 328174 [details]
Testcase 2 (derived from testcase 1 of j.j.)

Notice also that this bug doesn't happen if height is specified for table. It happens only if tbody has a specified height.

Also Bug 424585 happens only with tbody element. Is something related?
Attachment #325451 - Attachment is obsolete: true
Flags: wanted1.9.1? → wanted1.9.1+

Updated

10 years ago
Attachment #325451 - Attachment is obsolete: false
I'm not sure about the regression range. This seems to be a bug in table height calculation code. I notice that when you click on the link we do a reflow but we don't go through nsTableRowGroupFrame::CalculateRowHeights; presumably needToCalcRowHeights is false.

The comment for CalculateRowHeights says
// Even if rows don't change height, this method must be called to set the
// heights of each cell in the row to the height of its row.
Which makes me wonder why we are optimizing away calls to it.
(Assignee)

Comment 6

10 years ago
CalculateRowHeights is expensive, Chris tried to minimize the calls to it. Its basically doing the same what does BasicTableLayoutStrategy to widths only to heights.

The general rule of thumb is if a height bug occurs only in quirks mode one has to suspect special table height reflows.
bernd, would you able to take this? I'm not all that excited about learning about special table height reflows.
(Assignee)

Comment 8

10 years ago
> I'm not all that excited about learning about special table height reflows.
Yes this is very understandable, I didn't want that "excitement" for years ;-). But I would really get motivated by a line reflow fix for bug 428263.
(Assignee)

Comment 10

10 years ago
The code did not survive the reflow branch landing 
old code: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.344.10.4&mark=1673-1706#1670
new code: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.409&mark=463-486#460

The difference is in the check of the row height, which previously caused a row height calculation but today does not.  
(Assignee)

Comment 11

10 years ago
forget comment 10 its garbage
(Assignee)

Comment 12

10 years ago
in prinicple if (aReflowState.tableFrame->IsAutoHeight()) {  should be expanded to check for auto height on the rowgroup too. Which leads to the first question how to handle pct heights on rowgroups. The next thing that comes up is the if tableFrame->IsAutoHeight() implementation that seems strange and i am not sure that it is correct for strict mode.
(Assignee)

Comment 13

10 years ago
Created attachment 336229 [details] [diff] [review]
patch
(Assignee)

Comment 14

10 years ago
Created attachment 336253 [details] [diff] [review]
patch with reftest
Attachment #336229 - Attachment is obsolete: true
Attachment #336253 - Flags: superreview?(roc)
Attachment #336253 - Flags: review?(roc)
Would dholbert or dbaron be a better reviewer than me?
(Assignee)

Updated

10 years ago
Attachment #336253 - Flags: review?(roc) → review?(dholbert)
(Assignee)

Comment 16

10 years ago
Comment on attachment 336253 [details] [diff] [review]
patch with reftest

yeah Daniel is a real table height lover.....
Comment on attachment 336253 [details] [diff] [review]
patch with reftest

Patch makes sense. Basic result seems to be that, for table row groups with height-unit == eStyleUnit_Coord inside of auto-height tables, we'll make sure to call CalculateRowHeights, addressing Comment 5.

A few nits:

+++ b/layout/reftests/bugs/439639-1-ref.html
@@ -0,0 +1,26 @@
+
+<html>
[ ... ]
+</html>
\ No newline at end of file

Remove initial empty line, and add newline at end of file.

>+++ b/layout/reftests/bugs/439639-1.html
>\ No newline at end of file

Add newline at end of file here, too.

>+          if (aReflowState.tableFrame->IsAutoHeight() &&
>+              (unit == eStyleUnit_Auto || 
>+               unit == eStyleUnit_None ||
>+               unit == eStyleUnit_Percent)) {

It'd be faster & still correct to just check for unit != eStyleUnit_Coord, rather than individually checking the 3 other options, right?  (AFAIK, Auto/None/Percent/Coord are the only values expected for nsStylePosition::mHeight.GetUnit(), based on the comment in the nsStylePosition declaration, in nsStyleStruct.h)

r=dholbert with these changes.
Attachment #336253 - Flags: review?(dholbert) → review+
(Assignee)

Comment 18

10 years ago
Created attachment 338431 [details] [diff] [review]
revised patch
Attachment #338431 - Flags: superreview?(roc)
(Assignee)

Updated

10 years ago
Attachment #336253 - Flags: superreview?(roc)
Attachment #338431 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 19

10 years ago
taking the bug as I fixed it yesterday, 

Robert: just broad hint - I did my half of comment 8 -
Assignee: roc → bernd_mozilla
(Assignee)

Comment 20

10 years ago
http://hg.mozilla.org/mozilla-central/rev/63bc896857c9
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.