Closed Bug 77019 Opened 23 years ago Closed 20 years ago

<tr style="visibility: collapse;"> looks like hidden and collapsed

Categories

(Core :: Layout: Tables, defect, P3)

Product:
Core
Component:
Layout: Tables
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: martin.honnen, Assigned: bernd_mozilla)

References

Details

(Keywords: css2, regression, testcase, Whiteboard: [awd:tbl])

Attachments

(5 files, 2 obsolete files)

test case(bug demo): one row is shown empty but should be collapsed instead
846 bytes, text/html
Details
simple testcase.
590 bytes, text/html
Details
revised patch
12.00 KB, patch
bzbarsky
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
Yet another version... Very similar to the one described before. It works fine but the table keeps growing. If you look at the code after it as growned, the code looks just fine!
1.09 KB, text/html
Details
Interactive demo page
4.85 KB, text/html
Details 
CSS2 allows for
  visibility: collapse;
on tr elements, however M0.8.1 on Windows 95 doesn't collapse the row but simply 
 hides its content.

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
                        "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<title>
row collapsing
</title>
</head>
<body>
<input type="button" value="collapse/expand"
       onclick="if (document.getElementById('aTable').rows[3].style.visibility 
== 'collapse')
                  document.getElementById('aTable').rows[3].style.visibility = 
'';
                else
                  document.getElementById('aTable').rows[3].style.visibility = 
'collapse';"
>
<table id="aTable" border="1">
<script>
for (var i = 0; i < 10; i++)
  if (i == 3)
    document.write('<tr style="visibility: collapse;"><td>' + i + 
'<\/td><td>Kibology<\/td><\/tr>');
  else
    document.write('<tr><td>' + i + '<\/td><td>Kibology<\/td><\/tr>');
</script>
</table>
</body>
</html>
Reassigned to HTMLTables.
Assignee: pierre → karnaze
Component: Style System → HTMLTables
QA Contact: ian → amar
Hey this is a big time problem.  imho must-fix for 1.0.  Another testcase for
this bug is http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16059.
Keywords: css2, mozilla1.0
OS: Windows 95 → All
Hardware: PC → All
I was going to mention that this used to work, so it should be considered a
regression.  Some people have speculated that paint or reflow suppression may
have broken this.
Keywords: regression
Keywords: testcase
*** Bug 101790 has been marked as a duplicate of this bug. ***
Whiteboard: [awd:tbl]
Temporarily moving to future until a milestone can be assigned.
Status: NEW → ASSIGNED
Target Milestone: --- → Future
This issue is similar to bug 76497 (which is about collapsing a table's column
rather than row).  I have found some cases where both rows and columns do and do
not behave correctly in Mozilla 1.0.0+ nightly builds (I am using build
2002051008 for OS/2).

Note the test in the Debug menu, Viewer Demos sub-menu, #4 Simple Tables.  The
first two examples of row and column collapsing can be extracted (copy and paste
from the page source to a new page) and they display collapsed rows and
collapsed columns as expected (see http://syntheticdimension.net/test4.html
which has been validated as HTML 4.01 by the w3c).

But then if we remove the third and fourth tables from that (simply deleting
those tables from the HTML code and leaving absolutely everything else in the
file intact) the first example of collapsed rows and collapsed columns fails
(see http://syntheticdimension.net/test4a.html which has also been validated as
HTML 4.01 by the w3c).

Both pages are valid HTML and use exactly the same code for the first and second
tables.  The only difference is that the second test page above has had the
third and fourth tables deleted.  This simple act causes table 2 to display
incorrectly in test4a.html where it worked correctly in test4.html.  Same code
but different display even in the same browser version.  And this was the code
taken from the Mozilla debug tests which are included with the browser binaries.
Attached file simple testcase. — 
I just ran accross this bug today, here's my testcase :)
see http://www.w3.org/TR/REC-CSS2/tables.html#dynamic-effects for a description
of what should happen.

This is an incremental reflow bug, the wrong code is at
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableFrame.cpp#2209

2209   if (eReflowReason_Resize == aReflowState.reason) {
2210     if (!DidResizeReflow()) {
2211       // XXX we need to do this in other cases as well, but it needs to be
made more incremental
2212       aDoCollapse = PR_TRUE;
2213       SetResizeReflow(PR_TRUE);
2214     }
2215   }

if eReflowReason_Incremental == aReflowState.reason aDoCollapse is not set and
as a consequence the code is not executed.
Attached patch patch (obsolete) — — Splinter Review 
Oh Noooooo, we had the collapse code working in the tree for years and it was
simply optimized away. Hmm may be optimized is not the correct word for it.
Attachment #106678 - Flags: review?(karnaze)
Attached patch patch (obsolete) — — Splinter Review 
Chris, I cant imagine how this can be further optimized without having reflow
bugs. This may cause a performance penalty. I hope the penalty will be balanced
with the patch in bug 85755.
Attachment #106678 - Attachment is obsolete: true
Attachment #106678 - Flags: review?(karnaze)
Attachment #111336 - Flags: review?(karnaze)
I would consider two options (1) avoid calling AdjustForCollapsingRows/Cols if
there is nothing to collapse or (2) make your patch more efficient. 

(1) may involve setting a bit on nsTableFrame indicating that there are
rows/cols to collapse and then making sure the bit is kept accurate. This could
be a bit of a pain.

(2) make AdjustForCollapsingRows and CollapseRowGroupIfNecessary more efficient.
They process unaffected rows/cols unnecessarily. For example, 

      } else { // row is not collapsed but needs to be adjusted by those that are
        rowRect.y -= aYGroupOffset;
        rowFrame->SetRect(aPresContext, rowRect);
      }

could be changed to 

      } else if (aYGroupOffset != 0) { // row is not collapsed but needs to be
adjusted by those that are
        rowRect.y -= aYGroupOffset;
        rowFrame->SetRect(aPresContext, rowRect);
      }

(2) may be sufficient if this and similar optimizations are made.

The following should also be dealt with at some point and (1) might be a lot
faster if there are a lot of rows in the table.

    // XXX Is this needed?
#if 0
    AdjustForCollapsingRows(aPresContext, aDesiredSize.height);
    AdjustForCollapsingCols(aPresContext, aDesiredSize.width);
#endif


mass reassign to default owner
Assignee: karnaze → table
Status: ASSIGNED → NEW
QA Contact: amar → madhur
Target Milestone: Future → ---
Priority: -- → P3
Target Milestone: --- → Future
Comment on attachment 111336 [details] [diff] [review]
patch

I will follow path 1) as it promises more pain. I will not further optimize the
code. My idea is:
add a special bit to the mbits indicating that we have collapsed frames.
add in every frame that can be collapsed a style query for the visibility if
the frame is collapsed set the bit.
If the bit is set  in the table frame compute the collapsing for rows/cols but
reset the bit before. If during the computation a frame still needs the
collapsing set the bit again.
Attachment #111336 - Flags: review?(karnaze)
*** Bug 205459 has been marked as a duplicate of this bug. ***
Attached patch revised patch — — Splinter Review 

        Attachment #111336 -
        Attachment is obsolete: true

    
    

    
  


  
Using style="display:none|block" the table keeps growing.
Unlike other browser( works well in IE) the table grows when you hide rows.

    
  

        Attachment #133930 -
        Attachment description: Toggle rows  with javascript → Yet another version...
Very similar to the one described before.
It works fine but the table keeps growing. If you look at the code after it as growned, the code looks just fine!

    
  

        Attachment #125017 -
        Flags: review?(bz-vacation)

    
    

    
  
The growing thing is a different bug, and the correct display value for a table
row is "table-row" if you want it to render like a table row.

    
    

    
  
Comment on attachment 125017 [details] [diff] [review]
revised patch

Seems reasonable.  r=bzbarsky.

For future reference, using 'diff -pu8' would have made this patch _much_
easier to review...

        Attachment #125017 -
        Flags: review?(bz-vacation) → review+
Blocks: 149101

    
  

        Attachment #125017 -
        Flags: superreview?(dbaron)

    
  
Assignee: table → bernd_mozilla

    
  
Blocks: 206516
No longer blocks: 206516

    
  
Blocks: 76497

    
    

    
  
Review exists. Why not commit this patch and let us to test it ???

    
    

    
  
Comment on attachment 125017 [details] [diff] [review]
revised patch

sr=dbaron, although drop the weird whitespace change in
nsTableColGroupFrame.cpp.  Sorry for the delay.

        Attachment #125017 -
        Flags: superreview?(dbaron) → superreview+

    
    

    
  
fix checked in
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
    

    
  

      
      
      
      

        Attached file
          
        Interactive demo page
        — 
      

    


  
While using Mozilla 1.8a build 2004042909, I still see problems in this demo
page and I think this bugfile should be reopened.

    
    

    
  
Sorry, I forgot to explicit the problem I was seeing.
In the interactive demo case, click the collapse button and then the visible
button. The row reappears but is hidden: the text does is not visible even
though the row structure is present.

    
    

    
  
please open a new bug on this

    
    

    
  
I filed bug 242253 on the paint issue when rows become visible again.

    
    

    
  
Sorry Bernd, I wanted to open a bugfile on this but just did not have the time to.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: