155955 - [BC] outer half of collapsed table border bleeds out of table box (border-collapse)

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Tarmo Järvalt]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1a+) Gecko/20020629
BuildID:    2002062904

Using style="float: left" on tag before table tag causes table (with
style="margin: 0px 0px 0px 1px" set) to have visible style of "margin-top: -1px,
margin-left: -1px" but DOM Inspector says that it has 0px margin all the way.

In second row containing div and table with 10px borders it is seen that the
elements collide.

This bug only appreas when running in strict mode, removing doctype and
therefore going to quirks mode displays it correctly.

Reproducible: Always

Steps to Reproduce:
1. Open testcase
2. See wether boxes are aligned and with 1px spacing

Actual Results:  table box is not correctly aligned

Expected Results:  table box is at same height and has 1px margin either side

M$ IE 6.0 displays the testcase as expected.

Comment 1

[Tarmo Järvalt]

Attachment: testcase, HTML CSS

Comment 2

[Tarmo Järvalt]

Attachment: expected rendering

Comment 3

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

Given 'border-collapse: collapse' this seems like the correct result.  See CSS2
17.6.2.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 5

[Mats Palmgren (inactive)]

It seems the origin of a collapsed border table is set where the upper-left
grid lines meet. See:
http://www.w3.org/TR/REC-CSS2/tables.html#collapsing-borders

I don't know if this is a bug or not.

--> HTMLTables.

Comment 6

[Tarmo Järvalt]

What would be the workaround to display floated table in strict mode Mozilla as
it is rendered with MSIE?

And how is it useful being rendered as it is currently by strict mode Mozilla?

Comment 7

[Mats Palmgren (inactive)]

After reading the spec some more, I agree with Tarmo - this is a bug.
It breaks the box model, CSS2 8.1 and 9.4.1 for example.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 9

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

Note the sentence:  "Note that in this model, the width of the table includes
half the table border."  I still don't think this is a bug.

Comment 10

[Mats Palmgren (inactive)]

Yes, I saw that sentence, it is also indicated in the figure directly above.
What I don't understand is why this has anything to do with the positioning of
the table. Assuming 'margin:0', then the "outer edge" is the "border edge" (8.1).
I don't see anything in 17.6.2 that indicates an exception to this rule.

Comment 11

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

Hmm.  I always interpreted that as meaning both outer width and inner width, but
I guess it could mean just inner width.

Comment 12

[fantasai]

There's also this:
"Note only half of the two exterior borders are counted in the table width; the
other half of these two borders lies in the margin area."

If the margin has zero width, how can 5px of border lie in it? The margin would
have to be at least 5px wide for a 10px collapsed border. The question is,
should the space be added to the margin or should the space collapse with it?

Comment 13

[Bernd]

fantasai: thats a really nice sentence, how did I overlook this treasure. We can
easily incorporate this in nsTableOuterFrame.cpp. This would yield that the
table does not leak outside the outer table frame. I like this. Probably adding
the space is the clearer way to go.

Comment 14

[fantasai]

Hmmm.. I think that even if we add the space for a border that takes effect
along the whole edge (table, row group, col group, col, row), we should collapse
any extra due to individual cells. Suppose I had
  <table>
    <tr><td>a</td><td>b</td></tr>
    <tr><td id="c">c</td><td>d</td></tr>
    <tr><td>e</td><td>f</td></tr>
  </table>

table {
  margin: auto;
  border: 2px solid;
  border-collapse: collapse;
  width: 80%;
}
#c {
  border: 10px solid;
}

We want the table to appear centered, but if the extra 4px gets added to the
left margin, it will be off-center.

Comment 15

[Bernd]

I would rather not collapse them for cell but let them leak out of the outer
table frame.

Comment 16

[fantasai]

If you let the borders leak out, they'll overpaint adjacent content. Like in my
testcases for 4510: the top border overwrites part of the caption. And, it says
"exterior borders", not "exterior row, col, table, colgroup, or row group
borders". :) So, you should find the greatest effective border width on that
side and the greatest continuous (row/col/row group/col group/table) border
width. Add the greatest continuous border width and make sure that the resulting
margin is at least as big as the greatest effective border width.


... Suppose that the table border is 10px, but all the cells along the top have
border: hidden. Would it make sense to leave room for a border that doesn't exist?

Comment 17

[Hixie (not reading bugmail)]

The current model, as I understand it, lets you lay out the first row of a fixed
layout table without knowing the border widths of subsequent rows' first and
last cells. If you change the model to have the table width go up to the widest
border, then your table width can change upon receiving the data for subsequent
rows.

I don't pretend to understand this stuff though. Did I mention I hate tables?

Comment 18

[Bernd]

If you would not hate tables, the css2.1 spec would be implementable and may be
self consistent and I would have a person to ask :-). At least the question has
been posted to w3c-style: 
http://lists.w3.org/Archives/Public/www-style/2003Jan/0230.html 

Did I mention that there was no reply, as usual with table related questions?

Comment 19

[fantasai]

Ian -
In border-collapse mode, table width is measured from the center of the border,
not from the edge. http://www.w3.org/TR/REC-CSS2/tables.html#collapsing-borders

The extra width from the border would become part of the margin, so fixed layout
would still work. (The margin only affects the table width if the width is auto,
and if the width is auto we use auto layout.)

Comment 20

[Hixie (not reading bugmail)]

fantasai: yes, but haven't you requested that that be changed?

Comment 21

[Bernd]

Attachment: patch

Comment 22

[fantasai]

> fantasai: yes, but haven't you requested that that be changed?

No, Bernd did. I don't agree with him on that. :) I still think the extra width
should be part of the margin--and if you consider table backgrounds (*grin*)
you'll see that it only makes sense this way. I should've replied by now, though.

Comment 23

[karnaze (gone)]

mass reassign to default owner

Comment 24

[Mats Palmgren (inactive)]

See also bug 170281 (testcase: attachment 100223 [details])

Comment 25

[Mats Palmgren (inactive)]

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

Comment 26

[fantasai]

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

Comment 27

[fantasai]

http://fantasai.inkedblade.net/style/discuss/collapsed-outer-border/

At the 2004 Plenary, the CSS WG decided to leave this detail of border-collapse
behavior undefined. I recommend we follow Opera's example and implement what
I've decribed in the www-style proposal above.

Comment 28

[fantasai]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

Comment 30

[Mats Palmgren (inactive)]

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

Comment 31

[Mats Palmgren (inactive)]

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

Comment 32

[Mats Palmgren (inactive)]

(In reply to comment #27)
> http://fantasai.inkedblade.net/style/discuss/collapsed-outer-border/
> 
"The solution ... and add it to the table margin."

Instead of adding it one could perhaps inflate it instead?
(That is, the margin does not change when the border fits inside and is only
made wide enough, but not more, when the border does not fit.)

Comment 33

[Bernd]

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

Comment 34

[Benjamin Streeck]

As long as this bugs lacks a real solution due to a lack of concesus on what to
do here (or where is the problem now?), shouldn't an intermediary solution fix
the different calculations for left/right and top/bottom positioning as
demonstrated by https://bugzilla.mozilla.org/attachment.cgi?id=174298 (from dupe
https://bugzilla.mozilla.org/show_bug.cgi?id=282224 -- sorry!)? Then at least
the behavior is calculatable, and the discussion could go on forever, as much as
I care.

FYI: Opera does it just like the attachment proposes, and not not as described
by comment #27. Version 7.54 and 8.0b1.

Comment 35

[Bernd]

The problem is that this bug has lower priority, than http://tinyurl.com/3hw8y.
Feel free to fix it, ask me if you need help.

Comment 36

[Julien "_FrnchFrgg_" RIVAUD]

Should'nt the title of the bug be changed to reflect its evolution ?

I agree that the current way, people not understanding (or not reading at all)
the W3C specs and filing a bug can see there is already one.
Yet, the fact that the border bleeds outside of table is no longer considered
the bug to correct, but rather "outer half of collapsed table border should
inflate/extend/... the table margin to prevent overlap" or something like that.

Doing so, the bug may become invisible to people wanting the border to be _in_
the box, but volontary programmers could discover a bug they _do want_ to fix
because it better fits their view of the world.

_FrnchFrgg_

P.S.: Is it complicated to a newbie in Mozilla's code (but not in coding) to
take over this bug and try to write a patch ? Given a hint as to the place where
collapsed border's code hides, I could try (and maybe loose, you've been warned ;)

Comment 37

[Bernd]

>Is it complicated to a newbie in Mozilla's code (but not in coding) to
take over this bug and try to write a patch ?

Hmm it would be not fair to say its easy for a newbie however its feasible its
not rocket science.

The table margin handling is done at
http://lxr.mozilla.org/seamonkey/source/layout/tables/nsTableOuterFrame.cpp

But the first step is to get a working build (http://www.mozilla.org/build/)

Comment 38

[Julien "_FrnchFrgg_" RIVAUD]

(In reply to comment #37)

Shouldn't it be in nsTableFrame ? Because it is its margin that matters, and I
would rather have a table border non leaking into it's caption frame, do I ?

_FrnchFrgg_

Comment 39

[Bernd]

No, its the outer table frame that takes care of its child (the table itself and
the caption) margins.

Comment 40

[Julien "_FrnchFrgg_" RIVAUD]

What I meant is :
If there exists a function that returns margins of the (inner child) table, and
whose result is used by the outer container to a) calculate its caption position
& size, and b) exhibit some margins too (I'd say 3 unmodified margins of the
table, and one of the caption), I'd rather modify this one.

And I would have searched such a function in the inner table class, because it
seems to belong to it.

Is GetMarginPadding(...) such a function ? And in some other functions
OuterReflowChild(...), nearly identical code is used as in GetMarginPadding. The
main problem for me is that not knowing exaclty the way you reflow and when you
calculate margins, I have to guess, and I may gess wrong, or forgot somewhere...
I'll try to go on the dev forums to find help, unless some short explaination
can be given here without bloating the bug and taking to much time of the happy
few who know.

_FrnchFrgg_

Comment 41

[fantasai]

Making the margin adjustments in nsTableOuterFrame.cpp would not give the
intended effect when there is a caption. The changes must be in nsTableFrame.cpp.

I just went looking through the minutes of the csswg meeting. It was suggested
that the collapsed border width figure in the calculation of the table's
computed border width.

I think I need to run a few tests on what Opera's doing before we decide how
exactly we're going to fix this bug.

Comment 42

[Bernd]

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

Comment 43

[Bernd]

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

Comment 44

[Bernd]

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

Comment 45

[Bernd]

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

Comment 46

[Hixie (not reading bugmail)]

For the record, the spec changed several times on this. When fixing this bug, make sure to read the new text (section 17.6.2), in particular about how the first row is magical and never spills out, but that subsequent rows may spill out if they have wider borders:

# UAs must compute an initial left and right border width for the table by 
# examining the first and last cells in the first row of the table. The left 
# border width of the table is half of the first cell's collapsed left border, 
# and the right border width of the table is half of the last cell's collapsed 
# right border. If subsequent rows have larger collapsed left and right borders, 
# then any excess spills into the margin area of the table.

Comment 47

[Frank Wein [:mcsmurf]]

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

Comment 48

[Mats Palmgren (inactive)]

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

Comment 51

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

Note that when we implement what the spec says, I think we can do it in quirks mode too and remove our implementation of a similar idea (but somewhat different rules) in quirks mode.

Comment 55

[Glen A.]

What is the reason for calculating the width and height of the table using half of the border width/height? Wouldn't it be more logical to draw the left/right and top/bottom borders of the table inside the grid lines? (http://www.w3.org/TR/REC-CSS2/tables.html#collapsing-borders)

If you open the following page (http://dev.l-c-n.com/tables/borderCollapseTest.html) and use Firebug (or similar) to change the border-color of the divs to green (#0C0), you can see how ridiculous it looks at the moment with no table margins (compared to the 'border-collapse off' rendering).

I don't see the point/meaning of auto-adding margin just to accommodate the border. It doesn't make sense placing part of the border into the margin, IMHO. Aren't margins supposed to be empty, and used for spacing between elements?

I don't really know much about this subject, so please keep that in mind when reading the above.

Comment 58

[Bernd]

fantasai: we have three functions at http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.h#291

in a new CSS 2.1 compliant world what would the tree functions return

 nsMargin GetOuterBCBorder() const;

the new table border width? as defined by the width on the first and last cell on the first row together with the top border and bottom border on the first cell in the first and last row?

 nsMargin GetIncludedOuterBCBorder() const;

the new table border width?

 nsMargin GetExcludedOuterBCBorder() const;

0?

Comment 60

[negora]

"That is actually what the spec says.
http://www.w3.org/TR/CSS21/tables.html#collapsing-borders"

>>> philippe: But that part of the specification only states how the collapsed borders must be rendered according to the internal table grid, not how the whole border must be aligned with the container box, Right?

In my opinion that text "only" means that if you apply a border of 4 px, 2 px of it will be put out of the grid line and 2 px will be put inside. That doesn't mean that the alignment of the table has to be referred to the mentioned grid line. There's no line which says something like that. Am I wrong?

Indeed, doing that would contradict the "sense" of the CSS borders, as they take a room and they "push" any surrounding element (except if you use rules to alter that, obviously). The present implementation converts the border into a weird mixture: The inner half behaves like a border, whereas the external half acts like an outline.

Comment 61

[fantasai]

Based on the semantics of these functions,

  nsMargin GetOuterBCBorder() const;
would return everything that spills out, as I imagine it does today

  nsMargin GetIncludedOuterBCBorder() const;
would be:
  * on the left, 1/2 the collapsed left border width on the first
    cell in the first row
  * on the right, 1/2 the collapsed right border width on the last
    cell in the first row
  * on the top, 1/2 the widest collapsed top border segment on the
    first row
  * on the bottom, 1/2 the widest collapsed bottom border segment
    on the last row

  nsMargin GetExcludedOuterBCBorder() const;
would return GetOuterBCBorder() - GetIncludedOuterBCBorder()

GetIncludedOuterBCBorder() would be used for width/margin calculations and would be included in table background painting. GetExcludedOuterBCBorder() would be used only for calculating overflow.

Personally, I don't like these rules. I think
  - the table's own border should be used for GetIncludedOuterBCBorder()
  - that GetOuterBCBorder() should be calculated as 1/2 max collapsed
    border on all sides, falling to the 2.1 rules only for fixed tables
  - that GetExcludedOuterBCBorder() should collapse with the table's
    margins
but that's not what 2.1 says...

Comment 62

[Bernd]

fantasai: thanks,

is there a reason why the left/right  and top/bottom borders are treated differently?

>but that's not what 2.1 says...
you should talk to the editors ;-)

Comment 63

[fantasai]

The goal there was to avoid two passes over the table.

Comment 64

[Jan David]

My 3.0.3 Firefox browser was updated to 3.0.4 and the left border of my home page disappeared. It's a xhtml 1.0 strict validated page. It used to work perfectly in 3.0.3.

Here's my home page: http://www.susceptor.be

Comment 65

[Ilya]

Hi all. This bug really strains. Somebody knows when will correct?

Comment 66

[Bernd]

Attachment: patch

This does roughly what is required, it lacks testing.

Comment 67

[Bernd]

re comment 64 Jan this is a old bug it is certainly unrelated to what you see.

Comment 68

[Bernd]

Attachment: patch

Comment 69

[fantasai]

Comment on attachment 355133 [details] [diff] [review]
patch

Please update the comments in nsTableFrame.h for the functions you are changing.

+  <tr><td style="border:solid 4px red; height:30px">cell 1</td></tr>

Use a color other than red, since the presence of red is supposed to indicate failure.

+  if (propData) {
+    border.top += BC_BORDER_TOP_HALF_COORD(p2t, propData->mTopBorderWidth);

This should be =, not +=.

-  nsMargin offset(0,0,0,0);
+ nsMargin offset(0,0,0,0);

Stray whitespace deletion.

r=fantasai otherwise

Comment 70

[Bernd]

Attachment: revised patch

Comment 71

[Bryan]

Should this be fixed in the following build?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090208 Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090208181212

The test case is still not proper if it's supposed to be.

~B

Comment 72

[Bernd]

If you do not see this in a build from 2009-02-09 please reopen the bug, there are several reftests that run on tinderbox that rely on this changed behaviour.

Comment 73

[Bryan]

Still not fixed in: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090209 Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090209020258

~B

Comment 74

[Martijn Wargers (dead)]

Bryan, what exactly is not fixed? You mean the first testcase in this bug?

Comment 75

[Bryan]

Martijn,

Yes, the first testcase.  Maybe I've misunderstood this bug.

~B

Comment 77

[Jim Jeffery not reading bug-mail 1/2/11]

Did this patch actually land somewhere ?  I don't see where/when/by whom it was checked into either trunk or branch.  Perhaps 'fixed' by another bug that was not referenced ?

Comment 78

[Mats Palmgren (inactive)]

It's fixed on trunk AFAICT:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/
as of 2009-02-08 it seems:
http://hg.mozilla.org/mozilla-central/rev/fc7431820ea3

Comment 80

[Johannes Langlotz]

Attachment: test case

Comment 81

[Johannes Langlotz]

OK, seems to be fixed in 3.6a1pre! Will this patch also be applied to 3.5?

Comment 82

[Ryan VanderMeulen [:RyanVM]]

Not likely.

Comment 84

[Ria Klaassen (not reading all bugmail)]

Has this bug fixed Bug 489279 ?

Comment 85

[soandso]

Issue no longer shows up after install of 3.6a1! :)