Closed Bug 77019 Opened 24 years ago Closed 21 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: 21 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: