The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Future

Status

()

Core
Layout: Tables
P3
normal
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: Martin Honnen, Assigned: Bernd)

Tracking

({css2, regression, testcase})

Trunk
Future
css2, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [awd:tbl])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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>
(Reporter)

Comment 1

16 years ago
Created attachment 31722 [details]
test case(bug demo): one row is shown empty but should be collapsed instead

Comment 2

16 years ago
Reassigned to HTMLTables.
Assignee: pierre → karnaze
Component: Style System → HTMLTables
QA Contact: ian → amar

Comment 3

16 years ago
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

Comment 4

16 years ago
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. ***

Updated

16 years ago
Whiteboard: [awd:tbl]

Comment 6

15 years ago
Temporarily moving to future until a milestone can be assigned. 
Status: NEW → ASSIGNED
Target Milestone: --- → Future

Comment 7

15 years ago
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.

Comment 8

15 years ago
Created attachment 86233 [details]
simple testcase.

I just ran accross this bug today, here's my testcase :)

Comment 9

15 years ago
see http://www.w3.org/TR/REC-CSS2/tables.html#dynamic-effects for a description
of what should happen.
(Assignee)

Comment 10

15 years ago
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.
(Assignee)

Comment 11

15 years ago
Created attachment 106678 [details] [diff] [review]
patch

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.

Updated

15 years ago
Attachment #106678 - Flags: review?(karnaze)
(Assignee)

Comment 12

14 years ago
Created attachment 111336 [details] [diff] [review]
patch

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
(Assignee)

Updated

14 years ago
Attachment #106678 - Flags: review?(karnaze)
(Assignee)

Updated

14 years ago
Attachment #111336 - Flags: review?(karnaze)

Comment 13

14 years ago
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

Comment 14

14 years ago
mass reassign to default owner
Assignee: karnaze → table
Status: ASSIGNED → NEW
QA Contact: amar → madhur
Target Milestone: Future → ---

Updated

14 years ago
Priority: -- → P3
Target Milestone: --- → Future
(Assignee)

Comment 15

14 years ago
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)
(Assignee)

Comment 16

14 years ago
*** Bug 205459 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 17

14 years ago
Created attachment 125017 [details] [diff] [review]
revised patch
Attachment #111336 - Attachment is obsolete: true

Comment 18

14 years ago
Created attachment 133930 [details]
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!

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

Updated

14 years ago
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!
(Assignee)

Updated

14 years ago
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
(Assignee)

Updated

13 years ago
Attachment #125017 - Flags: superreview?(dbaron)
(Assignee)

Updated

13 years ago
Assignee: table → bernd_mozilla
(Assignee)

Updated

13 years ago
Blocks: 206516
No longer blocks: 206516
(Assignee)

Updated

13 years ago
Blocks: 76497

Comment 21

13 years ago
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+
(Assignee)

Comment 23

13 years ago
fix checked in
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 24

13 years ago
Created attachment 147353 [details]
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.

Comment 25

13 years ago
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.
(Assignee)

Comment 26

13 years ago
please open a new bug on this
(Assignee)

Comment 27

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

Comment 28

13 years ago
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.