Closed Bug 41262 Opened 25 years ago Closed 23 years ago

table collapsing border code needs rework

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: karnaze, Assigned: karnaze)

References

Details

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

Attachments

(8 files, 1 obsolete file)

The collapsing border code is in need of a rewrite because fixing the existing bugs is nearly impossible, and it adds too much working set overhead. Collapsing borders are important not only because of their new functionality, but because they are implied when the "rules" attribute is present. I have already made some progress on the rewrite.
*** Bug 2130 has been marked as a duplicate of this bug. ***
*** Bug 3000 has been marked as a duplicate of this bug. ***
*** Bug 12462 has been marked as a duplicate of this bug. ***
*** Bug 15248 has been marked as a duplicate of this bug. ***
*** Bug 22897 has been marked as a duplicate of this bug. ***
*** Bug 26614 has been marked as a duplicate of this bug. ***
*** Bug 31256 has been marked as a duplicate of this bug. ***
*** Bug 32793 has been marked as a duplicate of this bug. ***
*** Bug 21076 has been marked as a duplicate of this bug. ***
*** Bug 24113 has been marked as a duplicate of this bug. ***
Adding nsbeta2 keyword.
Keywords: nsbeta2
Status: NEW → ASSIGNED
Keywords: css2
NOTE: When verifying this bug (as always) be sure to verify all duplicates. (Many of them are unrelated.) I'll also try to do some thorough testing once this is done.
Blocks: 9191
Blocks: 2130
Blocks: 3000
Blocks: 12462
Blocks: 15248
Blocks: 21076
Blocks: 22897
Blocks: table-rules
Blocks: 26614
Blocks: 31256
Blocks: 32793
[nsbeta2+] [6/22]
Whiteboard: [nsbeta2+] [6/22]
Blocks: 2068
Blocks: 2436
Blocks: 32190
Blocks: 33175
Blocks: 43178
Cleaning up status whiteboard and marking beta2 minus (6/22 has passed)
Whiteboard: [nsbeta2+] [6/22] → [nsbeta2-]
Blocks: 43039
Keywords: donttest
What would be the (a) work required and (b) impact of each of the three options of (1) leaving things as is, or (2) desupporting collpsing borders, or (3) doing the work to fix it right, for nsbeta3 (which equates to FCS)?
So how is the rewrite of the border code going?
nominating for nsbeta3. karnaze has most of this done in his tree, according to attinasi.
Keywords: nsbeta3
Approving for beta3 - this needs to be fixed or support for collapsed borders needs to be removed entirely.
Whiteboard: [nsbeta2-] → [nsbeta2-] [nsbeta3+]
QA Contact: desale → chrisd
Priority: P3 → P1
The following attachment is a W3C example that we don't do right. There's even an extra column??? http://www.w3.org/TR/REC-CSS2/tables.html#propdef-border-style
Attached file Testcase that shows collapse-border (deleted) —
*** Bug 47053 has been marked as a duplicate of this bug. ***
Adding to the spam-list...
Due to the number of other more serious problems in tables this bug is being demoted from beta3+ to beta3-, meaning it will not get into beta3 or RTM. Although Chris has much of it fixed in his local tree, there is still a large completion, checkin, testing and verification cost, and there are bound to be some bugs in the implementation that will also take time from fixing other table problems. If there are known cases where we need this to support something in the UI, please add that information to this bug for further triage. Bug 9191 is the only blocked bug that is beta3+, and it can easily be demoted as well since it simply reports that we have the wrong default for the CSS2 property border-collapse. The work that does need to be done is to change the code to ignore the value of 'collapse' on the border-collapse property, and I'll open another bug on that for completion in beta3.
Whiteboard: [nsbeta2-] [nsbeta3+] → [nsbeta2-] [nsbeta3-]
I'm adding the "mostfreq" keyword. We have 13 duplicates, it's obviously going to get 15 dupes sooner (or later). It's getting annoying that people DO have fixes to these highly visable bugs, but aren't being allowed to check them in due to "risk". Take a look at almost any milestone release, and then take a look at the nightly builds released 2 days after the tree branched for those milestones. (When people were allowed to check in their bug fixes to the previous milestones). In almost EVERY case, the nightly build was MUCH better, because lots of the annoying bugs in the milestones were FIXED. In the unlikely case where this would cause a lot of regressions, we could back the changes out! PLEASE reconsider.
Keywords: mostfreq
Blocks: 44148
Just some thoughts: Attinasi at bug 29055: also Chris K. is busy with much more important table issues (like collapsed borders). What has happened now is that bug 915 and 29055 are nearly futured in order to give time to solve the border collapsing code. Not fixing this bug will also cause the highly visible bug 2068 not to be solved, which is directly from the html4 specification and is properly displayed by IE5. I agree about the high costs to fix this bug. On the other side which are more important bugs that needs to be fixed in first order? There are only 3 bugs with nsbeta3+ for karnaze. One is to turnoff the collapsed border code. One is some strange Mac-Suff and the other one is marked with fix in hand. In order to test the new rules and frames code it should be checked in as soon as possible. At least I would be pleased to help to test the stuff.
No longer blocks: 33175
Adding mozilla0.90 keyword to this bug. IMO, things that have been fixed but not allowed to land because of "risk" should be landed immediately after RTM.
Keywords: mozilla0.9
Suggest: platform=ALL, OS=ALL
per Bug 12462 copying All/All.
OS: Windows NT → All
Hardware: PC → All
Adding "highrisk" keyword, trying to get this checked in soon. :-)
Keywords: highrisk
adding keyword nominations
Keywords: nsbeta2, nsbeta3nsbeta1
Changing mozilla0.9 to mozilla1.0. There is still work to be done. Sorry for the spam.
Keywords: mozilla0.9mozilla1.0
Target Milestone: --- → mozilla1.0
Please don't change other people's nominations. I'm putting back the mozilla0.9 keyword, because it indicates that somebody thinks this bug should be fixed for mozilla0.9. The Target Milestone (which karnaze just set to mozilla 1.0) indicates when the assignee intends to fix the bug.
Keywords: mozilla0.9
Blocks: 65096
Considering this is a high risk bug that clearly blocks a lot of other bugs, is it wise to leave such a landing until 1.0? Surely there is plenty of opportunity for more bugs to be introduced by this landing and thus warrants plenty of testing time, indicating a mozilla0.9 target is far safer? cc self for the response.
*** Bug 67176 has been marked as a duplicate of this bug. ***
QA contact update
QA Contact: chrisd → amar
cc self. sorry for the SPAM.
I would agree with James Green. I think this should definately be fixed for mozilla0.9.
Blocks: 39411
cc self
Blocks: 48538
Blocks: 59037
I hope this issue has not been forgotten. Are we still planning to release the rewritten collapsing border code before Mozilla 1.0, and if so, how long before Mozilla 1.0?
if there is even a partial fix for this, can a patch be created and attached here? So that those who build moz can test and maybe fix it.
Keywords: donttest
Changing the milestone to m0.9.7, which I guess is one before 1.0.
Target Milestone: mozilla1.0 → mozilla0.9.7
*** Bug 31256 has been marked as a duplicate of this bug. ***
Blocks: 104166
Blocks: 103889
Whiteboard: [nsbeta2-] [nsbeta3-] → [awd:tbl]
m0.9.7 freezes in three days. Is this going to landed immediately after .97 branches? Or will it wait until post-1.0?
collapsing border bugs moved to m098
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Attached file testcase list (deleted) —
->m099
Target Milestone: mozilla0.9.8 → mozilla0.9.9
This has shown up on the branch landing page with a comment "ETA 12/19/2002." Is that a typo or a poor joke? What's the plan with this bug?
Yet another test case. No browser to render this corretly. In fact Opera 6 seems to be the only one to even support almost all the required features.
Yet another test case. No browser to render this corretly. In fact Opera 6 seems to be the only one to even support almost all the required features.
Marking nsbeta1+
Keywords: nsbeta1+
Adding testcase keyword
Keywords: testcase
Attached patch patch containing new border collapse code (obsolete) (deleted) — Splinter Review
The changes to the regression tests in the patch file are incorrect.
Comment on attachment 69739 [details] [diff] [review] patch containing new border collapse code Please improve the comment surrounding the use fo the bit-OR'ing for NS_STYLE_BORDER_STYLE_RULES_MASK (use of a bit is confusing for that field) in nsHTMLReflowState.cpp, IS_TABLE_CELL is defined twice DrawBorderSegment in nsCSSRendering shoudl probably be renamed to DrawTableBorderSegment since it is table specific [more...]
Comment on attachment 69739 [details] [diff] [review] patch containing new border collapse code ABORT0(); and ABORT1 may be replacements for NS_ENSURE_XXX - maybe use those? There is a lot of interesting new code in the TableCellMap, but no comments about the general algorithms used, the expected arguments, results etc. It would be nice... Is nsBCTableCellFrame::SizeOf implemented to account for the data members? I cannot tell from the diffs, looks like no. Te rest looks fine, from my brief inspection. You promised me you would provide written documentation, to be checked into layout\doc, and I'd like to see some mroe internal (code) documentation for the beefier new routines. Please also document in the bug what you have tested - I'm assuming you have tested a lot as you usually do, and hopefully some DHTML cases were included.
Attachment #69739 - Flags: superreview+
Comment on attachment 69739 [details] [diff] [review] patch containing new border collapse code r= alexsavulov (nice huge work chris!)
Attachment #69739 - Flags: review+
nice to see this patch, Chris probably you don't need to change rtest.bat and the filelist.txt.
Attached file new test (deleted) —
Keywords: approval
Attached file test-2 (deleted) —
Attached file testcase-3 (deleted) —
Attached patch revised patch (deleted) — Splinter Review
encorporated most of the suggestions, ran through all of the test cases (mostly positive), passed regression tests.
Attachment #69739 - Attachment is obsolete: true
my build of this failed [fbsd]. i'm too tired to try to find out why. sorry.
The new nsTableCellMap::GetRightMostBorder and nsTableCellMap::GetBottomMostBorder in nsCellMap.cpp produce compiler warnings: layout/html/table/src/nsCellMap.cpp:136 `class BCData * bcData' might be used uninitialized in this function layout/html/table/src/nsCellMap.cpp:157 `class BCData * bcData' might be used uninitialized in this function Although this looks like a case of compiler being not smart enough, it would still be nice not to have those warnings (since often they do indicate real problems and it easier to notice those problem if there are fewer warnings)...
The patch is in and it fixes nearly all of the test cases and related bugs. I filed bug 126540 to handle the dashed border problem in http://www.netzwelt.com/selfhtml/css/eigenschaften/anzeige/border_collapse.htm and bug 126543 to handle not being able to dynamically change to/from collapsed borders and the rules attribute (as illustrated by attachments 6, 7, 8). If there are any other problems with collapsed borders, please file separate bugs.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
*** Bug 126581 has been marked as a duplicate of this bug. ***
Can this have caused bug 126742 ?
marking verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: