Closed Bug 439639 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: duperon, Assigned: bernd_mozilla)

References

Details

(Keywords: regression, testcase)

Attachments

(4 files, 1 obsolete file)

testcase
302 bytes, text/html
Details
Testcase 2 (derived from testcase 1 of j.j.)
816 bytes, text/html
Details
patch with reftest
3.69 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
revised patch
3.54 KB, patch
roc
: superreview+
Details | Diff | Splinter Review 
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).
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
Attached file testcase — 
This bug is only in quirksmode.

Please add keywords "testcsase" and "regression".

Dup of ??
Attachment #325451 - Attachment mime type: application/octet-stream → text/html
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
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+
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.
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.
> 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.

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.  
forget comment 10 its garbage
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.
Attached patch patch (obsolete) — — Splinter Review 

    
    

    
  


  

        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?

    
  

        Attachment #336253 -
        Flags: review?(roc) → review?(dholbert)

    
    

    
  
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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        revised patch 
        — 
          — Splinter Review
      

    


  

        Attachment #338431 -
        Flags: superreview?(roc)

    
  

        Attachment #336253 -
        Flags: superreview?(roc)

        Attachment #338431 -
        Flags: superreview?(roc) → superreview+

    
    

    
  
taking the bug as I fixed it yesterday, 

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

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/63bc896857c9
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: