table collapsing border code needs rework

VERIFIED FIXED in mozilla0.9.9

Status

()

defect
P1
normal
VERIFIED FIXED
19 years ago
9 months ago

People

(Reporter: karnaze, Assigned: karnaze)

Tracking

({css2, highrisk, testcase})

Trunk
mozilla0.9.9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [awd:tbl])

Attachments

(8 attachments, 1 obsolete attachment)

Assignee

Description

19 years ago
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.
Assignee

Comment 1

19 years ago
*** Bug 2130 has been marked as a duplicate of this bug. ***
Assignee

Comment 2

19 years ago
*** Bug 3000 has been marked as a duplicate of this bug. ***
Assignee

Comment 3

19 years ago
*** Bug 12462 has been marked as a duplicate of this bug. ***
Assignee

Comment 4

19 years ago
*** Bug 15248 has been marked as a duplicate of this bug. ***
Assignee

Comment 5

19 years ago
*** Bug 22897 has been marked as a duplicate of this bug. ***
Assignee

Comment 6

19 years ago
*** Bug 26614 has been marked as a duplicate of this bug. ***
Assignee

Comment 7

19 years ago
*** Bug 31256 has been marked as a duplicate of this bug. ***
Assignee

Comment 8

19 years ago
*** Bug 32793 has been marked as a duplicate of this bug. ***
Assignee

Comment 9

19 years ago
*** Bug 21076 has been marked as a duplicate of this bug. ***
Assignee

Comment 10

19 years ago
*** Bug 24113 has been marked as a duplicate of this bug. ***
Assignee

Comment 11

19 years ago
Adding nsbeta2 keyword.
Keywords: nsbeta2
Assignee

Updated

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

Updated

19 years ago
Blocks: 9191
Assignee

Updated

19 years ago
Blocks: 2130
Assignee

Updated

19 years ago
Blocks: 3000
Assignee

Updated

19 years ago
Blocks: 12462
Assignee

Updated

19 years ago
Blocks: 15248
Assignee

Updated

19 years ago
Blocks: 21076
Assignee

Updated

19 years ago
Blocks: 22897
Assignee

Updated

19 years ago
Blocks: table-rules
Assignee

Updated

19 years ago
Blocks: 26614
Assignee

Updated

19 years ago
Blocks: 31256
Assignee

Updated

19 years ago
Blocks: 32793

Comment 13

19 years ago
[nsbeta2+] [6/22]
Whiteboard: [nsbeta2+] [6/22]
Assignee

Updated

19 years ago
Blocks: 2068
Assignee

Updated

19 years ago
Blocks: 2436
Assignee

Updated

19 years ago
Blocks: 32190
Assignee

Updated

19 years ago
Blocks: 33175

Updated

19 years ago
Blocks: 43178

Comment 14

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

Whiteboard: [nsbeta2+] [6/22] → [nsbeta2-]
Assignee

Updated

19 years ago
Blocks: 43039
Assignee

Updated

19 years ago
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)?

Comment 16

19 years ago
So how is the rewrite of the border code going?

Comment 17

19 years ago
nominating for nsbeta3. karnaze has most of this done in his tree, according to 
attinasi.
Keywords: nsbeta3

Comment 18

19 years ago
Approving for beta3 - this needs to be fixed or support for collapsed borders 
needs to be removed entirely.
Whiteboard: [nsbeta2-] → [nsbeta2-] [nsbeta3+]

Updated

19 years ago
QA Contact: desale → chrisd

Updated

19 years ago
Priority: P3 → P1

Comment 19

19 years ago
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 21

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

Comment 22

19 years ago
Adding to the spam-list...

Comment 23

19 years ago
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-]

Comment 24

19 years ago
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
Assignee

Updated

19 years ago
Blocks: 44148

Comment 25

19 years ago
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.
Assignee

Updated

19 years ago
No longer blocks: 33175

Comment 26

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

Comment 27

19 years ago
Suggest: platform=ALL, OS=ALL

Comment 28

19 years ago
per Bug 12462 copying All/All.
OS: Windows NT → All
Hardware: PC → All

Comment 29

19 years ago
Adding "highrisk" keyword, trying to get this checked in soon. :-)
Keywords: highrisk
adding keyword nominations
Keywords: nsbeta2, nsbeta3nsbeta1
Assignee

Comment 31

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

Updated

19 years ago
Blocks: 65096

Comment 33

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

19 years ago
*** Bug 67176 has been marked as a duplicate of this bug. ***
QA contact update
QA Contact: chrisd → amar

Comment 36

18 years ago
cc self. sorry for the SPAM.

Comment 37

18 years ago
I would agree with James Green.  I think this should definately be fixed for
mozilla0.9.
Blocks: 39411

Comment 38

18 years ago
cc self
Assignee

Updated

18 years ago
Blocks: 48538
Assignee

Updated

18 years ago
Blocks: 59037

Comment 39

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

18 years ago
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
Assignee

Comment 41

18 years ago
Changing the milestone to m0.9.7, which I guess is one before 1.0.
Target Milestone: mozilla1.0 → mozilla0.9.7
Assignee

Comment 42

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

Updated

18 years ago
Blocks: 104166
Assignee

Updated

18 years ago
Blocks: 103889

Updated

18 years ago
Whiteboard: [nsbeta2-] [nsbeta3-] → [awd:tbl]

Comment 43

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

Comment 44

18 years ago
collapsing border bugs moved to m098
Target Milestone: mozilla0.9.7 → mozilla0.9.8

Comment 45

18 years ago
Posted file testcase list
Assignee

Comment 47

18 years ago
->m099
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Comment 48

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

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

18 years ago
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
Assignee

Comment 53

18 years ago
Assignee

Comment 54

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

Comment 55

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

18 years ago
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 57

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

r= alexsavulov
(nice huge work chris!)
Attachment #69739 - Flags: review+

Comment 58

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

Comment 59

18 years ago
Posted file new test

Updated

18 years ago
Keywords: approval

Comment 60

18 years ago
Posted file test-2

Comment 61

18 years ago
Posted file testcase-3
Assignee

Comment 62

18 years ago
Posted patch revised patchSplinter Review
encorporated most of the suggestions, ran through all of the test cases (mostly
positive), passed regression tests.
Assignee

Updated

18 years ago
Attachment #69739 - Attachment is obsolete: true

Comment 63

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

Comment 64

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

Comment 65

18 years ago
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
Last Resolved: 18 years ago
Resolution: --- → FIXED
*** Bug 126581 has been marked as a duplicate of this bug. ***

Comment 67

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