49490 - value of 'collapse' needs to be ignored for border-collapse: style property

Status: VERIFIED FIXED

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

Description

[Marc Attinasi]

We need to ignore 'collapse' for border-collapse on tables since the collapsed 
borders are rendered incorrectly and we cannot fix them in time for beta3. Once 
we get time to fix the collapsing border model we can pay attention to the 
collapse value again.

Comment 1

[Marc Attinasi]

This is kinda painful, but necessary for beta3.

Comment 2

[Jason Eager]

And did we actually do this?

Comment 3

[karnaze (gone)]

It will probably be done when there is no chance that the new version of 
collapsing borders will not make it in. So, it hasn't been disabled yet.

Comment 4

[Jason Eager]

*Sigh at extreme communication difficulties*

YES, the new border collapsing code won't make it in, as Marc said:
------- Additional Comments From Marc Attinasi 2000-08-18 12:53 -------

This is kinda painful, but necessary for beta3.


Things just keep slipping through our fingers, don't they.

Comment 5

[Marc Attinasi]

I think that this should be ignored by the css parser, that way if the DOM
accesses the style value it cannot get a value of 'collapse' when in fact we are
treating it like 'separate'. At the same time, we need to:
1] make the CSSParser ignore the border-collapse property
2] ensure that the default for border-collapse in the stylecontext is 'separate'
3] remove the border-collapse: separate from html.css
4] ASSERT in table code that the value for border-collapse is always separate

1 is trivial, 2 is already done, 3 is trivial, and 4 is trivial and optional

We could just change the table code to always use the separate border model
regardless of the value of the style property, but then the DOM and the display
will be out of synch. I think it is better to act like we just don't recognize
the property at all.

Chris, if this is indeed the path we want/need to go, send me this bug and I'll
take care of the style system changes. Also, if this is beta3+ then we need to
do it right away, I think.

Comment 6

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

You need to make sure the property name can't be changed through the CSSOM.

Comment 7

[Phil Peterson]

Not holding PR3 for this. Marking nsbeta3-. Please nominate for rtm if we really
need to fix this before shipping seamonkey.

Comment 8

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

BTW, an alternative solution to this might be to change the part of the table
code where the border model is selected based on style information, which I
suspect might be a very local change.  It would avoid CSSOM complications.

Comment 9

[Marc Attinasi]

If we only change the table code then the CSSOM can be out of sync. For example,
if there is a stylesheet that has border-collapes:collapsed the CSSOM will
return that value, but the table will use separate borders. If we are willing to
live with this then, yes, it is a more localized and less risky change.

Comment 10

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

I think that's how we behave for all CSS props we don't support, e.g.,
text-shadow.

Comment 11

[Hixie (not reading bugmail)]

Since border-collapse only takes two values, then there is no forwards-
compatability issue as there is with unsupported values on 'display'. Thus
I think David is right, we don't really need to worry about either the
parser nor the DOM, just block out the collapsing border code in table layout.

Comment 12

[Marc Attinasi]

I understand about the lack of forward-compatibility issues, but are you saying
it is OK if the CSSOM reports that the border-collapse is 'collapsed' when
really it is 'separate'? Granted, it doesn't sound like the end of the world,
but clearly it is wrong. That said, it is obviously easier to just hard-wire the
table code to always use the separate border model regardless of the style
context value.

Comment 13

[karnaze (gone)]

The nasty part of turning off collapsing borders will be to deal with turning 
off the rules attribute, since rules imply the collapsing border model.

Comment 14

[Hixie (not reading bugmail)]

Marc: This is a non issue, as as David pointed out, we already do it for 
attributes/properties that we don't support such as textShadow/text-shadow.

Comment 15

[karnaze (gone)]

Adding [rtm need info] and marking this P1 since fixing it fixes bug 54124.

Comment 16

[karnaze (gone)]

Attachment: patch to fix the bug

Comment 17

[karnaze (gone)]

changing to rtm+

Comment 18

[karnaze (gone)]

rtm++ per discussion with PDT.

Comment 19

[karnaze (gone)]

Fixed

Comment 20

[Christine Hoffman]

Using David Baron's testcase for collapsing table borders:

http://www.people.fas.harvard.edu/~dbaron/css/test/sec1706c

with 10/6 build on Win, Mac and Linux, verified bug fixed. 'border-collapse: 
collapse' is turned off. Will contact Karnaze regarding 'rules'

Comment 21

[fantasai]

karnaze, you forgot to disable nsTableFrame::ProcessGroupRules()
rows=groups still affects the table

Comment 22

[fantasai]

sorry, that should be rules=groups :)