Closed Bug 41262 Opened 21 years ago Closed 19 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
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
*** 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
->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
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
Keywords: approval
Attached file test-2
Attached file testcase-3
Attached patch revised patchSplinter 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: 19 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.