41262 - table collapsing border code needs rework

Status: VERIFIED FIXED

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

Description

[karnaze (gone)]

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.

Comment 1

[karnaze (gone)]

*** Bug 2130 has been marked as a duplicate of this bug. ***

Comment 2

[karnaze (gone)]

*** Bug 3000 has been marked as a duplicate of this bug. ***

Comment 3

[karnaze (gone)]

*** Bug 12462 has been marked as a duplicate of this bug. ***

Comment 4

[karnaze (gone)]

*** Bug 15248 has been marked as a duplicate of this bug. ***

Comment 5

[karnaze (gone)]

*** Bug 22897 has been marked as a duplicate of this bug. ***

Comment 6

[karnaze (gone)]

*** Bug 26614 has been marked as a duplicate of this bug. ***

Comment 7

[karnaze (gone)]

*** Bug 31256 has been marked as a duplicate of this bug. ***

Comment 8

[karnaze (gone)]

*** Bug 32793 has been marked as a duplicate of this bug. ***

Comment 9

[karnaze (gone)]

*** Bug 21076 has been marked as a duplicate of this bug. ***

Comment 10

[karnaze (gone)]

*** Bug 24113 has been marked as a duplicate of this bug. ***

Comment 11

[karnaze (gone)]

Adding nsbeta2 keyword.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 13

[Peter Trudelle]

[nsbeta2+] [6/22]

Comment 14

[Jim Roskind]

Cleaning up status whiteboard and marking beta2 minus (6/22 has passed)

Comment 15

[ekrock's old account (dead)]

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)?

Comment 16

[Jason Eager]

So how is the rewrite of the border code going?

Comment 17

[Chris Waterson]

nominating for nsbeta3. karnaze has most of this done in his tree, according to 
attinasi.

Comment 18

[Marc Attinasi]

Approving for beta3 - this needs to be fixed or support for collapsed borders 
needs to be removed entirely.

Comment 19

[Peter Lubczynski]

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

Comment 20

[Peter Lubczynski]

Attachment: Testcase that shows collapse-border

Comment 21

[Marc Attinasi]

*** Bug 47053 has been marked as a duplicate of this bug. ***

Comment 22

[Marc Attinasi]

Adding to the spam-list...

Comment 23

[Marc Attinasi]

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.

Comment 24

[Jason Eager]

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.

Comment 25

[Bernd Mielke]

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.

Comment 26

[Jason Eager]

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.

Comment 27

[Tobias Burnus]

Suggest: platform=ALL, OS=ALL

Comment 28

[timeless]

per Bug 12462 copying All/All.

Comment 29

[Jason Eager]

Adding "highrisk" keyword, trying to get this checked in soon. :-)

Comment 30

[Asa Dotzler [:asa]]

adding keyword nominations

Comment 31

[karnaze (gone)]

Changing mozilla0.9 to mozilla1.0. There is still work to be done. Sorry for the
spam.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 33

[James Green]

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.

Comment 34

[Bernd]

*** Bug 67176 has been marked as a duplicate of this bug. ***

Comment 35

[Amarendra Hanumanula]

QA contact update

Comment 36

[James Lariviere]

cc self. sorry for the SPAM.

Comment 37

[Trevor Andreas]

I would agree with James Green.  I think this should definately be fixed for
mozilla0.9.

Comment 38

[Brett Donald]

cc self

Comment 39

[Brett Donald]

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?

Comment 40

[basic]

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.

Comment 41

[karnaze (gone)]

Changing the milestone to m0.9.7, which I guess is one before 1.0.

Comment 42

[karnaze (gone)]

*** Bug 31256 has been marked as a duplicate of this bug. ***

Comment 43

[Nick Lewycky]

m0.9.7 freezes in three days. Is this going to landed immediately after .97
branches? Or will it wait until post-1.0?

Comment 44

[karnaze (gone)]

collapsing border bugs moved to m098

Comment 45

[Bernd]

Attachment: testcase list

Comment 46

[Ivan Dolezal]

The webpage
http://homen.vsb.cz/~dol72/bordercollapse.html
produces this:
http://homen.vsb.cz/~dol72/bordercollapse-msie.jpg
in MSIE and this:
http://homen.vsb.cz/~dol72/bordercollapse-mozilla.jpg
in Mozilla.
Who is actually right?

Comment 47

[karnaze (gone)]

->m099

Comment 48

[Nick Lewycky]

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?

Comment 49

[Mikko Rantalainen]

Attachment: A little harder test case for collapsing border

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.

Comment 50

[Mikko Rantalainen]

Attachment: A little harder test case for collapsing border

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.

Comment 51

[Kevin McCluskey (gone)]

Marking nsbeta1+

Comment 52

[Amarendra Hanumanula]

 Adding testcase keyword

Comment 53

[karnaze (gone)]

Attachment: patch containing new border collapse code

Comment 54

[karnaze (gone)]

The changes to the regression tests in the patch file are incorrect.

Comment 55

[Marc Attinasi]

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 56

[Marc Attinasi]

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.

Comment 57

[Alexandru Savulov]

Comment on attachment 69739 [details] [diff] [review]
patch containing new border collapse code

r= alexsavulov
(nice huge work chris!)

Comment 58

[Bernd]

nice to see this patch, Chris probably you don't need to change rtest.bat and
the filelist.txt.

Comment 59

[Bernd]

Attachment: new test

Comment 60

[Bernd]

Attachment: test-2

Comment 61

[Bernd]

Attachment: testcase-3

Comment 62

[karnaze (gone)]

Attachment: revised patch

encorporated most of the suggestions, ran through all of the test cases (mostly
positive), passed regression tests.

Comment 63

[timeless]

my build of this failed [fbsd]. i'm too tired to try to find out why. sorry.

Comment 64

[Aleksey Nogin]

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)...

Comment 65

[karnaze (gone)]

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.

Comment 66

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 126581 has been marked as a duplicate of this bug. ***

Comment 67

[R.K.Aa.]

Can this have caused bug 126742 ?

Comment 68

[Amarendra Hanumanula]

marking verified